Fix overflowing_literals lint with repeated negation

[theemathas]

Fixes #158295

This PR consists of: two commits with non-functional cleanups, a commit that adds a test, and a commit that actually changes the behavior. The following description concerns the changed behavior. See the test changed in the fourth commit for code demonstrating the change in behavior.

---

The overflowing_literal lint is, I believe, supposed to consider the literal and not the surrounding context when deciding whether to lint. This is in contrast to the arithmetic_overflow lint, which only considers code that executes at run time, which excludes the negation inside a literal.

According to the reference, a negation in front of an integer does not result in an overflow. I think of this as: a negation, and only one negation, in front of an integer, is "part of" the integer. Therefore, the overflowing_literal lint should consider that when linting. It should not consider the second or third negation.

Therefore, this PR changes overflowing_literals so that it only lints on 128_i8 when there is no preceding negation at all. This is in contrast to the previous behavior, which lints on 128_i8 preceded by an even number of negations.

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 22 candidates

[adwinwhite]

I personally lean towards catching more overflows at compile-time if possible, and we can clarify this in reference instead?

Of course we should hear more opinions. Is starting FCP the right process for that?

[theemathas]

If the arithmetic_overflow and overflowing_literal lints are both enabled, then, in my testing, this PR doesn't change what code gets linted. It just changes which lints fire.

I've just noticed that the arithmetic_overflow lint appears to not be firing at all in the ui tests. I'm very confused on why this is happening.

In any case, I'm lang-nominating for opinions on when exactly these two lints should fire.

[theemathas]

I figured out that the arithmetic_overflow lint wasn't firing in the tests because compiletest was doing the equivalent of cargo check. So, I edited the test to use --emit link (the equivalent of cargo build), so the lint would fire.

Hopefully it should be obvious from the tests now, that the same amount of overflows are still caught by lints.

[theemathas]

Another thing I forgot to mention previously: The run time behavior of an allow(arithmetic_overflow) is a panic in debug mode. In contrast, the run time behavior of an allow(overflowing_literals) is seemingly supposed to be silently overflowing, even in debug mode. This PR makes the lint fire exactly when such silent overflows would otherwise happen.

[scottmcm]

I agree with @theemathas's analysis of this. In particular I think the

In contrast, the run time behavior of an allow(overflowing_literals) is seemingly supposed to be silently overflowing, even in debug mode.

note is a good one, and a particularly good reason for it to be deny-by-default and not warn-by-default. (Though it looks like both are currently deny-by-default.)

Not saying (indirectly) that --1 is "a literal" is definitely an improvement, in my mind.

---

So let's say "yes, for the lint we consider -1 a single literal (since otherwise there's no way to write -128_i8) but further negation is overflow, not part of the literal".

@rfcbot fcp merge lang

(Admittedly the whole thing is a bit weird, since of course -1.exp() isn't a negated literal, but that's not this lint's problem.)

---

One day I'll remember how to use the bot, sorry

[rust-rfcbot]

@scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Makes sense to me.

@rfcbot reviewed

[tmandry]

@rfcbot reviewed

(I believe this is a quality of implementation issue and doesn't need a lang FCP.)

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