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

Resolve fully qualified names #4056

Merged
merged 8 commits into from
Jan 18, 2023
Merged

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Jan 16, 2023

Pull Request Description

Added a separate pass, FullyQualifiedNames, that partially resolves fully qualified names. The pass only resolves the library part of the name and replaces it with a reference to the Main module.

There are 2 scenarios that could be potentially:

  1. the code uses a fully qualified name to a component that has been
    parsed/compiled
  2. the code uses a fully qualified name to a component that has not be
    imported

For the former case, it is sufficient to just check PackageRepository for the presence of the library name.
In the latter we have to ensure that the library has been already parsed and all its imports are resolved. That would require the reference to Compiler in the FullyQualifiedNames pass, which could then trigger a full compilation for missing library. Since it has some undesired consequences (tracking of dependencies becomes rather complex) we decided to exclude that scenario until it is really needed.

Important Notes

With this change, one can use a fully qualified name directly.
e.g.

import Standard.Base
main =
  Standard.Base.IO.println "Hello world!"

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 code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@hubertp hubertp force-pushed the wip/hubert/fqn-resolution-184213601 branch from 5c22872 to c054b8c Compare January 16, 2023 14:18
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

We have a CompilerBasedDependencyExtractor that is used to extract dependencies between libraries based on imports.

It is meant to be used to track dependencies between our libs, it is currently not used too heavily, although it is ran as part of generating a manifest for a package.

Since this PR introduces a change which makes libraries loaded not only by imports but also by qualified names, I'd like to ask:

  1. If we can make sure that this dependency extractor will also be able to detect which libraries are brought in as part of this pass? If so, it would be good to add a test extracting dependencies of Test_Fully_Qualified_Name verifying this works.
    1a. Alternatively, maybe we could actually make a pass which desugars the FQN into a regular reference + adding a fake import to the module? What do you think? Maybe then we could avoid modifications in the dependency extractor. It seems quite valuable to have a single place to find dependencies (the list of imports).

  2. Since this is not necessarily top priority, we may mark this for later, but I think we should create a Pivotal issue to tackle this at some point, to avoid figuring this out in the future by a bug where library dependencies are not tracked correctly.

@hubertp
Copy link
Collaborator Author

hubertp commented Jan 16, 2023

We have a CompilerBasedDependencyExtractor that is used to extract dependencies between libraries based on imports.

Wasn't aware of that. Thanks @radeusgd

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I have slight preference to smaller change - e.g. without invoking the compiler - if that works when the IDE imports from Standard.Base import all.

@hubertp hubertp force-pushed the wip/hubert/fqn-resolution-184213601 branch from c054b8c to 7fe6010 Compare January 17, 2023 15:24
Added a separate pass, `FullyQualifiedNames`, that partially resolves
fully qualified names. The pass only resolves the library part of the
name and replaces it with a reference to the `Main` module.

There are 2 scenarios covered in this change:
1) the code uses a fully qualified name to a component that has been
   parsed/compiled
2) the code uses a fully qualified name to a component that has not be
   imported

For the former case, it is sufficient to just check `PackageRepository`
for the presence of the library name. In the latter case we have to
ensure that the library has been already parsed and all its imports are
resolved. Hence the reference to `Compiler` in the `FullyQualifiedNames`
pass.
Don't trigger compilation of library late in the pipeline as this may
have undesired consequences for dependency resolutions.
This will be a known limitation of FQN.
We are no longer triggering compilation so that is not necessary.
Unfortunately, due to the way passes and metadata are currently modeled
we cannot pass `PackageRepository` directly to the pass and we have to
do it via context.
@hubertp hubertp force-pushed the wip/hubert/fqn-resolution-184213601 branch from 6bbd6a1 to 60992f2 Compare January 18, 2023 15:27
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jan 18, 2023
@mergify mergify bot merged commit d463a43 into develop Jan 18, 2023
@mergify mergify bot deleted the wip/hubert/fqn-resolution-184213601 branch January 18, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants