Skip to content

Warn if a top-level definition shadows an imported one #4545

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

arielherself
Copy link
Contributor

@arielherself arielherself commented May 1, 2025

This PR hopefully resolves #4519. A new warning will be shown like this:

warning: Definition of "foo" shadows an imported value
  ┌─ /src/warning/wrn.gleam:3:4
  │
3 │ fn foo(x) {
  │    ^^^

The imported value could not be used in functions anyway.

However, I found a similar issue for constructors and type aliases. Especially if we import a constructor and define a type (not constructor) with the same name, the imported item will be used:

import module1.{Bar}  // imported Bar is a constructor

type Bar {  // local Bar is a type
}

pub fn main() {
	Bar  // this resolves to module1.Bar
}

Is this an intended behavior? If not I think we should add an error for this.

@arielherself arielherself changed the title fix(analyse): warn if a function definition shadows an imported one Warn if a function definition shadows an imported one May 1, 2025
@GearsDatapacks
Copy link
Member

The latter is intended behaviour. Types and values live in different namespaces

@arielherself arielherself force-pushed the lsp-warn-shadowing branch from e3a3e62 to ace5809 Compare May 1, 2025 16:06
@arielherself
Copy link
Contributor Author

I see. I have updated the warning message and changelog.

@arielherself arielherself force-pushed the lsp-warn-shadowing branch 3 times, most recently from 03f077c to c92f383 Compare May 1, 2025 16:30
@arielherself arielherself changed the title Warn if a function definition shadows an imported one Warn if a top-level definition shadows an imported one May 2, 2025
@arielherself
Copy link
Contributor Author

I also updated the test result of this existing test:

#[test]
fn used_shadowed_imported_value() {
    assert_warning!(
        (
            "thepackage",
            "wibble",
            "
pub const wibble = 1
"
        ),
        "
import wibble.{wibble}

pub const wibble = wibble
"
    );
}

Since redefining values is not actually legal, there should be a warning on shadowing of wibble here even if we used the imported wibble explicitly. Correct me if I'm wrong!

@lpil
Copy link
Member

lpil commented May 2, 2025

That one should not warn 🙏 Until #614 is resolved it has valid uses.

@arielherself arielherself force-pushed the lsp-warn-shadowing branch from e4233e4 to e63b49a Compare May 3, 2025 06:00
@arielherself
Copy link
Contributor Author

Thanks for your clarification! I've added logic to not complain about direct re-exports like const c = c.

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!

I've actually realised I was wrong with my previous guidance, the programmer should in that case use the qualified syntax. Sorry, I was quite tired when I wrote it! Could you remove that exception please 🙏

location: f.location,
name,
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Please move the code for this check into the existing definition analysis rather than iterating over the functions and constants again 🙏

@arielherself arielherself force-pushed the lsp-warn-shadowing branch from 8d8f6c7 to d32bb06 Compare May 3, 2025 16:54
@arielherself
Copy link
Contributor Author

Of course! 💜

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.

Warn when unqualified import is shadowed by top-level definition.
3 participants