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

Dynamically Loading Libraries #1826

Merged
merged 19 commits into from
Jul 5, 2021
Merged

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jun 30, 2021

Pull Request Description

Refactors the modules and packages systems so that they can be loaded dynamically.

Related to #1781, but does not implement its full spec (edition resolution is left for a separate PR).

It also implements an interface allowing to use the standard SLF4J logging utils from within the runtime; this interface is connected to the TruffleLogger which then passes the logs to our logging service.

Important Notes

  • I removed the concept of 'shadowed packages' as it does not seem to be well-defined in the context of the new edition-based resolution system, but I'm willing to discuss if this should be handled differently. Main point is - we do not know what packages are available (well, we could look through what is defined in the edition + what is available on all paths in the local search path) so it is unclear if something is shadowing something else.
    • The only reasonable situation seems to report that the current project is shadowing some library that is defined within the edition - this could indeed be checked.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@radeusgd radeusgd self-assigned this Jun 30, 2021
@radeusgd radeusgd added Category: Interpreter p-high Should be completed in the next sprint labels Jun 30, 2021
@radeusgd
Copy link
Member Author

radeusgd commented Jul 2, 2021

  • I removed the concept of 'shadowed packages' as it does not seem to be well-defined in the context of the new edition-based resolution system, but I'm willing to discuss if this should be handled differently. Main point is - we do not know what packages are available (well, we could look through what is defined in the edition + what is available on all paths in the local search path) so it is unclear if something is shadowing something else.

    • The only reasonable situation seems to report that the current project is shadowing some library that is defined within the edition - this could indeed be checked.

cc: @kustosz @iamrecursion any thoughts about this?

@radeusgd radeusgd marked this pull request as ready for review July 2, 2021 19:09
@radeusgd radeusgd force-pushed the wip/rw/dynamically-loading-libraries branch from dbd1c75 to 8bd8be2 Compare July 5, 2021 10:12
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Code looks good, but there's an architectural problem with this solution. You have the context (which is "deep compiler, here be dragons") talking directly to the language server (which is "what compiler, me just want code go brrr"). This skips a layer of our runtime server, responsible for translating between the compiler and the LS. This will probably result in very client-specific logic being added to the context over time. All of this logic should be mediated by our pre-existing handlers system.

Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Just some notes on documentation. I'd like to echo what Marcin has said in his review.

RELEASES.md Outdated Show resolved Hide resolved
docs/libraries/sharing.md Show resolved Hide resolved
docs/libraries/sharing.md Outdated Show resolved Hide resolved
docs/libraries/sharing.md Outdated Show resolved Hide resolved
docs/libraries/sharing.md Outdated Show resolved Hide resolved
docs/libraries/sharing.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

LGTM

var notificationHandler = new Forwarder();
boolean isInteractiveMode = env.getOptions().get(RuntimeOptions.INTERACTIVE_MODE_KEY);
boolean isTextMode = !isInteractiveMode;
if (!isTextMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are you just inverted interactive mode twice, or I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that is a mistake, it was supposed to be if(isTextMode)! Thanks!

@radeusgd radeusgd merged commit e58b5eb into main Jul 5, 2021
@radeusgd radeusgd deleted the wip/rw/dynamically-loading-libraries branch July 5, 2021 22:27
@radeusgd radeusgd mentioned this pull request Jul 7, 2021
4 tasks
iamrecursion pushed a commit that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-high Should be completed in the next sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants