Skip to content

Warn when top-level definition shadows imported value #4672

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

Merged
merged 8 commits into from
Jun 16, 2025

Conversation

aayush-tripathi
Copy link
Contributor

@aayush-tripathi aayush-tripathi commented Jun 7, 2025

Added Warning::TopLevelDefinitionShadowsImport with tests and snapshots

Closes #4666

This PR adds a compiler warning when a top-level function/constant shadows an imported value:

  • New Warning::TopLevelDefinitionShadowsImport
  • Emitted in analyse.rs
  • Updates tests & snapshots
  • Updated to_diagnostic in warning.rs

@aayush-tripathi aayush-tripathi force-pushed the shadow‐import‐warning branch from 31ecdd8 to 183f2e3 Compare June 8, 2025 03:53
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Nice, thank you! I've left some notes inline 🙏

Could you update the changelog too please

location: c.location,
name: name.clone(),
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this and the other instance of this block below into a helper method on self please 🙏

name
} => Diagnostic {
title: format!("`{}` shadows an imported name", name),
text: format!("Definition of `{}` shadows an imported value.\n\
Copy link
Member

Choose a reason for hiding this comment

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

Use the wrap function please 🙏

"thepackage",
"module",
r#"
pub const foo = 1
Copy link
Member

Choose a reason for hiding this comment

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

No foobar please, it's a rather grim WW2 reference.

@aayush-tripathi aayush-tripathi force-pushed the shadow‐import‐warning branch from 183f2e3 to a3dc304 Compare June 10, 2025 19:06
@aayush-tripathi
Copy link
Contributor Author

Hi. I have updated the PR.

@aayush-tripathi aayush-tripathi requested a review from lpil June 13, 2025 17:49
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

@lpil lpil force-pushed the shadow‐import‐warning branch from a3dc304 to fbbd1e9 Compare June 16, 2025 14:28
@lpil lpil enabled auto-merge (rebase) June 16, 2025 14:29
@lpil lpil merged commit 82b12de into gleam-lang:main Jun 16, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate shadowing imported value
2 participants