-
-
Notifications
You must be signed in to change notification settings - Fork 892
Bugfix: Modules with same original name wrongly being marked as unused #5158
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
base: main
Are you sure you want to change the base?
Conversation
Also updated assert_no_warnings to avoid ambiguity in parsing test cases with multiple extra modules
1. Modules with an alias had to be marked as shadowed 2. When registering module references from callsites, we can't look up the module_name_to_node map as we have limited information about the module name
lpil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've left some questions inline.
compiler-core/src/reference.rs
Outdated
| } | ||
|
|
||
| pub fn register_module_reference(&mut self, name: EcoString) { | ||
| pub fn register_module_reference_for_unqualified_imports_and_aliases( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this new name. It seems to say something about the context in which it is used, but not name the function itself, and the meaning is unclear.
Could these two functions be given clearer names please, and documentation comments could be useful 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry about that. I've given them both better names now, relating to the sort of module name they take as arguments.
compiler-core/src/reference.rs
Outdated
| } | ||
|
|
||
| pub fn register_module( | ||
| pub fn register_unaliased_module( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this new name mean? What distinguishes it from the new function with the previous name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I was trying to work around making EntityLayer public and changing all the call sites of register_module, but it does seem cleaner to avoid the extra wrapper function. I've made the changes necessary to remove it.
Removed unnecessary register_unaliased_module function Renamed both register_module_reference_* functions to be clearer about intention Added documentation for register_module_reference_* functions
|
Hey @lpil, I'm just bumping this because I'm not sure if the inline responses show up as notifications. |
Description
This resolves #5145.
Three changes were made:
Shadowedlayer and not theModulelayer. This prevents it from shadowing previous imports with the same module name.module_name_to_nodemap should not be checked. This map contains full original names of modules (e.g.wibble/wobbleandwobble) and hence isn't guaranteed to point us to the correct node when we have limited information (e.g. justwobble). We skip this check and directly look up the entity in the Module layer. Themodule_name_to_nodemap is only needed/used when registering references from unqualified imports and aliases, since we are guaranteed to have information only about the original name of the module in those cases.assert_no_warnings!macro was defined ambiguously. Ifsrcwas anyexpr, then the compiler would not know whether a second module tuple was part of the repeated list of module tuples orsrc. Changingsrcto be aliteralfixes this.Example
Before
Here, at
wobble.wimble(), a lookup in themodule_name_to_nodemap would return the top-levelwobblemodule that is aliased towob. Hence, no reference towibble/wobbleis registered and it is incorrectly marked unused.A less visible problem is that
wibble/wobbleis added to theModulelayer as entity with namewobble. Then, on the next line,wobbleis once again added to theModulelayer with the same name, shadowing the first import.After
At
wobble.wimble(), we look up thewobbleentity in theModulelayer instead. This gives us the correctwibble/wobblemodule.The
Modulelayer contains two entities:wobbleandwob. TheShadowedlayer contains one entity:wobble.