Unify and improve const-prop lints #69185

RalfJung · 2020-02-15T10:54:06Z

Add a single helper method for all lints emitted by const-prop, and make that lint different from the CTFE const_err lint. Also consistently check overflow on arithmetic, not on the assertion, to make behavior the same for debug and release builds.

See this summary comment for details and the latest status.

In terms of lint formatting, I went for what seems to be the better style: have a general message above the code, and then a specific message at the span:

error: this arithmetic operation will overflow
  --> $DIR/const-err2.rs:21:18
   |
LL |     let a_i128 = -std::i128::MIN;
   |                  ^^^^^^^^^^^^^^^ attempt to negate with overflow

We could also just have the specific message above and no text at the span if that is preferred.

I also converted some of the existing tests to use compiletest revisions, so that the same test can check a bunch of different compile flags.

Fixes #69020.
Helps with #69021: debug/release are now consistent, but the assoc-const test in that issue still fails (there is a FIXME in the PR for this). The reason seems to be that const-prop notices the assoc const in T::N << 42 and does not even bother calling const_prop on that operation.
Has no effect on #61821; the duplication there has entirely different reasons.

…erflow on BinOps and not on Assert

rust-highfive · 2020-02-15T10:54:10Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

RalfJung · 2020-02-15T10:54:17Z

r? @oli-obk

src/librustc_session/lint/builtin.rs

RalfJung · 2020-02-15T10:58:00Z

src/test/ui/consts/const-err2.rs

@@ -2,33 +2,34 @@
 // optimized compilation and unoptimized compilation and thus would
 // lead to different lints being emitted

+// revisions: default noopt opt opt_with_overflow_checks


I may have gone overboard by having 4 revisions -- specifically, default (no extra flags) is likely a duplicate of one of the others, but I am not sure which. We already have so many tests, so I am somewhat inclined to remove default again... any opinions?

I like revisions

Sure but there's such a thing as too much of any good thing. ;)

RalfJung · 2020-02-15T10:58:43Z

src/test/ui/consts/const-eval/promoted_errors.rs


 fn main() {
    println!("{}", 0u32 - 1);
+    //[opt_with_overflow_checks,noopt]~^ WARN [overflow]


This makes it look like default is the same as opt, which I find surprising.

src/test/ui/issues/issue-8460-const.rs

src/test/compile-fail/consts/const-err3.rs

src/librustc_session/lint/builtin.rs

oli-obk · 2020-02-15T13:18:51Z

src/test/ui/consts/const-err2.rs

@@ -2,33 +2,34 @@
 // optimized compilation and unoptimized compilation and thus would
 // lead to different lints being emitted

+// revisions: default noopt opt opt_with_overflow_checks


I like revisions

src/test/compile-fail/consts/const-err3.rs

src/librustc_mir/transform/const_prop.rs

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

RalfJung · 2020-02-15T20:37:03Z

Man, is there any part of the test suite that does not check these lints?^^

RalfJung · 2020-02-18T22:01:13Z

I now picked arithmetic_overflow and unconditional_panic as lint names. I also decided, because this PR already renames lints, it is better to do all renaming at once, so exceeding_bitshifts is now also covered by arithmetic_overflow. However, the lint level remains the same: deny-by-default.

So, in case you want to FCP or otherwise want a summary: what this PR does is to take some of the lints that were so far reported as const_err, and move them to a different category. const_err ought to be for hard errors during const-eval, but we were also using it for when we determined that some run-time code was likely buggy (e.g. because it has overflowing arithmetic or an operation that will always panic, such as div-by-0). This analysis is performed as a side-effect of const-prop (not cons-eval).
Those cases (and only those) are now reported as arithmetic_overflow and unconditional_panic, respectively. It also turns out that const-prop already used a dedicated lint for one particular kind of overflow, namely exceeding_bitshifts. So this PR also changes those lints to be emitted as arithmetic_overflow, because it seems odd to have one lint per operation and now that we are touching the lints anyway we might as well do something that makes sense.

In other words, no new lints get emitted by this PR, but the following changes:

Parts of const_err become unconditional_panic.
Other parts of const_err become arithmetic_overflow.
exceeding_bitshifts becomes arithmetic_overflow.

With this done, one rarely wants to allow const_err (this lint is now only fired for constants that cannot be used because their evaluation failed [including promoteds]); the only reason that is a lint and not a hard error is backwards-compatibility. The other lints however are more like our regular diagnostics. I think we should make them warn-by-default instead of deny-by-default, but for now this PR leaves the default levels unchanged (but I am happy to adjust in this PR or a follow-up).

Centril · 2020-02-18T22:39:48Z

I'm happy with the new lint names. I don't think FCP for renaming necessary. I would like to leave the lint severities as-is though. The enumerated lints seem like a case of correctness class lints that highlight actual bugs, so I think stopping compilation (as achieved with deny-by-default) is warranted.

RalfJung · 2020-02-19T10:20:51Z

All right. @oli-obk the PR is awaiting your review, then. :)

oli-obk · 2020-02-19T13:14:10Z

@bors r+

bors · 2020-02-19T13:14:12Z

📌 Commit 58bb47e has been approved by oli-obk

