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 redefined function source location #5720

Merged
merged 1 commit into from Jan 31, 2017
Merged

Fix redefined function source location #5720

merged 1 commit into from Jan 31, 2017

Conversation

tsubery
Copy link
Contributor

@tsubery tsubery commented Jan 30, 2017

When functions are redefined in a different file, the translation uses
the first definition source location for all subsequent definitions.
This manifests itself as warnings that are ascribed to the wrong file.

Fixes #5719

test "unused variable in redefined function in different file" do
output = capture_err(fn ->
Code.eval_string """
defmodule Sample do
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the supervisor example as a test so we don't need to define a separate fixture file. :)

Copy link
Contributor Author

@tsubery tsubery Jan 30, 2017

Choose a reason for hiding this comment

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

Not sure I understand what you mean by "use the supervisor example". I can't find an existing fixture that has a redefinition of a function from a different source. I can avoid the eval_string by change the fixture file to use supervisor, but this would still require having a fixture file.

Copy link
Member

@josevalim josevalim Jan 30, 2017

Choose a reason for hiding this comment

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

Wouldn't this be enough to have a warning?

Code.eval_string """
defmodule Sample do
  use Supervisor

  def init(params) do
  end
end
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code does expose the bug because the warning would refer to no_file:3 instead of supervisor.ex:5.
I like the assertion assert output =~ "redefine_sample.ex:3" better because it seems more expressive and less brittle.

Copy link
Member

Choose a reason for hiding this comment

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

So you can pass :file option to eval_string if that's the case. No need for a fixture file. :)

Copy link
Contributor Author

@tsubery tsubery Jan 30, 2017

Choose a reason for hiding this comment

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

Oh great. That is definitely better..
Pulling in Supervisor might make following the intention of the test setup harder
What about this version?

check_valid_kind(Ann, File, Name, Arity, Kind, StoredKind),
(Check and StoredCheck) andalso
check_valid_clause(Ann, File, Name, Arity, Kind, Data, StoredAnn, StoredFile),
check_valid_defaults(Ann, File, Name, Arity, Kind, Defaults, StoredDefaults, LastDefaults, LastHasBody),
{StoredAnn, StoredLocation, {max(Defaults, StoredDefaults), HasBody, Defaults}};
{StoredAnn, Location, {max(Defaults, StoredDefaults), HasBody, Defaults}};
Copy link
Member

Choose a reason for hiding this comment

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

Perfect. :)

When functions are redefined in a different file, the translation uses
the first definition source location for all subsequent definitions.
This manifests itself as warnings that are ascribed to the wrong file.

Fixes #5719
@josevalim josevalim merged commit 1341bdf into elixir-lang:master Jan 31, 2017
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

josevalim pushed a commit that referenced this pull request Jan 31, 2017
When functions are redefined in a different file, the translation uses
the first definition source location for all subsequent definitions.
This manifests itself as warnings that are ascribed to the wrong file.

Fixes #5719

Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants