Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

chore: Handle forward references, repeatable annotations, and use str enums #43

Merged
merged 1 commit into from
May 2, 2024
Merged

chore: Handle forward references, repeatable annotations, and use str enums #43

merged 1 commit into from
May 2, 2024

Conversation

Christopher-Chianelli
Copy link
Collaborator

  • Previously, we eagerly compile the class as soon as @planning_entity or @planning_solution is reached. This cause problem if the class contains forward references, since:

    • The referenced class does not exist when @planning_entity or @planning_solution is reached, since it is defined later.

    • This causes get_type_hints to raise a NameError, since it cannot find the type name in locals or globals

    Now, the classes are compiled when the SolverConfig is read (and
    thus, all the referenced classes should be defined).

  • In order to handle forward references in annotations and the class translator, usage of getJavaClass (which may throw an exception if the class is in the middle of being defined) need to be changed to getJavaClassInternalName (which will never throw). Thus:

    • ArgumentSpec now take the return type and parameters of a function as String instead of their Class. There could be a Class overload that calls the String version, but I decided against it to prevent the temptation to pass getJavaClass to it.

    • PythonDefaultArgumentImplementor now sets its constants inside , which means ArgumentSpec must store itself in a static field so clinit can access it. This is because if a Class cannot be loaded until all the Classes it references are defined (and thus, we cannot set the field values from Java, since PythonDefaultArgumentImplementor might reference a class still being defined).

    • AnnotationMetadata now store Class values as Type instead of Class.

  • In order for Timefold to discover the "true" type of annotated getters, we remove NoneType from the Union of the getter return type (but keep it in the actual field type). That is, if a type is annotated Value | None, the getter type will be Value, and the field type will be get_common_ancestor(Value, NoneType) = object.

  • Use PythonClassWriter instead of ClassWriter in the ClassTranslator, so ASM will not complain about missing classes when computing frames.

  • In order to properly visit a repeatable annotation, we need to group the repeated annotation by type and put them as the value of their container annotation class.

  • Make VersionMapping throw an exception if it gets a null version mapping (otherwise, an undescriptive NPE will happen if the unsupported opcode is in the bytecode).

  • Added the missing PreviousElementShadowVariable and NextElementShadowVariable annotations.

  • Use str enums, so the enum serializes to their names instead of a number.

… enums

- Previously, we eagerly compile the class as soon as `@planning_entity`
  or `@planning_solution` is reached. This cause problem if the class
  contains forward references, since:

  - The referenced class does not exist when `@planning_entity` or
    `@planning_solution` is reached, since it is defined later.

  - This causes `get_type_hints` to raise a NameError, since it
    cannot find the type name in locals or globals

  Now, the classes are compiled when the SolverConfig is read (and
  thus, all the referenced classes should be defined).

- In order to handle forward references in annotations and the class
  translator, usage of getJavaClass (which may throw an exception if the
  class is in the middle of being defined)  need to be changed to
  getJavaClassInternalName (which will never throw). Thus:

  - ArgumentSpec now take the return type and parameters of a function
    as String instead of their Class. There could be a Class overload
    that calls the String version, but I decided against it to prevent
    the temptation to pass getJavaClass to it.

  - PythonDefaultArgumentImplementor now sets its constants inside
    <clinit>, which means ArgumentSpec must store itself in a static
    field so clinit can access it. This is because if a Class cannot
    be loaded until all the Classes it references are defined (and
    thus, we cannot set the field values from Java, since
    PythonDefaultArgumentImplementor might reference a class still
    being defined).

  - AnnotationMetadata now store Class values as Type instead of
    Class.

- In order for Timefold to discover the "true" type of annotated
  getters, we remove NoneType from the Union of the getter
  return type (but keep it in the actual field type). That is,
  if a type is annotated `Value | None`, the getter type will
  be `Value`, and the field type will be
  `get_common_ancestor(Value, NoneType) = object`.

- Use PythonClassWriter instead of ClassWriter in the
  ClassTranslator, so ASM will not complain about missing
  classes when computing frames.

- In order to properly visit a repeatable annotation, we need
  to group the repeated annotation by type and put them as the
  value of their container annotation class.

- Make VersionMapping throw an exception if it gets a null version
  mapping (otherwise, an undescriptive NPE will happen if the
  unsupported opcode is in the bytecode).

- Added the missing PreviousElementShadowVariable and NextElementShadowVariable
  annotations.

- Use str enums, so the enum serializes to their names instead of a
  number.
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

One thing that's not obvious from the test coverage is how was this problem discovered. I'm guessing it comes from writing the quickstarts?

Side note: if this PR were two PRs (one for fwd refs and the other for repeatable annotations), it'd have been easier to review.

@Christopher-Chianelli
Copy link
Collaborator Author

The forward reference issue was discovered over 1 year ago (that is, this is an issue I knew about since OptaPy), and was always on the back burner; Vehicle Routing requires it to be fixed due to the reference cycle between Visit and Vehicle.

The repeatable annotation issue was discovered because Vehicle's variable listener has two different sources.

The other mini-issues (str enum, missing annotations, etc.) were things discovered when writing the quickstarts.

@Christopher-Chianelli Christopher-Chianelli merged commit e3214be into TimefoldAI:main May 2, 2024
3 checks passed
@triceo triceo deleted the investigate-slow-3.12 branch May 2, 2024 06:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants