Fix overflowing_literals lint with repeated negation#158302
Conversation
This better matches how the argument is actually used.
See `rustc_hir::intravisit::{walk_expr,walk_pat_expr}`.
|
cc @rust-lang/clippy |
|
r? @adwinwhite rustbot has assigned @adwinwhite. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
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? |
|
If the I've just noticed that the In any case, I'm lang-nominating for opinions on when exactly these two lints should fire. |
|
I figured out that the Hopefully it should be obvious from the tests now, that the same amount of overflows are still caught by lints. |
|
Another thing I forgot to mention previously: The run time behavior of an |
Fixes rust-lang#158295 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](https://doc.rust-lang.org/1.96.0/reference/expressions/operator-expr.html#r-expr.operator.int-overflow.unary-neg), 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 commit 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.
|
I agree with @theemathas's analysis of this. In particular I think the
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 So let's say "yes, for the lint we consider @rfcbot fcp merge lang (Admittedly the whole thing is a bit weird, since of course One day I'll remember how to use the bot, sorry |
|
@scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
|
Makes sense to me. @rfcbot reviewed |
|
@rfcbot reviewed (I believe this is a quality of implementation issue and doesn't need a lang FCP.) |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
|
@bors r+ rollup |
…uwer Rollup of 8 pull requests Successful merges: - #159197 (Rename ModDefId to ModId and use more) - #159222 (semicolon_in_expressions_from_macros: Lint on non-local macros too) - #159296 (Implement `bool::toggle`) - #158302 (Fix `overflowing_literals` lint with repeated negation) - #158484 (Add documentation for the `cold` and `track_caller` attributes) - #159239 (Add codegen test for remainder match) - #159330 (fix some edition annotations in UI tests) - #159331 (Fix ignore-llvm-version directive in codegen-llvm/array-equality.rs) Failed merges: - #158962 (Add `proc_macro` attribute documentation)
…uwer Rollup of 8 pull requests Successful merges: - #159197 (Rename ModDefId to ModId and use more) - #159222 (semicolon_in_expressions_from_macros: Lint on non-local macros too) - #159296 (Implement `bool::toggle`) - #158302 (Fix `overflowing_literals` lint with repeated negation) - #158484 (Add documentation for the `cold` and `track_caller` attributes) - #159239 (Add codegen test for remainder match) - #159330 (fix some edition annotations in UI tests) - #159331 (Fix ignore-llvm-version directive in codegen-llvm/array-equality.rs) Failed merges: - #158962 (Add `proc_macro` attribute documentation)
…uwer Rollup of 8 pull requests Successful merges: - #159197 (Rename ModDefId to ModId and use more) - #159222 (semicolon_in_expressions_from_macros: Lint on non-local macros too) - #159296 (Implement `bool::toggle`) - #158302 (Fix `overflowing_literals` lint with repeated negation) - #158484 (Add documentation for the `cold` and `track_caller` attributes) - #159239 (Add codegen test for remainder match) - #159330 (fix some edition annotations in UI tests) - #159331 (Fix ignore-llvm-version directive in codegen-llvm/array-equality.rs) Failed merges: - #158962 (Add `proc_macro` attribute documentation)
Rollup merge of #158302 - theemathas:neg-lit-lint, r=adwinwhite Fix `overflowing_literals` lint with repeated negation 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](https://doc.rust-lang.org/1.96.0/reference/expressions/operator-expr.html#r-expr.operator.int-overflow.unary-neg), 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.
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_literallint is, I believe, supposed to consider the literal and not the surrounding context when deciding whether to lint. This is in contrast to thearithmetic_overflowlint, 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_literallint should consider that when linting. It should not consider the second or third negation.Therefore, this PR changes
overflowing_literalsso that it only lints on128_i8when there is no preceding negation at all. This is in contrast to the previous behavior, which lints on128_i8preceded by an even number of negations.