- Notifications
You must be signed in to change notification settings - Fork 3.5k
Give warning on unused require#14779
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lib/elixir/src/elixir_import.erl Outdated
| Traceandalsoelixir_env:trace({import, Meta, Ref, Opts}, E), | ||
| EI=E#{functions :=Functions, macros :=Macros}, | ||
| {ok, Added, elixir_aliases:require(Meta, Ref, Opts, EI, Trace)}; | ||
| {ok, Added, elixir_aliases:require([{from_import, true} | Meta], Ref, Opts, EI, Trace)}; |
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.
Maybe instead of from_import we could do [{warn, false} | Opts] instead?
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.
Done :)
| end)=="" | ||
| after | ||
| purge(KernelTest.WarnFalsePreservesFunctionality) | ||
| end |
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.
We already test these different conditions in the unit tests in lexical tracker. In here, we need only a single test that verifies the format of the warning itself.
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.
Done :)
thiamsantos commented Sep 23, 2025
The results of the compilation of some popular libraries using this branch are promising. plugecto_sqlphoenix |
f089571 into elixir-lang:mainUh oh!
There was an error while loading. Please reload this page.
josevalim commented Sep 23, 2025
💚 💙 💜 💛 ❤️ |
lukaszsamson commented Sep 23, 2025
@josevalim@thiamsantos I suspect this may not work correctly with |
josevalim commented Sep 23, 2025
In this case the require is not used, so it should warn (and the user should use an alias instead)! A test would be nice indeed! |
lukaszsamson commented Sep 23, 2025
Yes, the warning should say that, e.g. |
thiamsantos commented Sep 23, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I am not sure if I followed the -- |
sabiwara commented Sep 23, 2025
Not discouraged if you need both require + alias, but in the case no macro is used, it should be a regular alias 🙂 |
thiamsantos commented Sep 23, 2025
@sabiwara yeah, makes sense. Now I was able to connect the dots 😂 thank you |
michallepicki commented Sep 26, 2025
Shouldn't this warning be suppressed for |
josevalim commented Sep 26, 2025
Yes, it should. I will take a look at it. |
josevalim commented Sep 27, 2025
Fixed in main, thank you! |
This a draft PR for the for the proposal: https://groups.google.com/g/elixir-lang-core/c/d9ZjViQaOY8/m/tZN0jn6LAAAJ. The implementation is based on the unused import warning.
I plan to compile a project like plug to check the impact that such warning would have in codebases.