-
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
Name clash in star imports should result in error when the name is used #5963
Conversation
…sh-on-star-imports
…s been imported more than once
Benchmark for c1e6d19Click to view benchmark
|
Benchmark for f0511ddClick to view benchmark
|
Benchmark for 156a15bClick to view benchmark
|
Benchmark for 3d7f425Click to view benchmark
|
Benchmark for a345357Click to view benchmark
|
test/src/e2e_vm_tests/test_programs/should_pass/language/import_star_name_clash/Forc.toml
Show resolved
Hide resolved
Left a couple suggestions, but overall really nice improvements here @jjcnn, great stuff. |
Is it? I think of it as a bugfix. In any case the change will not change the behavior of programs that successfully compile. Compilation will only fail some programs that used to compile without error. |
…s/sway into jjcnn/name-clash-on-star-imports
Review comments addressed. There is one outstanding bug: When the name is bound to an enum variant, we report I'll fix this issue later today, and update the failing test accordingly. |
Benchmark for a188b6bClick to view benchmark
|
Final issues fixed. |
Benchmark for e16c46dClick to view benchmark
|
Benchmark for ffeda73Click to view benchmark
|
Description
Fixes #5940.
This PR deals with the situation where the same name is imported by multiple star imports, e.g.,
So far we have resolved this name clash by letting the latter import shadow the former, which is incorrect.
The correct behavior is to allow the import without error, but to report an error if the unqualified name (i.e.,
X
) is used (qualified namesa::X
andb::X
are legal)`. This PR fixes this problem.Note that if
X
is imported multiple times, but each time is bound to the same entity (e.g., because it is imported both throughcore:;prelude
andcore::codec
), then this should not cause an error.Note also that there is a problem with the implicit imports from the core and std preludes, which in some cases causes the implementation to think that a name is bound to multiple different entities, even though the entities are actually the same. As a workaround anytime a name is imported from either
std
orcore
when it is already imported fromstd
orcore
, the new binding replaces the old one instead of being added as a new binding.Unfortunately this workaround means that it is not currently possible to redefine and use a name that exists in
std
orcore
, e.g., to create a specialized version ofassert_eq
as is done in one of our tests. To use such a redefinition one must use a qualified name for the entity, which seems like an acceptable workaround. (Note that this only applies if the redefined entity is imported using a star import - if imported using an item import (use a::X;
), then there is no issue).Once #3487 is implemented this workaround should no longer be necessary.
Checklist
Breaking*
orNew Feature
labels where relevant.