SpaceAroundOperator: false positive on unary minus (Elixir 1.6.0)

[randycoulman]

Precheck

• For proposing a new feature, be sure to name your issue "Proposal: ..." (n/a)
• For bugs, please do a quick search and make sure the bug has not yet been reported here (done)

Environment

• Credo version (mix credo -v): 0.8.10
• Erlang/Elixir version (elixir -v): 1.6.0
• Operating system: macOS Sierra (10.12.6)

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]

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]

@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]

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

[AntonShevel]

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]

@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]

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

[Cleop]

@rrrene I'm experiencing this error too

Environment

• credo 0.9.0-rc3
• Elixir 1.6.0

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 #496 (comment) and now it runs successfully locally doing mix credo but still breaks when done as part of my pre-commit.

Pass:

Fail with no further explanation:

Any advice?

[rrrene]

@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]

@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]

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

• https://git-scm.com/docs/githooks

  pre-commit
  [...]
  Exiting with a non-zero status from this script causes the git commit command to abort before creating a commit.

• https://en.wikipedia.org/wiki/Exit_status#POSIX

• https://github.com/rrrene/credo#exit-status

  Credo fails with an exit status != 0 if it shows any issues. This enables shell based pipeline workflows (e.g. on CI systems) which test Credo compliance.

Hope this helps to clear things up. 👍

[Cleop]

Thanks @rrrene I understand it now 👍

[lasseebert]

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]

@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]

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]

The issue I described above is gone now.

Thanks! :)

[dabaer]

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