Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Scala definitions containing deeply nested classes to mitigate problems when accessing them from Java #7017

Closed
hubertp opened this issue Jun 12, 2023 · 6 comments · Fixed by #7681
Assignees
Labels
-compiler -tooling Category: tooling p-medium Should be completed in the next few sprints

Comments

@hubertp
Copy link
Collaborator

hubertp commented Jun 12, 2023

TreeToIr translates from new AST to Enso's internal IR. The class has been written in Java, by design and on purpose (reasons omitted), but it had to make use of the existing class hierarchy defined in IR.

While Scala and Java can co-exist, calling the former from the latter is not pleasant when dealing with nested classes. And Scala code often makes use nested classes. This is easily demonstrable just by looking at the imports of TreeToIr:
Screenshot from 2023-06-12 16-58-43
i.e. we can't use the typical "dot" syntax for nested classes. This makes it apparently impossible to recognize for IDE's, as demonstrated in the screenshot.

There are possible solutions (most of them hard to enforce because out of our control):

  1. Make sure Scala emits better InnerClasses attribute for deeply (2+ level) nested inner classes. Not sure if this is even possible given the rather complex interop logic (e.g. https://discord.com/channels/@me/959434339182067712/1116738522003550269). Will file a ticket in the Scala compiler.
  2. Make javac understand that nested classes belonging to companion objects are called Foo$Bar$Baz and not Foo$Bar$$Baz. Also unlikely to happen because most of the implementation follow blindly the $ pattern.
  3. Fix IDE - unlikely to happen in a timely manner, given that IntelliJ is closed source
  4. Refactor IR to multiple classes.

Due to the lack of other alternatives, this ticket proposes to pursue option 4. This bug slows down developer productivity quite significantly.

@hubertp hubertp added -tooling Category: tooling p-medium Should be completed in the next few sprints -compiler labels Jun 12, 2023
@hubertp hubertp self-assigned this Jun 12, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Jun 13, 2023
@hubertp
Copy link
Collaborator Author

hubertp commented Jun 13, 2023

Same situation with Runtime.scala

@hubertp hubertp changed the title Refactor IR.scala to mitigate problems with calling nested classes from Java Refactor Scala definitions containing deeply nested classes to mitigate problems when accessing them from Java Jun 13, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 29, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-08-28):

Progress: Started refactoring IR so that we have a more a shallow hierarchy of classes. Sadly IDE refactoring didn't give too much support in this case. Added more debugging info to #7624, will wait for feedback. Waiting on PR review for logging. Some planning discussions. It should be finished by 2023-08-30.

Next Day: Next day I will be working on the #7017 task. Continue with refactoring.

@jdunkerley jdunkerley moved this from 📤 Backlog to 🔧 Implementation in Issues Board Aug 29, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 30, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-08-29):

Progress: More refactoring work. It should be finished by 2023-08-30.

Next Day: Next day I will be working on the #7017 task. Continue with refactoring.

@mergify mergify bot closed this as completed in #7681 Aug 31, 2023
mergify bot pushed a commit that referenced this issue Aug 31, 2023
Refactoring deeply nested IR classes to shallow nesting.
Fixes #7017

# Important Notes
It's a big PR but fairly consistent. Split multiple hierarchy levels into subpackages.
@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Aug 31, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 31, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-08-30):

Progress: Finished refactoring while waiting on CI to stabilize. It should be finished by 2023-08-30.

Next Day: Next day I will be working on the #7017 task. Wait on CI to resolve its problems and merge PR. Come back to logging PR.

@enso-bot
Copy link

enso-bot bot commented Sep 1, 2023

Hubert Plociniczak reports a new 🔴 DELAY for yesterday (2023-08-31):

Summary: There is 1 day delay in implementation of the Refactor Scala definitions containing deeply nested classes to mitigate problems when accessing them from Java (#7017) task.
It will cause 0 days delay for the delivery of this weekly plan.

Delay Cause: Extra day to finish refactoring all classes

@enso-bot
Copy link

enso-bot bot commented Sep 1, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-08-31):

Progress: Took some extra time to make sure that everything works as expected and cleaned up remaining craft (and waiting on CI to be sane again). Started looking at the logging PR feedback. It should be finished by 2023-08-31.

Next Day: Next day I will be working on the #7253 task. Back to addressing logging feedback

mergify bot pushed a commit that referenced this issue Sep 12, 2023
Makes it clear that those IR entries are not needed for executing native runner.

# Important Notes
Follow up on #7017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -tooling Category: tooling p-medium Should be completed in the next few sprints
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant