Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` #59199

estebank · 2019-03-15T02:40:21Z

In the spirit of continuing through errors in type checking (#39275), remove track_errors from a couple of locations in the codebase.

rust-highfive · 2019-03-15T02:40:24Z

r? @davidtwco

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

estebank · 2019-03-15T02:42:45Z

@davidtwco DO NOT APPROVE without agreement from the compiler team.

CC @rust-lang/compiler please poke holes at the code. If you can come up with test cases that would break with this change, please post them here so we can properly handle them. An alternative to landing this as is would be to enable it only in nightly to gather information. I would also like to have a crater run.

@bors try

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` In the spirit of continuing through errors in type checking (#39275), remove `track_errors` from a couple of locations in the codebase.

bors · 2019-03-15T05:06:25Z

☀️ Try build successful - checks-travis
Build commit: 68fcf88

estebank · 2019-03-15T05:19:06Z

@craterbot run start=master#bc44841ad2a2ad5f6c5e67b9e35ed8e7e71d4dc7 end=try#68fcf881bd1e4dbfc21e22f8c7a56b8a5d8e8f7b mode=check-only

craterbot · 2019-03-15T05:19:08Z

👌 Experiment pr-59199 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2019-03-15T05:19:10Z

🚧 Experiment pr-59199 is now running on agent aws-3-tmp.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

src/librustc_mir/build/matches/mod.rs

src/librustc_mir/hair/pattern/check_match.rs

src/librustc_typeck/check/mod.rs

craterbot · 2019-03-16T07:19:08Z

🎉 Experiment pr-59199 is completed!
📊 0 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

estebank · 2019-03-18T04:31:16Z

I didn't see any regressions... Would @rust-lang/compiler feel comfortable landing this and letting it ride the release train to catch any potential ICEs?

src/librustc_mir/build/matches/mod.rs

eddyb · 2019-03-18T08:02:21Z

src/librustc_mir/const_eval.rs

-        if let Err(ErrorReported) = tcx.check_match(def_id) {
-            return Err(ErrorHandled::Reported)
-        }
+        tcx.check_match(def_id);


Calling this feels pointless, also similar for mir_const_qualif below.
cc @oli-obk Can we just remove these and rely only on tainted_by_errors instead?

I seem to remember that building MIR before match checking results in ICEs. Not sure this belongs here though.

That shouldn't make sense nowadays, though, there should be no mutable state involved?

As in check_match reports errors for match expressions while MIR building ICEs on those same expressions.

Ah, sure, but that means MIR building should be handling this gracefully.

Are you ok with this being addressed in a separate PR? If so, I'll open an issue

It should be handled here, as the behavior of "exit if check_match errored" is gone! You could try to turn an error into an ICE if you want.

But I guess I'd be fine with a // FIXME(#...) check_match may have errored but we're not checking for that anymore here.

eddyb · 2019-03-18T08:03:19Z

src/librustc_mir/hair/pattern/check_match.rs

    let body_id = if let Some(id) = tcx.hir().as_local_hir_id(def_id) {
        tcx.hir().body_owned_by(id)
    } else {
-        return Ok(());
+        return;


cc @nikomatsakis Is this intentional? I remember some of the queries I wrote would panic if they got a non-local DefId.

eddyb · 2019-03-18T08:13:20Z

r=me with nits fixed, I guess?

davidtwco · 2019-03-18T08:52:39Z

r? @eddyb

estebank · 2019-03-18T17:53:00Z

For what is worth, greping through the crater results I couldn't find any ICEs, but of course these are more likely to crop up when actually writing code. I see four course of actions:

do nothing, kill this PR and revisit in the future
merge this PR now and deal with problems as they come up
delay merging this PR until the next beta gets cut, so that we have a full release cycle to catch problems that might come up
merge this PR modified to gate the new behavior on being a nightly compiler to have multiple cycles of testing in nightly before promoting and letting it ride the train

eddyb · 2019-03-21T06:58:40Z

@estebank Worst case, you've turned an error into an ICE. Code that compiled is almost impossible to be affected.

I'd say we should merge this, so r=me after #59199 (comment) is addressed (just the comment + issue should be enough, seems low-priority).

estebank · 2019-03-23T03:30:27Z

@bors r=eddyb

bors · 2019-03-23T03:30:28Z

📌 Commit ffc97cf3b3621e8f47a2512b822cdcf05950f0bd has been approved by eddyb

estebank · 2019-03-23T03:32:26Z

@bors r=eddyb

bors · 2019-03-23T03:32:27Z

📌 Commit 5723632 has been approved by eddyb

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` In the spirit of continuing through errors in type checking (rust-lang#39275), remove `track_errors` from a couple of locations in the codebase.

bors · 2019-03-24T05:38:17Z

⌛ Testing commit 5723632 with merge fb5ed48...

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` In the spirit of continuing through errors in type checking (#39275), remove `track_errors` from a couple of locations in the codebase.

bors · 2019-03-24T09:05:30Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing fb5ed48 to master...

rust-highfive assigned davidtwco Mar 15, 2019

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

estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 15, 2019

This comment has been minimized.

Sign in to view

craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2019

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 15, 2019

eddyb reviewed Mar 15, 2019

View reviewed changes

src/librustc_mir/build/matches/mod.rs Outdated Show resolved Hide resolved

eddyb reviewed Mar 15, 2019

View reviewed changes

src/librustc_mir/hair/pattern/check_match.rs Outdated Show resolved Hide resolved

eddyb reviewed Mar 15, 2019

View reviewed changes

src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 16, 2019

estebank force-pushed the untrack-errors branch from eaa69d5 to a794f7a Compare March 18, 2019 00:38

eddyb reviewed Mar 18, 2019

View reviewed changes

src/librustc_mir/build/matches/mod.rs Show resolved Hide resolved

eddyb reviewed Mar 18, 2019

View reviewed changes

rust-highfive assigned eddyb and unassigned davidtwco Mar 18, 2019

estebank added 4 commits March 22, 2019 20:15

Remove track_errors from check_match

d1808aa

Continue running after typeck_item_bodies has failed

936dec8

Do not track_errors in register_plugins

c89872c

address review comments

afdc38d

estebank mentioned this pull request Mar 23, 2019

Follow up on const_eval.rs check_match call #59378

Closed

estebank force-pushed the untrack-errors branch from a794f7a to ffc97cf Compare March 23, 2019 03:30

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 Mar 23, 2019

Add note to address check_match errors

5723632

estebank force-pushed the untrack-errors branch from ffc97cf to 5723632 Compare March 23, 2019 03:32

Mark-Simulacrum mentioned this pull request Mar 23, 2019

Rollup of 8 pull requests #59387

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 24, 2019

bors merged commit 5723632 into rust-lang:master Mar 24, 2019

estebank deleted the untrack-errors branch November 9, 2023 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` #59199

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` #59199

estebank commented Mar 15, 2019

rust-highfive commented Mar 15, 2019

estebank commented Mar 15, 2019

This comment has been minimized.

bors commented Mar 15, 2019

This comment has been minimized.

This comment has been minimized.

estebank commented Mar 15, 2019

craterbot commented Mar 15, 2019

craterbot commented Mar 15, 2019

craterbot commented Mar 16, 2019

estebank commented Mar 18, 2019

eddyb Mar 18, 2019 •

edited

Loading

Zoxc Mar 18, 2019

eddyb Mar 18, 2019

Zoxc Mar 18, 2019

eddyb Mar 18, 2019

oli-obk Mar 18, 2019

eddyb Mar 21, 2019

eddyb Mar 21, 2019

eddyb Mar 18, 2019

eddyb commented Mar 18, 2019

davidtwco commented Mar 18, 2019

estebank commented Mar 18, 2019

eddyb commented Mar 21, 2019

estebank commented Mar 23, 2019

bors commented Mar 23, 2019

estebank commented Mar 23, 2019

bors commented Mar 23, 2019

bors commented Mar 24, 2019

bors commented Mar 24, 2019

Remove track_errors from check_match, typeck_item_bodies and register_plugins #59199

Remove track_errors from check_match, typeck_item_bodies and register_plugins #59199

Conversation

estebank commented Mar 15, 2019

rust-highfive commented Mar 15, 2019

estebank commented Mar 15, 2019

This comment has been minimized.

bors commented Mar 15, 2019

This comment has been minimized.

This comment has been minimized.

estebank commented Mar 15, 2019

craterbot commented Mar 15, 2019

craterbot commented Mar 15, 2019

craterbot commented Mar 16, 2019

estebank commented Mar 18, 2019

eddyb Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Mar 18, 2019

davidtwco commented Mar 18, 2019

estebank commented Mar 18, 2019

eddyb commented Mar 21, 2019

estebank commented Mar 23, 2019

bors commented Mar 23, 2019

estebank commented Mar 23, 2019

bors commented Mar 23, 2019

bors commented Mar 24, 2019

bors commented Mar 24, 2019

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` #59199

Remove `track_errors` from `check_match`, `typeck_item_bodies` and `register_plugins` #59199

eddyb Mar 18, 2019 •

edited

Loading