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 Import Resolution To Use a Package Repository #1768

Closed
8 tasks
radeusgd opened this issue May 29, 2021 · 1 comment · Fixed by #1822
Closed
8 tasks

Refactor Import Resolution To Use a Package Repository #1768

radeusgd opened this issue May 29, 2021 · 1 comment · Fixed by #1822
Labels
p-high Should be completed in the next sprint

Comments

@radeusgd
Copy link
Member

radeusgd commented May 29, 2021

Summary

We want to introduce a concept of a PackageRepository which resolves library names to library paths and downloads any missing libraries.

This part consists of creating a dummy PackageRepository that is still using the old import logic but most importantly refactoring the ImportResolver to use the PackageRepository interface, allowing for future migration.

Value

  • First step towards dynamically loading libraries.

Specification

  • Move the Context initialization logic that is related to preloading libraries into a legacy implementation of PackageRepository.
  • Make the ImportResolver use a PackageRepository for requesting Modules.
  • Add a callback that is called when PackageRepository encounters a new library (that was not previously loaded) which notifies the language-server (if it is connected) that a new content root may be added.
  • Make sure that module update logic also uses the PackageRepository.
    • That includes all relevant references within the Context, for example the packages list should also be updated to rely on the PackageRepository.
  • Update the TopLevelScope to refer to the PackageRepository.
    • In particular, it may be good to rename operations like getModules to getLoadedModules.

Acceptance Criteria & Test Cases

  • The above specification is completed.
  • The import resolution logic and the callback have been tested.
@radeusgd radeusgd added Category: Libraries p-high Should be completed in the next sprint labels May 29, 2021
This was referenced May 29, 2021
@radeusgd
Copy link
Member Author

radeusgd commented Jun 2, 2021

Old specification for historical reference:

Summary

When a new library is added to the project, it needs to be loaded. We want to avoid restarting the whole context if possible, to not disrupt user's workflow.

I'm thinking that we may want to split this task into two parts - one for dynamically adding modules and another for managing when to do this. But some parts of it are highly coupled, so I'd like to discuss this on Tuesday @MichaelMauderer @kustosz.

While the import logic is being updated (especially the logic for triggering loading new packages), the following related issues may be fixed by the way: #1569, #1756, #1683.

Value

  • We can add libraries without disrupting user's workflow.

Specification

  • It should be possible to dynamically add the library to the TopLevelScope without reloading the whole context.
  • Conflicting Java classpaths should be handled as best as possible.
    • It is most likely infeasible to check all namespaces added when a library is added.
    • But when a Java class is being loaded and there is ambiguity - there are differing JARs which contain this class, an error should be raised.
    • However, ideally, if two libraries include the same JAR, there should be no error (?).
  • Adding imports should cause to load the library if it was not loaded before.
    • I'd suggest just overriding the import mechanism in such a way that if it fails to find the requested module, it tries to resolve the related library and load it before retrying (however I'm not sure if this is a viable approach, so I'd like to discuss this @kustosz).
    • If we do the above, we would likely not need any separate logic for loading libraries at startup, because we could just use this mechanism to load them lazily.
    • The above mechanism must also work with transitive dependencies - the modules that are imported may depend on other libraries etc.
  • We should discuss how the above mechanism should behave if the library that is being imported is not yet installed.
    • This scenario should be supported, because while the standard way to add libraries will be through the IDE's interface and then IDE can coordinate that the library should be installed first and only then the import should be added to the code. But the user can always manually edit the code and add the imports, so it is very easy to trigger this kind of condition and getting import errors may be confusing.
    • On the other hand, downloading a library may take a lot of time - we probably don't want to freeze the execution context for so long, so ideally, there should be a mechanism that will ignore the import error in execution (but indicate it somehow to the IDE), trigger the installation and once it is completed re-load the import.
    • This may need to be a separate task, but it has to be discussed first.

@radeusgd radeusgd changed the title Dynamically Loading Libraries Refactor Import Resolution To Use a Package Repository Jun 2, 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 a pull request may close this issue.

1 participant