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

Fix resolution of local items that shadow imported ones #5418

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Dec 21, 2023

Description

Closes #5383.

#5383 blocks PR #5306.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@ironcev ironcev marked this pull request as ready for review December 21, 2023 20:12
@ironcev ironcev requested review from xunilrj, IGI-111 and a team December 21, 2023 20:13
Copy link
Contributor

@jjcnn jjcnn left a comment

Choose a reason for hiding this comment

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

This definitely fixes a bug, but there's still an issue when the imported declaration is used before the local declaration:

use lib::Enum;

pub const Y: Enum = Enum::A;

enum Enum {
    A: (),
    B: (),
}

The type Enum in the declaration of Y resolves to the locally defined type rather than the imported one.

It's probably intentional that type declarations are in scope in the entire file, but I was surprised to see it resolve this way, so maybe it's worth it to issue a warning when a used type name has multiple resolutions?

In any case the scope of type declarations is unclear from the documentation.

@ironcev
Copy link
Member Author

ironcev commented Dec 21, 2023

This definitely fixes a bug, but there's still an issue when the imported declaration is used before the local declaration

This is actually expected behavior. Items follow item-style shadowing and not sequential shadowing. They are visible in the whole module no matter where they are declared. E.g.:

fn first() {
   second();
}

fn second() {
   let _ = Enum::A;
}

enum Enum {
    A: (),
}

Which means Enum is visible everywhere and thus shadows the imported one everywhere.

In the above example we will also get an error for the name clash, but the resolution should still be to the local one, because the shadowing is not sequential but item-style.

To item-style shadowing and sequential shadowing, I like the explanation/definitions given in the Rust RFC 116 (No module shadowing).

@ironcev ironcev self-assigned this Dec 22, 2023
@ironcev ironcev added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Dec 22, 2023
@ironcev ironcev requested a review from jjcnn December 22, 2023 00:06
@jjcnn
Copy link
Contributor

jjcnn commented Dec 22, 2023

To item-style shadowing and sequential shadowing, I like the explanation/definitions given in the Rust RFC 116 (No module shadowing).

"Due to a certain lack of official, clearly defined semantics and terminology, ..."

Suffice it to say I don't. :-)

@xunilrj xunilrj merged commit a676a3d into master Dec 22, 2023
33 checks passed
@xunilrj xunilrj deleted the ironcev/5383-glob-imports-and-item-resolution branch December 22, 2023 18:25
@ironcev
Copy link
Member Author

ironcev commented Dec 22, 2023

Suffice it to say I don't. :-)

Well, as long as we don't have anything better... 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong resolution of items when using glob imports
3 participants