bors · 2020-02-19T14:31:19Z

⌛ Testing commit 58bb47e with merge 46f7297f050cd21d50120779f5b06a7d3be72347...

bors · 2020-02-19T15:55:51Z

💔 Test failed - checks-azure

RalfJung · 2020-02-19T19:12:44Z

@bors r=oli-obk

bors · 2020-02-19T19:12:46Z

📌 Commit 88d14bf has been approved by oli-obk

@ghost

Rollup of 5 pull requests Successful merges: - #68877 (On mismatched argument count point at arguments) - #69185 (Unify and improve const-prop lints) - #69305 (Tweak binding lifetime suggestion text) - #69311 (Clean up E0321 and E0322) - #69317 (Fix broken link to the rustc guide) Failed merges: r? @ghost

RalfJung · 2020-02-21T10:02:41Z

I marked this for relnotes, given the user-visible changes here. I hope that is appropriate.

Pkgsrc changes: * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the bootstrap is not (yet?) available. Upstream changes: Version 1.43.0 (2020-04-23) ========================== Language -------- - [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having the type inferred correctly.][68129] - [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201] **Syntax only changes** - [Allow `type Foo: Ord` syntactically.][69361] - [Fuse associated and extern items up to defaultness.][69194] - [Syntactically allow `self` in all `fn` contexts.][68764] - [Merge `fn` syntax + cleanup item parsing.][68728] - [`item` macro fragments can be interpolated into `trait`s, `impl`s, and `extern` blocks.][69366] For example, you may now write: ```rust macro_rules! mac_trait { ($i:item) => { trait T { $i } } } mac_trait! { fn foo() {} } ``` These are still rejected *semantically*, so you will likely receive an error but these changes can be seen and parsed by macros and conditional compilation. Compiler -------- - [You can now pass multiple lint flags to rustc to override the previous flags.][67885] For example; `rustc -D unused -A unused-variables` denies everything in the `unused` lint group except `unused-variables` which is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies everything in the `unused` lint group **including** `unused-variables` since the allow flag is specified before the deny flag (and therefore overridden). - [rustc will now prefer your system MinGW libraries over its bundled libraries if they are available on `windows-gnu`.][67429] - [rustc now buffers errors/warnings printed in JSON.][69227] Libraries --------- - [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>` respectively.][69538] **Note** These conversions are only available when `N` is `0..=32`. - [You can now use associated constants on floats and integers directly, rather than having to import the module.][68952] e.g. You can now write `u32::MAX` or `f32::NAN` with no imports. - [`u8::is_ascii` is now `const`.][68984] - [`String` now implements `AsMut<str>`.][68742] - [Added the `primitive` module to `std` and `core`.][67637] This module reexports Rust's primitive types. This is mainly useful in macros where you want avoid these types being shadowed. - [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642] - [`string::FromUtf8Error` now implements `Clone + Eq`.][68738] Stabilized APIs --------------- - [`Once::is_completed`] - [`f32::LOG10_2`] - [`f32::LOG2_10`] - [`f64::LOG10_2`] - [`f64::LOG2_10`] - [`iter::once_with`] Cargo ----- - [You can now set config `[profile]`s in your `.cargo/config`, or through your environment.][cargo/7823] - [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's executable path when running integration tests or benchmarks.][cargo/7697] `<name>` is the name of your binary as-is e.g. If you wanted the executable path for a binary named `my-program`you would use `env!("CARGO_BIN_EXE_my-program")`. Misc ---- - [Certain checks in the `const_err` lint were deemed unrelated to const evaluation][69185], and have been moved to the `unconditional_panic` and `arithmetic_overflow` lints. Compatibility Notes ------------------- - [Having trailing syntax in the `assert!` macro is now a hard error.][69548] This has been a warning since 1.36.0. - [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly led to some instances being accepted, and now correctly emits a hard error. [69340]: rust-lang/rust#69340 Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of `rustc` and related tools. - [All components are now built with `opt-level=3` instead of `2`.][67878] - [Improved how rustc generates drop code.][67332] - [Improved performance from `#[inline]`-ing certain hot functions.][69256] - [traits: preallocate 2 Vecs of known initial size][69022] - [Avoid exponential behaviour when relating types][68772] - [Skip `Drop` terminators for enum variants without drop glue][68943] - [Improve performance of coherence checks][68966] - [Deduplicate types in the generator witness][68672] - [Invert control in struct_lint_level.][68725] [67332]: rust-lang/rust#67332 [67429]: rust-lang/rust#67429 [67637]: rust-lang/rust#67637 [67642]: rust-lang/rust#67642 [67878]: rust-lang/rust#67878 [67885]: rust-lang/rust#67885 [68129]: rust-lang/rust#68129 [68672]: rust-lang/rust#68672 [68725]: rust-lang/rust#68725 [68728]: rust-lang/rust#68728 [68738]: rust-lang/rust#68738 [68742]: rust-lang/rust#68742 [68764]: rust-lang/rust#68764 [68772]: rust-lang/rust#68772 [68943]: rust-lang/rust#68943 [68952]: rust-lang/rust#68952 [68966]: rust-lang/rust#68966 [68984]: rust-lang/rust#68984 [69022]: rust-lang/rust#69022 [69185]: rust-lang/rust#69185 [69194]: rust-lang/rust#69194 [69201]: rust-lang/rust#69201 [69227]: rust-lang/rust#69227 [69548]: rust-lang/rust#69548 [69256]: rust-lang/rust#69256 [69361]: rust-lang/rust#69361 [69366]: rust-lang/rust#69366 [69538]: rust-lang/rust#69538 [cargo/7823]: rust-lang/cargo#7823 [cargo/7697]: rust-lang/cargo#7697 [`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed [`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html [`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html [`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html [`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html [`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html

RalfJung added 6 commits February 14, 2020 22:49

const-prop: use one helper method for all lints; consistently lint ov…

ad4b3f3

…erflow on BinOps and not on Assert

fix tests, and use variants to test debug and release builds together

bd48522

more revisions and use them for another test

9c76394

add test for issue 69020

4b8c784

fix exceeding_bitshift lint and test

2107e73

expand assoc-const test a bit, just to be sure

415218f

rust-highfive assigned estebank Feb 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2020

rust-highfive assigned oli-obk and unassigned estebank Feb 15, 2020

RalfJung commented Feb 15, 2020

View reviewed changes

src/librustc_session/lint/builtin.rs Outdated Show resolved Hide resolved

RalfJung commented Feb 15, 2020

View reviewed changes

src/librustc_session/lint/builtin.rs Show resolved Hide resolved

RalfJung commented Feb 15, 2020

View reviewed changes

src/test/ui/issues/issue-8460-const.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

fix another test

25870a0

This comment has been minimized.

Sign in to view

fix compile-fail

c4a6f84

RalfJung commented Feb 15, 2020

View reviewed changes

src/test/compile-fail/consts/const-err3.rs Show resolved Hide resolved

oli-obk reviewed Feb 15, 2020

View reviewed changes

Centril reviewed Feb 15, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

Tighter type bounds for messages

0c8c800

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

This comment has been minimized.

Sign in to view

RalfJung added 2 commits February 15, 2020 19:02

adjust run-fail tests

3134df2

remove no-longer-needed test

94047f1

This comment has been minimized.

Sign in to view

avoid excessive number of revisions

58bb47e

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2020

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2020

fix 32bit-only test

88d14bf

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2020

Centril mentioned this pull request Feb 20, 2020

Rollup of 5 pull requests #69325

Merged

bors merged commit d237e0f into rust-lang:master Feb 20, 2020

RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 21, 2020

RalfJung deleted the const-prop-lints branch February 21, 2020 10:02

ehuss mentioned this pull request Feb 21, 2020

Update for change in const lint name. rust-lang/reference#764

Merged

RalfJung mentioned this pull request Feb 24, 2020

"exceeding_bitshifts" lint does not work in associated consts #69021

Closed

ehuss mentioned this pull request May 14, 2021

Allowing a lint fails on let binding. #51942

Closed

RalfJung mentioned this pull request Sep 28, 2022

Heads-up: const_err lint is going away rusticstuff/simdutf8#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify and improve const-prop lints #69185

Unify and improve const-prop lints #69185

RalfJung commented Feb 15, 2020 •

edited

Loading

rust-highfive commented Feb 15, 2020

RalfJung commented Feb 15, 2020

RalfJung Feb 15, 2020

oli-obk Feb 15, 2020

RalfJung Feb 15, 2020

RalfJung Feb 15, 2020

This comment has been minimized.

This comment has been minimized.

oli-obk Feb 15, 2020

This comment has been minimized.

This comment has been minimized.

RalfJung commented Feb 15, 2020

RalfJung commented Feb 18, 2020

Centril commented Feb 18, 2020

RalfJung commented Feb 19, 2020

oli-obk commented Feb 19, 2020

bors commented Feb 19, 2020

bors commented Feb 19, 2020

This comment has been minimized.

bors commented Feb 19, 2020

RalfJung commented Feb 19, 2020

bors commented Feb 19, 2020

RalfJung commented Feb 21, 2020

Unify and improve const-prop lints #69185

Unify and improve const-prop lints #69185

Conversation

RalfJung commented Feb 15, 2020 • edited Loading

rust-highfive commented Feb 15, 2020

RalfJung commented Feb 15, 2020

RalfJung Feb 15, 2020

Choose a reason for hiding this comment

oli-obk Feb 15, 2020

Choose a reason for hiding this comment

RalfJung Feb 15, 2020

Choose a reason for hiding this comment

RalfJung Feb 15, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

oli-obk Feb 15, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

RalfJung commented Feb 15, 2020

RalfJung commented Feb 18, 2020

Centril commented Feb 18, 2020

RalfJung commented Feb 19, 2020

oli-obk commented Feb 19, 2020

bors commented Feb 19, 2020

bors commented Feb 19, 2020

This comment has been minimized.

bors commented Feb 19, 2020

RalfJung commented Feb 19, 2020

bors commented Feb 19, 2020

RalfJung commented Feb 21, 2020

RalfJung commented Feb 15, 2020 •

edited

Loading