search

rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.93k stars 415 forks source link

SpaceAroundOperator: false positive on unary minus (Elixir 1.6.0) #495

Closed randycoulman closed 6 years ago

randycoulman commented 6 years ago

Precheck

Environment

What were you trying to do?

Attempting to upgrade an application from Elixir 1.5.3/credo 0.8.6 -> Elixir 1.6.0. In Elixir 1.6.0, credo 0.8.6 failed in Code.toTokens. After reviewing the CHANGELOG, I realized that I'd need to also upgrade credo to 0.8.10, so I did that as well.

Expected outcome

No credo warnings.

Actual outcome

Running credo on the app resulted in a new SpaceAroundOperator warning that wasn't present previously. The line in question is:

defp parabolic(kwh), do: -abs(kwh) * kwh / (50 * 50 * 2) + 0.5

The error message points at the * in the 50 * 50 part of the expression, but by changing the code to this:

defp parabolic(kwh), do: -1 * abs(kwh) * kwh / (50 * 50 * 2) + 0.5

the error goes away.

It looks like the - is not being properly interpreted as a unary minus in this case. There is code to detect in when the following characters are numeric (which is why the -1 works in the new code).

I also tested with Elixir 1.5.3/credo 0.8.10 and did not see the same warning, so this appears to be a new issue with Elixir 1.6.0.

halfdan commented 6 years ago

I'm also seeing warnings with Elixir 1.6.0 when compiling verk (which depends on latest credo): 

==> credo
Compiling 161 files (.ex)
warning: this clause cannot match because a previous clause at line 165 always matches
  lib/credo/cli/output/summary.ex:165

warning: module attribute @lint was set but never used
  lib/credo/check/find_lint_attributes.ex:23
randycoulman commented 6 years ago

@halfdan I see those too, but I think they're unrelated to this issue. Might be worth opening a separate issue for that so that this thread doesn't get confusing for people.

halfdan commented 6 years ago

@randycoulman Ah, fair enough 👍 - only glimpsed over your issue and didn't realise it was describing a different issue.

AntonShevel commented 6 years ago

Experienced the same issue with Elixir 1.6.1 and Credo 0.8.10:

There are spaces around operators most of the time, but not here.

datetime_with_offset(-50 * 60)

Here are workaround which are possible

datetime_with_offset(-1 * 50 * 60)
datetime_with_offset(-50*60)
rrrene commented 6 years ago

@randycoulman @halfdan @AntonShevel Could you test with 0.9.0-rc3? There's still issues to iron out, but I think we're close to release.

randycoulman commented 6 years ago

@rrrene Seems to be fixed for the example I originally reported. Thanks!

Cleop commented 6 years ago

@rrrene I'm experiencing this error too

Environment

What were you trying to do?

Set up credo into my pre-commit on the project: https://github.com/dwyl/hits-elixir

Expected outcome

Credo runs and reports any needed changes or success message.

Actual Outcome

Credo was erroring: Error while running Elixir.Credo.Check.Consistency.SpaceAroundOperators and so I looked at the issues on this repo and tried setting the Credo dep to:

{:credo, github: "rrene/credo"}

Based on your comment in https://github.com/rrrene/credo/issues/496#issuecomment-362642775 and now it runs successfully locally doing mix credo but still breaks when done as part of my pre-commit.

Pass: image

Fail with no further explanation: image

Any advice?

rrrene commented 6 years ago

@Cleop There is no evidence of Credo breaking in the screens. Maybe there's a misunderstanding here?

Since Credo is reporting issues on the code, it will exit with an exit status other than zero. This causes "Pre-commit failed on mix credo."

You can mute the exit status via mix credo --mute-exit-status, but I am unsure if that is useful in your scenario.

Cleop commented 6 years ago

@rrrene thanks for getting back to me. So credo was failing with the SpaceAroundOperators error but then I applied your recommendation of changing my dep to: {:credo, github: "rrene/credo"}.

Now it passes when run simply as mix credo but fails when run as part of the pre-commit. Given that all my other pre-commits run fine and that before credo was also not working when run on its own, I presumed that the issue was still with credo. I've found it difficult to debug because when I run the pre-commit (the second screen shot) I'm getting nothing more than pre-commit failed on mix credo to help me work out what is going wrong. So perhaps you can tell me whether you think this message is an issue with the pre-commit hook or credo?

rrrene commented 6 years ago

@Cleop I am in a bit of a rush, so I will be impolite and just dump this list of links:

Hope this helps to clear things up. :+1:

Cleop commented 6 years ago

Thanks @rrrene I understand it now 👍

lasseebert commented 6 years ago

I'm having a similar issue after updating to 0.9.0. The problem was not there on 0.8.10.

I am able to reproduce with this small example code:

def my_fun(x) when x < -4 do
  x
end

def my_fun(x) when -4 < x do
  x
end

Credo will warn about the second function but not the first:

There are spaces around operators most of the time, but not here.
$ mix credo info
System:
  Credo: 0.9.0
  Elixir: 1.6.3
  Erlang: 20
rrrene commented 6 years ago

@randycoulman @halfdan @AntonShevel @Cleop Again, thanks for reporting this 😀 It should now be fixed on master.

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

rrrene commented 6 years ago

It should be be fixed in the latest version (v0.9.1) of Credo.

If it is not, please feel free to re-open this issue!

lasseebert commented 6 years ago

The issue I described above is gone now.

Thanks! :)

dabaer commented 2 years ago

Hello, I know this is quite an old issue, but this is flagging in newest elixir and credo. Interestingly I have unary 1 in other parts of that file it doesn't seem to be bothered by.

System:
  Credo: 1.7.0-dev-ref.master.b5b51303
  Elixir: 1.13.4
  Erlang: 24.3.4

image