Fix false positive with nested shadowed alias #13564
Conversation
lib/elixir/src/elixir_lexical.erl
Outdated
| trace({alias, Meta, _Old, New, Opts}, #{lexical_tracker := Pid}) -> | ||
| ?tracker:add_alias(Pid, New, Meta, should_warn(Meta, Opts)), | ||
| trace({alias, Meta, _Old, New, Opts}, #{lexical_tracker := Pid, function := Function}) -> | ||
| ?tracker:add_alias(Pid, New, Meta, should_warn(Meta, Opts), Function), |
There was a problem hiding this comment.
Super massive nitpick:
| ?tracker:add_alias(Pid, New, Meta, should_warn(Meta, Opts), Function), | |
| ?tracker:add_alias(Pid, New, Meta, should_warn(Meta, Opts), Function /= nil), |
So we send less data to the GenServer. :)
There was a problem hiding this comment.
493727c too much, WDYT?
Since both variables were about how to warn. Might also be less confusing than the double bool.
There was a problem hiding this comment.
Too much in my opinion. I feel like I need to undo the warn logic when I have to understand its impact in the lexical tracker. While checking for warn and function? had a clearer meaning to me. WDYT?
There was a problem hiding this comment.
I'm fine either way.
I liked the idea of having the emitting part decide what to warn, with atom names to explain, and the receiving part just following instructions.
The downside with the non-bool atom is that I needed to come up with names (not fully satisfied with :except_unused_shadowed), and also that we'd probably need guards to make sure we don't emit something else and end up with some random accidental behavior. Finally, consistency with other messages of the module is also a strong point to keep booleans.
Will revert to the double bool!
)" This reverts commit 8dc94db.
Closes #13563.
For now, I went with the suggestion to fix the issue:
We might be able to do something even smarter with
functionin order to track unused shadowing within a function, I'm not sure yet, will keep exploring. Otherwise seems like a good compromise if this keeps thestatesimpler.