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

alias lifting: auto-detect conflicts rather than relying on "excludes" configuration #193

Closed
jsw800 opened this issue Sep 4, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@jsw800
Copy link

jsw800 commented Sep 4, 2024

Versions

  • Elixir: 1.17.1
  • Styler: 1.0.0

Example Input

Before running mix format:

defmodule SomeModule do
  def do_something(token) do
    # Guardian.Token.Jwt is a library module in the guardian library
    # MyApp.Guardian is a module defined inside my application
    Guardian.Token.Jwt.decode_token(MyApp.Guardian, token)
  end

  def do_something_else do
    Application.get_env(:my_app, MyApp.Guardian)
  end
end

Stacktrace / Current Behaviour

After running mix format:

defmodule SomeModule do
  alias MyApp.Guardian

  def do_something(token) do
    # elixir now can't find the Guardian.Token.Jwt module because it is
    # looking for MyApp.Guardian.Token.Jwt which is not a real module
    Guardian.Token.Jwt.decode_token(Guardian, token)
  end

  def do_something_else do
    Application.get_env(:my_app, Guardian)
  end
end
@jsw800 jsw800 changed the title Alias lifting produces broken code when aliases conflict with other existing usages bug: alias lifting produces broken code when aliases conflict with other existing usages Sep 4, 2024
@novaugust
Copy link
Contributor

thanks for the report! definitely something styler's supposed to notice, sorry that it's got a bug there

@novaugust
Copy link
Contributor

hey @jsw800 , i wasn't able to reproduce the issue with the snippet you gave me:

https://github.com/adobe/elixir-styler/compare/me/fix-193?expand=1

tests still pass. is there more context you can provide?

@jsw800
Copy link
Author

jsw800 commented Sep 4, 2024

Ah sorry about that - I had kinda changed it from my original code so as to not share my actual source code. It looks like the difference between my original code and the example I shared is that MyApp.Guardian was actually MyApp.Something.Guardian. When the code looks like this, the test fails as expected:

defmodule SomeModule do
  @moduledoc false

  def do_something(token) do
    # Guardian.Token.Jwt is a library module in the guardian library
    # MyApp.Something.Guardian is a module defined inside my application
    Guardian.Token.Jwt.decode_token(MyApp.Something.Guardian, token)
  end

  def do_something_else do
    Application.get_env(:my_app, MyApp.Something.Guardian)
  end
end

@novaugust
Copy link
Contributor

ty, got it now :)

@novaugust novaugust added the bug Something isn't working label Nov 7, 2024
@novaugust
Copy link
Contributor

novaugust commented Jan 14, 2025

hey @jsw800 , was going through old issues and realized that this exact issue is why styler has the alias_lifting_exclude configuration. if you add Guardian to that list this issue will be resolved

that said, it'd be neat if styler could just detect this itself rather than relying on configuration

docs: https://hexdocs.pm/styler/module_directives.html#collisions

@novaugust novaugust added enhancement New feature or request and removed bug Something isn't working labels Jan 14, 2025
@novaugust novaugust changed the title bug: alias lifting produces broken code when aliases conflict with other existing usages alias lifting: auto-detect conflicts rather than relying on "excludes" configuration Jan 14, 2025
@jsw800
Copy link
Author

jsw800 commented Jan 24, 2025

Thanks @novaugust! We had started using the excludes configuration for some scenarios, or for more one offs, just manually renaming the colliding aliases with the as option for aliases. This will be super nice to have built-in to styler!

@novaugust
Copy link
Contributor

cheers dude, hopefully what's on main handles whatever you throw at it now :) i'll cut a release early next week

novaugust added a commit that referenced this issue Feb 13, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…loses #193
novaugust added a commit that referenced this issue Feb 13, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…loses #193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants