Closed NJichev closed 4 months ago
So diffing elixir 1.13 and 1.12 doesn't reveal at first glance what changed between the two versions. Checking the test examples I found that all the errors are coming from function calls coming from the erlang std library, i.e:
:erlang.open_port(...)
:os.cmd(...)
Yeah. I just pushed some "checks" to the testing pipeline that make sure the trigger, line and column info is actually consisistent.
Could you rebase against master
?
Okay, so the new pipeline is showing some deeper issues: https://github.com/rrrene/credo/actions/runs/8901349869/job/24445604486?pr=1127
We have to provide an accurate column
without changing the trigger
(the trigger
being the String to be highlighted by tooling like an editor).
Hmm I don't think I've touched any of the trigger code, I'll take a look tomorrow morning
I probably did not phrase that in a good way, apologies.
The trigger
is the string triggering the issue, e.g. ":os.cmd". The column
has to be the start of that string in the source code.
This PR wants to fix instances where the column
is wrong when there are two instances of the same call on the same line, which is super good 👍
But we cannot pick the column
arbitrarily, because it has to be the actual start of the trigger
🙂
What I meant with my last comment is that it is not an option to change the trigger from ":os.cmd" to "cmd", because that would potentially break backwards compatibility and the intent of the check in question.
Maybe in this case I could get the module name and calculate it's string length and remove that from the column length to fix it, but it still doesn't cover the case where the AST is off by 1 on older versions of Elixir. I could either compile a different version of the code for older versions to make it consistent if you think that's okay? Another approach would be to just make this elixir version check in the tests and leave it like that?
Let's ignore the off-by-one thing for Elixir 1.11 and 1.12 for now (I can deal with that after merging).
Our main focus should be to get the correct column for the existing trigger, as that is the primary improvement this PR brings 👍
Hey I fixed the Erlang stdlib function columns by just calculating the string length of the module. Also fixed some of the triggers that were producing warnings to match the real trigger.
Do you have any suggestions on how to fix the other 3 triggers that mismatch the actual code and column that are producing the warnings in the tests:
Logger.info(..., key: :foo)
while the trigger is just "key" in this example - here I'm not exactly sure what's the most appropriate.So the last 2 can maybe be left as is. The empty enum check I've left it to follow the operator semantics above, but am open to changing.
So the last 2 can maybe be left as is.
What do you mean? We can not include a wrong column
that is not the start of the trigger
in the source code.
In your first example, the trigger is actually Enum.count
and not some kind of operator (I would also say that is the case from a educational point of view: the problem is not that you are using ==
, the problem is that you are using Enum.count
when you don't need to).
In the second example, of course the problem is the metadata entry in the keyword list (named :key
in the test, not a good name I have to admit) and not the call to a Logger function.
Of course, in the special case with the grouped aliases for the Forbidden Module check, the trigger
is actually wrong, because the string does not appear in the source code. So we have to change the (incorrect) trigger
in this case (or you could simply revert your changes to this check and we deal with it in the future).
Hey :wave: I adjusted the expensive enum check column to get it from the correct operator side instead of using the line;col of the operator. Also fixed the trigger for the forbidden module to be the short name when in a group alias since the trigger is wrong otherwise and won't match the source code(did a small refactor there).
Decided to revert the column info for the missing metadata key in the Logger config since it's not so trivial to calculate the right column info for the starting position of the key in question. Maybe I could do it in a separate PR by using Macro.to_string instead of doing it by hand on the AST.
Reverted the changes on the forbidden module check and just added a trigger as a variable + reduce one pass with enum.
@NJichev Thx for this! It highlighted several pain points in Credo's codebase 👍
FYI: I tried to simplify your PR further when merging on the command line: ba4f622c790
Thanks for guiding me through ❤️
Hey :wave:, I was working on next-ls code actions from credo when we noticed that the column information is missing. This branch is a follow up on #1126. The main use case is generating a correct code action if there are two warnings on the same line. Without the column information 2 different warnings will map to the same AST when searching for it.
I kinda picked the column arbitrarily, so ping me if we should take it from another AST node, but generally I tried to follow the following rules:
|Enum.count(...)
x |and x
instead of the right/left hand side)erlang.|function
since the AST for theerlang
bit is different and has no column information)