-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix performance and semantic problem when creating Module
s and make Module::name
mandatory
#5498
Comments
…le (#5577) ## Description This PR is the first step of a large refactoring of the `namespace` module. The PR removes the `DeRef` and `DeRefMut` implementations in the `namespace` module, as they make refactoring more difficult to keep track of. This change means that calls to `Root` and `Module` must now explicitly go through `Namespace`, since that is the type that is passed along during typechecking. I'm submitting the PR now because there have been changes to `master` while I've been working on this, and the changes are really difficult to merge, so I want `master` to be relatively stable. Subsequent PRs will: - Eliminate the `dst` parameter from all import functions (the destination is always the current module). - Move all import functions to `Namespace` (they currently reside in `Module` where they aren't needed, and where they require a clone the module path to work). - Move `Namespace::init` to `Root`, and change how it is used (it currently holds a constant declaration of `CONTRACT_ID` for contract modules, and that declaration is cloned into every submodule. The nice way to do this is to declare it in the root module and implicitly import it into each submodule). - Eliminate the `Option` part of `Module::name` (modules are not allowed to be nameless - the option only exists because they root module can in principle be nameless, but it ought to be named based on the package the module structure is in). - Fix the import and name resolution functions so that they don't treat absolute paths as relative (this should fix some of the performance issues we're seeing). - Make `Namespace::root` a reference so that the entire module structure isn't cloned for every submodule (this requires a change in ownership of the root module). - Introduce reexports in the form of `pub use`. This PR is the first step in fixing #5498. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
…le (#5577) ## Description This PR is the first step of a large refactoring of the `namespace` module. The PR removes the `DeRef` and `DeRefMut` implementations in the `namespace` module, as they make refactoring more difficult to keep track of. This change means that calls to `Root` and `Module` must now explicitly go through `Namespace`, since that is the type that is passed along during typechecking. I'm submitting the PR now because there have been changes to `master` while I've been working on this, and the changes are really difficult to merge, so I want `master` to be relatively stable. Subsequent PRs will: - Eliminate the `dst` parameter from all import functions (the destination is always the current module). - Move all import functions to `Namespace` (they currently reside in `Module` where they aren't needed, and where they require a clone the module path to work). - Move `Namespace::init` to `Root`, and change how it is used (it currently holds a constant declaration of `CONTRACT_ID` for contract modules, and that declaration is cloned into every submodule. The nice way to do this is to declare it in the root module and implicitly import it into each submodule). - Eliminate the `Option` part of `Module::name` (modules are not allowed to be nameless - the option only exists because they root module can in principle be nameless, but it ought to be named based on the package the module structure is in). - Fix the import and name resolution functions so that they don't treat absolute paths as relative (this should fix some of the performance issues we're seeing). - Make `Namespace::root` a reference so that the entire module structure isn't cloned for every submodule (this requires a change in ownership of the root module). - Introduce reexports in the form of `pub use`. This PR is the first step in fixing #5498. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
## Description This PR contains the final bit of refactoring before I start fixing the import problems mentioned in #5498. The semantics of imports has so far been implemented in the `Module` struct, and has assumed that the destination path of an import was relative to `self`. In particular, this allows imports to any submodule of the current module. However, whenever that logic was triggered by the compiler the destination path was always absolute. The only reason this worked was that the logic was only ever triggered on the root module. To reflect this behavior I have therefore moved the import semantics to the `Root` struct, and made it clear in the comments that the paths are assumed to be absolute. This also reflects the fact that once the module structure has been populated with the imported entities we don't actually need the import logic anymore, so it shouldn't be located in the `Module` struct. (Name resolution should obviously stay in the `Module` struct, so that logic has not been moved). I have left a couple of TODOs in the code to remind myself where I need to make changes when I start implementing `pub use` (see #3113 ). ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: IGI-111 <igi-111@protonmail.com>
As mentioned elsewhere this issue has blown up to an unmanageable size, and it's not helped by us not having documented the intended semantics of imports and their name resolution anywhere. To get a feel for the intended semantics of our current implementation, I took at look at the tests we currently have in our test suite. I don't know where else to put them, but I would like for them to be somewhere we can all see them: Failing tests
Passing tests:
Missing tests:
In other words, we are missing about as many test cases as we actually have. |
@tritao: This is the one. |
## Description Fixes #5913 . Name resolution for imports and top-level items in module currently happens at the module level, in part by traversing the submodules of the current module. This PR moves name resolution to the root of the module structure. This refactoring is needed to enable fixes to the following issues: - #5713 : The resolved type declaration must be understood in the context of the declaring module rather than in the context of the importing module, which means that name resolution must happen across the module structure. - #5498 : The modules from which items are imported are currently introduced as submodules to the importing module (in addition to their absolute location in the module hierarchy). To remove those submodules the name resolution algorithm needs access to the full module hierarchy, which it can get by having access to the root of the hierarchy. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: IGI-111 <igi-111@protonmail.com> Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
…Decode (#6044) ## Description Fixes #5500 . Also fixes the issue that caused one of @xunilrj 's tests to fail. The callpaths generated for struct declarations have so far been incorrect. The `shadowed_glob_imports` test case shows how this causes a compilation error when there is a name clash between a star imported struct and a locally declared struct. This causes a number of issues, in particular that the generated trait impls of `AbiEncode` and `AbiDecode` can refer to the wrong type or to a type that doesn't exist. The incorrect paths were the result of a combination of two bugs: 1. The `is_external` flag on the containing modules was often set incorrectly. In particular, `core` often had `is_external = false`, leading to the package name being added to the path of anything declared in `core`. 2. When typechecking a struct/enum declaration we would look up paths in the current environment before the name of the struct/enum was bound in the environment. If a type with the same name had already been imported this lookup would result in a path to the import source module rather than the path to current module (which is obviously where the struct/enum is located). This PR fixes both bugs: 1. When initializing the root namespace we now ensure that all modules that have already been added are marked with `is_external = true`. This happens in `Namespace::init_root()`. 2. When typechecking a type declaration we generate the callpath directly from the name of the type and the path of the current module. This happens in `CallPath::ident_to_fullpath()`. Step 1 unfortunately adds an extra cloning step in the `namespace` module, but I don't think it's avoidable at the moment. To avoid this extra cloning step we would need to have a better way of building the external module structure than we currently have. #5498 tracks this and other issues. The improved callpath generation should move us a step closer towards fixing #4700. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Igor Rončević <ironcev@hotmail.com> Co-authored-by: João Matos <joao@tritao.eu>
## Description Fixes #3113 . Closes #2767 as Won't Fix, since one must use `pub use` to reexport items. This PR Introduce reexports through the use of `pub use`. The implementation now keeps track of the visibility of a `use` statement, and that visibility is in turn taken into account when items are imported from the module where the `use` statement resides. This feature allows `std::prelude` and `core::prelude` to be handled correctly, instead of the special-case handling that we have been using so far. The method `star_import_with_reexports` has therefore been merged with `star_import`, and the `prelude.sw` files have accordingly been changed to use `pub use` instead of `use`. Note that `sway-lib-std/lib.sw` has a spurious import of `core::*`, which has no effect, so I have removed that import. This change also allows us to remove a hacky solution in `lexical_scope.rs`. The hack was introduced because the special-case handling of preludes meant that the preludes would contain multiple copies of the same `std` and `core` items. Now that we no longer need to treat the preludes specially, we can remove the hack. I have gone a little overboard in adding tests, partly because I wanted to make sure I hadn't missed some corner cases, but also because our tests of the import logic has poor code coverage. I have found the following issues that I don't think belongs in this PR, but which need to be handled at some point: - Path resolution is broken. The tests `should_pass/language/reexport/simple_path_access` and `should_fail/language/reexport/simple_path_access` are supposed to test that you can access reexported using a path into the reexporting module, i.e., without importing them using a `use` statement. Both tests are disabled because path resolution is broken. I plan to fix that as part of #5498 . A simlar problem exists in the test `should_pass/language/reexport/reexport_paths` where an item is imported via `std::prelude`. - There is an issue with the import and reexport of enum variants, which is illustrated in `should_pass/language/reexport/reexport_paths_external_lib`. In short, `pub use ext_3_lib::Items3_Variants::U` elevates the name `U` to the top-level namespace in a module, and this makes `U` inaccessible as a variant of `Items3_Variants`. I'm not sure how this is supposed to work, but since it's related to all imports whether they are reexported or not I have left it as a separate issue #6233 . - Aliasing of enum variants (`use lib::Enum::X as MyX`) doesn't work properly. In some cases `MyX` is available in the way you would expect, and in other cases it is not, and sometimes it is available but not recognized as being a variant of `lib::Enum`. The test `should_pass/language/reexport/aliases` has a number of tests of these types of things. #6123 tracks the issue. - Aliased traits are not handled correctly by the dead code analysis. The test `should_pass/language/reexport/aliases` generates warnings about unimplemented traits that are actually implemented, but since they are aliased in imports the DCA doesn't catch them. #6234 tracks the issue. Re. documentation: The whole import system is completely undocumented in the Sway book, and I plan to write up a draft how it works when I'm done fixing (the majority of) the many issue we have there. At the very least I'd like to postpone the documentation efforts until path resolution works correctly, since that is key to how the import system works. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Joshua Batty <joshpbatty@gmail.com> Co-authored-by: João Matos <joao@tritao.eu>
The following tests are currently disabled, but should be reenabled as part of this PR:
|
## Description Partial fix of #5498 . So far the `name` field in `namespace::Module` has been public and of type `Option<Ident>`. This makes little sense, since the name of a module should always have a value, and should retain that value once it is set. This PR fixes this problem. The `visibility` and `span` fields have also been made private. `visibility` can be read-only, but there is a single instance in which the `span` field must be set later, so I have added a setter for that field too. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: IGI-111 <igi-111@protonmail.com>
## Description Fixes #3113 . Closes #2767 as Won't Fix, since one must use `pub use` to reexport items. This PR Introduce reexports through the use of `pub use`. The implementation now keeps track of the visibility of a `use` statement, and that visibility is in turn taken into account when items are imported from the module where the `use` statement resides. This feature allows `std::prelude` and `core::prelude` to be handled correctly, instead of the special-case handling that we have been using so far. The method `star_import_with_reexports` has therefore been merged with `star_import`, and the `prelude.sw` files have accordingly been changed to use `pub use` instead of `use`. Note that `sway-lib-std/lib.sw` has a spurious import of `core::*`, which has no effect, so I have removed that import. This change also allows us to remove a hacky solution in `lexical_scope.rs`. The hack was introduced because the special-case handling of preludes meant that the preludes would contain multiple copies of the same `std` and `core` items. Now that we no longer need to treat the preludes specially, we can remove the hack. I have gone a little overboard in adding tests, partly because I wanted to make sure I hadn't missed some corner cases, but also because our tests of the import logic has poor code coverage. I have found the following issues that I don't think belongs in this PR, but which need to be handled at some point: - Path resolution is broken. The tests `should_pass/language/reexport/simple_path_access` and `should_fail/language/reexport/simple_path_access` are supposed to test that you can access reexported using a path into the reexporting module, i.e., without importing them using a `use` statement. Both tests are disabled because path resolution is broken. I plan to fix that as part of #5498 . A simlar problem exists in the test `should_pass/language/reexport/reexport_paths` where an item is imported via `std::prelude`. - There is an issue with the import and reexport of enum variants, which is illustrated in `should_pass/language/reexport/reexport_paths_external_lib`. In short, `pub use ext_3_lib::Items3_Variants::U` elevates the name `U` to the top-level namespace in a module, and this makes `U` inaccessible as a variant of `Items3_Variants`. I'm not sure how this is supposed to work, but since it's related to all imports whether they are reexported or not I have left it as a separate issue #6233 . - Aliasing of enum variants (`use lib::Enum::X as MyX`) doesn't work properly. In some cases `MyX` is available in the way you would expect, and in other cases it is not, and sometimes it is available but not recognized as being a variant of `lib::Enum`. The test `should_pass/language/reexport/aliases` has a number of tests of these types of things. #6123 tracks the issue. - Aliased traits are not handled correctly by the dead code analysis. The test `should_pass/language/reexport/aliases` generates warnings about unimplemented traits that are actually implemented, but since they are aliased in imports the DCA doesn't catch them. #6234 tracks the issue. Re. documentation: The whole import system is completely undocumented in the Sway book, and I plan to write up a draft how it works when I'm done fixing (the majority of) the many issue we have there. At the very least I'd like to postpone the documentation efforts until path resolution works correctly, since that is key to how the import system works. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Joshua Batty <joshpbatty@gmail.com> Co-authored-by: João Matos <joao@tritao.eu>
## Description Partial fix of #5498 . So far the `name` field in `namespace::Module` has been public and of type `Option<Ident>`. This makes little sense, since the name of a module should always have a value, and should retain that value once it is set. This PR fixes this problem. The `visibility` and `span` fields have also been made private. `visibility` can be read-only, but there is a single instance in which the `span` field must be set later, so I have added a setter for that field too. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: IGI-111 <igi-111@protonmail.com>
The
Namespace::enter_submodule
method creates a new submodule by copying the entire content of theNamespace::init
into it.init
contains all the external modules with all their submodules, items, etc.This causes an obvious performance issue because literally every internal module contains all the modules of all the external modules recursively. Even within a simple project depending only on
std
we will still have all thecore
modules copied as submodules of all thestd
modules. In a typical workspace this will be true for all the external dependencies blowing up the memory and slowing down the compilation time because of extensive copying ofModule
s.Beside the performance problem, having all the external modules as submodules of each and every module also leads to nonsensical imports like these, which are currently all valid:
To fix this, we need to, obviously, stop copying the entire modules as submodules and refactor the way preludes are imported into namespace/module items.
As a part of that refactoring, one more point should be addressed. Currently,
Module::name
s areOption
s which does not correspond to the semantics of the modules - we do not have nameless modules. Thus every module must have a name, and this is currently not enforced by the type system. On the contrary, have thename
beingOption<Ident>
convey the message that we support nameless modules.Also, the root module must have a name coming from the package name. Making sure that the name actually exists is done by the callers, e.g., tooling and with internal methods like the mentioned
Namespace::enter_submodule
. Still, we had places in tests where root modules are created without a name. Having a root module without a name makes absolute paths ambiguous, because we can have internal libraries with the same name as some external package and the same inner module structure. Currently are such two indistinguishable.Why do we have then
Module::name
s asOption
in the first place? After going through the code, my conclusion is that it was done to support theNamespace::init
beingModule
. Since it was not a real module, there was no sense to have a name for it.IMO,
Module
a must havename
,visibility
, andis_external
set at creation.To sum it up, fixing the overall issues with the namespaces and modules should:
Module::name
beingIdent
instead ofOption<Ident>
Module::default()
and introduceModule::new(name, visibility, is_external
) and make these fields read-only, accessible through getters.Namespace::init
beingModule
containing all the external dependencies, introduce a mechanism to keep track of external dependencies and preludes and importing only the content of preludes into every new module.The text was updated successfully, but these errors were encountered: