Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
RalfJung opened this issue Feb 10, 2020 · 6 comments · Fixed by #70566 or #71663
Closed

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

RalfJung opened this issue Feb 10, 2020 · 6 comments · Fixed by #70566 or #71663
Assignees
@rustbot
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Feb 10, 2020 •
edited
Loading

The exceeding_bitshifts lint should fire on the following code, but it does not:

pub trait Foo {
    const N: i32;
}

impl<T: Foo> Foo for Vec<T> {
    const N: i32 = T::N << 42; // no warning here
}

Playground
(This testcase now exists in-tree as a FIXME in ui/lint/lint-exceeding-bitshifts.rs.)

Cc @oli-obk @wesleywiser

This issue has been assigned to @jumbatm via this comment.
@RalfJung RalfJung changed the title "exceeding_bitshifts" does not work in debug mode and in associated consts "exceeding_bitshifts" lint does not work in debug mode and in associated consts Feb 10, 2020
@RalfJung RalfJung mentioned this issue Feb 10, 2020
Merged
@jonas-schievink jonas-schievink added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 10, 2020
@jumbatm
Copy link
Contributor

jumbatm commented Feb 11, 2020

I'd like to take this.

@rustbot claim

@rustbot rustbot self-assigned this Feb 11, 2020
@RalfJung
Copy link
Member Author

@jumbatm I have some plans to follow-up on #68969 with some cleanup. That might at least partially resolve this issue here. So it's probably best to wait until after that is done.

@RalfJung RalfJung mentioned this issue Feb 15, 2020
Merged
Centril added a commit to Centril/rust that referenced this issue Feb 20, 2020
@Centril
Rollup merge of rust-lang#69185 - RalfJung:const-prop-lints, r=oli-obk
d237e0f 
Unify and improve const-prop lints

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](rust-lang#69185 (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 rust-lang#69020.
Helps with rust-lang#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 rust-lang#61821; the duplication there has entirely different reasons.
@RalfJung RalfJung changed the title "exceeding_bitshifts" lint does not work in debug mode and in associated consts "exceeding_bitshifts" lint does not work in associated consts Feb 24, 2020
@RalfJung
Copy link
Member Author

All right, #69185 landed, so what I planned here is done.

The remaining issue is about associated consts. The issue there is that check_binary_op does not even get called (according to trace! output). My guess is that one of the conditionals guarding this line fails, or maybe we are hitting an early return in const_prop, but I have not tried further.

The issue is all yours, @jumbatm :)

@jumbatm
Copy link
Contributor

jumbatm commented Feb 24, 2020

Great! I'll start investigating.

@jumbatm jumbatm mentioned this issue Mar 15, 2020
Closed
@jumbatm jumbatm mentioned this issue Mar 30, 2020
Merged
@RalfJung RalfJung mentioned this issue Apr 1, 2020
Closed
@RalfJung
Copy link
Member Author

RalfJung commented Apr 1, 2020 •
edited
Loading

Possibly related: #70660 Never mind, that one is already fixed on beta.

jumbatm added a commit to jumbatm/rust that referenced this issue Apr 4, 2020
@jumbatm
Amend exceeding-bitshifts test case for rust-lang#69021.
5118048 
- Change to warnings so that all lints are emitted
- Expect build-pass
- Change placeholder FIXME to WARN.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#70566 - jumbatm:exceeding-bitshifts-constpr…
bf06312 
…op, r=RalfJung

Don't bail out before linting in generic contexts.

Fixes rust-lang#69021.

cc rust-lang#70017

r? @RalfJung
jumbatm added a commit to jumbatm/rust that referenced this issue Apr 14, 2020
@jumbatm
Amend exceeding-bitshifts test case for rust-lang#69021.
3e6609e 
- Change to warnings so that all lints are emitted
- Expect build-pass
- Change placeholder FIXME to WARN.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 16, 2020
@Dylan-DPC
Rollup merge of rust-lang#70566 - jumbatm:exceeding-bitshifts-constpr…
c540bcc 
…op, r=RalfJung

Don't bail out before linting in generic contexts.

Fixes rust-lang#69021.

cc rust-lang#70017

r? @RalfJung
@bors bors closed this as completed in 33500a2 Apr 16, 2020
@RalfJung
Copy link
Member Author

#70566 got reverted in #71533, so we have to reopen this.

@RalfJung RalfJung reopened this Apr 25, 2020
@jumbatm jumbatm mentioned this issue Apr 29, 2020
Merged
@bors bors closed this as completed in 5b17290 May 3, 2020
@RalfJung RalfJung mentioned this issue May 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rustbot rustbot

Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
4 participants
@RalfJung @jonas-schievink @jumbatm @rustbot