Fix float ICE #90927

terrarier2111 · 2021-11-15T16:52:47Z

This fixes #90728

rust-highfive · 2021-11-15T16:52:51Z

(rust-highfive has picked a reviewer for you, use r? to override)

terrarier2111 · 2021-11-15T18:30:04Z

Should i create a sub-directory for my test, because the CI errors because there are 1001 files in the parser test directory but only 1000 files are allowed in a single directory?

jackh726 · 2021-11-15T18:44:44Z

Maybe put it in src/test/ui/parser/float-literals.rs? I guess there's also a src/ext/test-float-parse, but I'm unsure what should go there vs src/test/ui. I think of #86761 when I see this, so maybe cc @estebank, who r+ed that PR

terrarier2111 · 2021-11-15T19:08:32Z

@estebank what do you think is the best solution?

estebank

I'm ok with moving the test to src/ext/test-float-parse. After all, it is testing the parser.

src/test/ui/parser/issue-90728.rs

estebank · 2021-11-16T05:11:13Z

src/test/ui/parser/issue-90728.stderr

+error: expected at least one digit in exponent
+  --> $DIR/issue-90728.rs:2:7
+   |
+LL |     a.5.2E+
+   |       ^^^^^


This error in particular could likely be more actionable.

Could you be a bit more precise about that?

