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.

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

Fix non-constant value ICE (#90878) #90930

Merged
merged 4 commits into from
Nov 20, 2021

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Nov 15, 2021

This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of const to replace with a let. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the const and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found const is on line 0, because that triggers the ICE.

Asking Esteban to review since he was the last one to change the relevant code.

r? @estebank

Fixes #90878

This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of `const` to replace with a `let`. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the `const` and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found `const` is on line 0, because that triggers the ICE.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2021
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2021

📌 Commit d64aea6 has been approved by estebank

@bors 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 Nov 16, 2021
@Badel2
Copy link
Contributor

Badel2 commented Nov 16, 2021

Hi, I found another edge case that is not fixed by this change, when the first character is a newline instead of a space:


fn main() {
    |x: usize| [0; x];  //~ ERROR attempt to use a non-constant value in a constant [E0435]
    // (note the empty line before "fn")
}

@Noratrieb
Copy link
Member Author

I actually tested it out with newlines myself, but apparently I messed up, thank you for finding it!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2021
…r=estebank

Fix `non-constant value` ICE (rust-lang#90878)

This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of `const` to replace with a `let`. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the `const` and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found `const` is on line 0, because that triggers the ICE.

Asking Esteban to review since he was the last one to change the relevant code.

r? `@estebank`

Fixes rust-lang#90878
@Noratrieb
Copy link
Member Author

I don't find a way to make a test for that case since tidy rejects the test because of the traling newline. But I tested it locally and let's hope that there will never be a regression for that case 😅 (if you know of an option to ignore tidy for a file like that hit me up)

…ewline

I cannot provide a test for that thanks to tidy.
@estebank
Copy link
Contributor

// ignore-tidy-end-whitespace and/or // ignore-tidy-trailing-newlines

@Noratrieb
Copy link
Member Author

Does the ignore comment have to be at the start of the file or anywhere? Because it wouldn't be possible to insert it at the start. (also it's a leading newline and not a trailing one but I guess you can ignore that too)

@Noratrieb
Copy link
Member Author

Turns out there wasn't an option for ignoring leading newlines, so I simply added it :). I hope this is that misplaced here, but tell me how I should handle it otherwise if this isn't very good

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2021
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2021

📌 Commit 96c37c8 has been approved by estebank

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 20, 2021
…r=estebank

Fix `non-constant value` ICE (rust-lang#90878)

This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of `const` to replace with a `let`. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the `const` and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found `const` is on line 0, because that triggers the ICE.

Asking Esteban to review since he was the last one to change the relevant code.

r? `@estebank`

Fixes rust-lang#90878
This was referenced Nov 20, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#90575 (Improve suggestions for compatible variants on type mismatch.)
 - rust-lang#90628 (Clarify error messages caused by re-exporting `pub(crate)` visibility to outside)
 - rust-lang#90930 (Fix `non-constant value` ICE (rust-lang#90878))
 - rust-lang#90983 (Make scrollbar in the sidebar always visible for visual consistency)
 - rust-lang#91021 (Elaborate `Future::Output` when printing opaque `impl Future` type)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7993571 into rust-lang:master Nov 20, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 20, 2021
@Noratrieb Noratrieb deleted the fix-non-const-value-ice branch November 20, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants