Please provide
And a thorough description of the problem / question:
S1854 complains about this statement:
CalculateSomething(++i);
If i is not used afterwards. I would expect it to complain about i++, but not ++i, and in fact this caused a bug in my code, since I just trusted sonarlint and removed the increment…
Hey there.
Can you provide a more complete reproducer? A full snippet of code that reproduces the issue will be helpful (it’s important the context in which CalculateSomething(++i); takes place)
Hello @Thomas_Haukland,
There has been a discussion again regarding this (please see this post for information).
From our side, we don’t consider this an FP because ++i is still i = i + 1, thus you still have an unused assignment.
Of course, that is your prerogative, just as long as you realize you are breaking this pretty useful pattern, and causing a lot of pain to users of it, if they automatically follow your suggestion of removing unused assignment.
The result is changed behaviour and in my cause a very real bug.
In my opinion, a refactor suggestion should never change the behaviour of the code.
Hello,
I’m sorry I maybe was not detailed enough in my answer.
The proposal for the particular example above is to replace only the last ++number with number+1 to avoid the unused assignment (since ++number is actually number = number + 1 in the background).
It’s not clear to me how this can cause a bug - could you provide me with a snippet where this is causing an issue so I can investigate further?
Thanks!
EDIT: I’ve opened an issue in our backlog to improve the message as it’s a bit confusing for this context.
This is a pretty common pattern e.g. when adding items to a grid which requires a placement index, and you want to conditionally skip some items, instead of hardcoding the number you just use “++i”, and you are free to move things around/skip items without worrying about updating numbers.
When you are warned about unused assignment(at least in rider), the default suggestion is then to change ++i to just i, and that changes behaviour and will most likely cause a bug, since i and ++i is most definitely not the same…
Hello @Thomas_Haukland and thanks for the prompt reply.
the default suggestion is then to change ++i to just i, and that changes behaviour and will most likely cause a bug, since i and ++i is most definitely not the same…
This particular rule does not have a quickfix (there’s no suggestion that should be proposed by our analyzer for this rule and applied in case you click it).
I suspect this might be coming from the code inspection of Rider. A quick way to check is to disable the SonarLint plugin and see if the suggestion is still there.