I mean using a span_suggestion_verbose (and SourceMap::next_point(Span)) to produce something like the following (just a rough draft, doesn't have to look like this exactly):

error: expected at least one digit in exponent --> $DIR/issue-90728.rs:2:7 | LL | a.5.2E+ | -----^ expected a number here | | | this numeric literal is incomplete help: write a valid exponent | LL | a.5.2E+1 | + This is similar to what we do on missing `;` or `,` in statements or between `fn` args.

estebank · 2021-11-16T05:11:34Z

src/test/ui/parser/issue-90728.stderr

+error: unexpected token: `5.2E+`
+  --> $DIR/issue-90728.rs:2:7
+   |
+LL |     a.5.2E+
+   |       ^^^^^
+
+error: expected one of `.`, `;`, `?`, `}`, or an operator, found `5.2E+`
+  --> $DIR/issue-90728.rs:2:7
+   |
+LL |     a.5.2E+
+   |       ^^^^^ expected one of `.`, `;`, `?`, `}`, or an operator


And we could recover more gracefully after encountering it ^_^

Do you mean like spitting out no errors afterwards?

Correct, but that doesn't have to be addressed in this PR.

terrarier2111 · 2021-11-18T16:18:45Z

For some reason the test doesn't work any more correctly after moving it to the suggested location (but it works locally). Do you guys have any idea what the issue is? @estebank @jackh726

jackh726 · 2021-11-18T16:26:56Z

Looks like it's caused by tidy checking that file with stage0. Probably the easiest thing to do is to do a tidy ignore with a FIXME to remove it after bootstrap is updated.

terrarier2111 · 2021-11-18T16:33:53Z

Looks like it's caused by tidy checking that file with stage0. Probably the easiest thing to do is to do a tidy ignore with a FIXME to remove it after bootstrap is updated.

Okay, how would I do that?

jackh726 · 2021-11-18T16:53:14Z

I think maybe #[rustfmt::skip] on fn main() will work?

Mark-Simulacrum · 2021-11-18T17:21:58Z

The test appears to be in src/etc which is why tidy complained - src/test is not fmt checked currently. I'm not sure we run tests in src/etc at all, so maybe it was accidentally moved into that directory?

Or, if not, the appropriate fix is to adjust rustfmt.toml at the root and ignore the right directory for this test.

terrarier2111 · 2021-11-18T17:25:44Z

The test appears to be in src/etc which is why tidy complained - src/test is not fmt checked currently. I'm not sure we run tests in src/etc at all, so maybe it was accidentally moved into that directory?

Or, if not, the appropriate fix is to adjust rustfmt.toml at the root and ignore the right directory for this test.

there was src/ext/test-float-parse suggested which doesn't exist, but i found out that src/etc/test-float-parse actually does exist that's why i moved it in there instead idk if that was wrong

Mark-Simulacrum · 2021-11-18T17:58:21Z

How was src/etc/test-float-parse/src/bin/issue-90728.stderr generated? As far as I can tell, CI does not run test-float-parse tests...

It seems like this test probably belongs under the UI test directory (src/test/ui).

terrarier2111 · 2021-11-18T18:24:05Z

How was src/etc/test-float-parse/src/bin/issue-90728.stderr generated? As far as I can tell, CI does not run test-float-parse tests...

It seems like this test probably belongs under the UI test directory (src/test/ui).

I did put it into src/test/ui/parser/ but it errored because there were already 1000 tests inside the directory which is why i had to move it in the first place

jackh726 · 2021-11-18T18:52:21Z

Okay, let's just move this as a test case in src/test/ui/parser/float-literals.rs. Can you also squash the commits?

terrarier2111 · 2021-11-18T19:18:05Z

Okay, let's just move this as a test case in src/test/ui/parser/float-literals.rs. Can you also squash the commits?

But my test is supposed to fail while the float literals stuff is supposed to compile. Also i will squash my commits once this test issue is resolved.

jackh726 · 2021-11-18T20:27:07Z

Okay...so new plan. Let's make an issues directory in parser and move all the issue-* tests there...but can we do that in a separate PR? Sorry, it's a pain.

Mark-Simulacrum · 2021-11-18T21:00:18Z

You can just make an exception in tidy (or bump the limit) -- that'll temporarily resolve the problem to let this PR proceed.

// check-pass or // check-fail etc can work to adjust tests into pass/fail as needed.

jackh726 · 2021-11-18T21:01:54Z

Yeah, I guess a tidy exception is fine.

terrarier2111 · 2021-11-18T21:11:24Z

You can just make an exception in tidy (or bump the limit) -- that'll temporarily resolve the problem to let this PR proceed.

// check-pass or // check-fail etc can work to adjust tests into pass/fail as needed.

Where can I bump that limit? and should i bump it to 1001 or to something else?

jackh726 · 2021-11-18T21:21:32Z

That would be

rust/src/tools/tidy/src/ui_tests.rs

Line 26 in b6f580a

let limit = if is_root {

jackh726 · 2021-11-18T21:33:38Z

You would have to make a new one. And probably just use 1001 as a limit.

terrarier2111 · 2021-11-19T14:29:50Z

I had to pull the changes from master because there were some changes made to the test limits and now if i try to build the compiler i get the following ICE: https://pastebin.com/VpX3QZZC @jackh726
Isn't there a better solution for this testing stuff than increasing the limit in every pr which adds a test? Also what's this limit's purpose in the first place if it gets constantly changed?

jackh726 · 2021-11-19T17:20:53Z

Maybe try to ./x.py clean?

We weren't shouldn't be increasing the test limit - the fact that we've hit it means that we're already past the point of the directory being "unwieldy". A proper solution would be either moving tests into subdirectories (like issues, but others maybe) or to combine related tests (this can be tricky and there hasn't really been a push for this).

Another thing to think about (not for this PR), would be to have tidy issue a warning at some lower threshold than the 1000 for errors. In theory, the noise might prompt a contributor to organize tests a bit.

Co-authored-by: Esteban Kuber <[email protected]>

terrarier2111 · 2021-11-20T16:25:12Z

@jackh726 i think this should be ready to be merged.

jackh726 · 2021-11-20T20:32:05Z

Its helpful that PRs prior to this had to handle the parser test limit 🤣

@bors r+ rollup

bors · 2021-11-20T20:32:07Z

📌 Commit 5f6059d has been approved by jackh726

…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#89741 (Mark `Arc::from_inner` / `Rc::from_inner` as unsafe) - rust-lang#90927 (Fix float ICE) - rust-lang#90994 (Fix ICE `rust-lang#90993`: add missing call to cancel) - rust-lang#91018 (Adopt let_else in more places in rustc_mir_build) - rust-lang#91022 (Suggest `await` in more situations where infer types are involved) - rust-lang#91088 (Revert "require full validity when determining the discriminant of a value") Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Nov 15, 2021

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

This comment has been minimized.

Sign in to view

estebank approved these changes Nov 16, 2021

View reviewed changes

src/test/ui/parser/issue-90728.rs Outdated Show resolved Hide resolved

estebank reviewed Nov 16, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

terrarier2111 force-pushed the fix-float-ice branch from 02ebda8 to aa88fbd Compare November 20, 2021 11:17

This comment has been minimized.

Sign in to view

terrarier2111 force-pushed the fix-float-ice branch from aa88fbd to 7909efd Compare November 20, 2021 15:29

Fix float ICE

5f6059d

Co-authored-by: Esteban Kuber <[email protected]>

terrarier2111 force-pushed the fix-float-ice branch from a678e54 to 5f6059d Compare November 20, 2021 15:42

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 20, 2021

matthiaskrgr mentioned this pull request Nov 20, 2021

Rollup of 6 pull requests #91093

Merged

bors merged commit 410d64f into rust-lang:master Nov 21, 2021

rustbot added this to the 1.58.0 milestone Nov 21, 2021

terrarier2111 deleted the fix-float-ice branch December 6, 2021 17:24

Fix float ICE #90927

Fix float ICE #90927

Conversation

terrarier2111 commented Nov 15, 2021

rust-highfive commented Nov 15, 2021

This comment has been minimized.

terrarier2111 commented Nov 15, 2021

jackh726 commented Nov 15, 2021

terrarier2111 commented Nov 15, 2021

estebank left a comment

Choose a reason for hiding this comment

estebank Nov 16, 2021

Choose a reason for hiding this comment

terrarier2111 Nov 16, 2021

Choose a reason for hiding this comment

estebank Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

estebank Nov 16, 2021

Choose a reason for hiding this comment

terrarier2111 Nov 16, 2021

Choose a reason for hiding this comment

estebank Nov 16, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

terrarier2111 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

terrarier2111 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

Mark-Simulacrum commented Nov 18, 2021

terrarier2111 commented Nov 18, 2021

This comment has been minimized.

Mark-Simulacrum commented Nov 18, 2021 • edited Loading

terrarier2111 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

terrarier2111 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

Mark-Simulacrum commented Nov 18, 2021

jackh726 commented Nov 18, 2021

terrarier2111 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

jackh726 commented Nov 18, 2021

This comment has been minimized.

This comment has been minimized.

terrarier2111 commented Nov 19, 2021 • edited Loading

jackh726 commented Nov 19, 2021

This comment has been minimized.

terrarier2111 commented Nov 20, 2021

jackh726 commented Nov 20, 2021

bors commented Nov 20, 2021

estebank Nov 19, 2021 •

edited

Loading

Mark-Simulacrum commented Nov 18, 2021 •

edited

Loading

terrarier2111 commented Nov 19, 2021 •

edited

Loading