Dead code analysis does not catch use of aliased traits #6234
Comments
JoshuaBatty
added a commit
that referenced
this issue
Jul 15, 2024
## 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 test |
esdrubal
pushed a commit
that referenced
this issue
Aug 13, 2024
## 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This will result in a warning that
lib::Traitis never used, even though it is.The text was updated successfully, but these errors were encountered: