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 error counting #62055

Merged
merged 2 commits into from Jun 25, 2019

Conversation

Projects
None yet
6 participants
@matthewjasper
Copy link
Contributor

commented Jun 22, 2019

Count duplicate errors for track_errors and other error counting checks.
Add FIXMEs to make it clear that we should be moving away from this kind of logic.

Closes #61663

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

📌 Commit 95a3215 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

nominating for beta backport since it fixes an ICE that is a stable-to-beta regression.

Centril added a commit to Centril/rust that referenced this pull request Jun 24, 2019

Rollup merge of rust-lang#62055 - matthewjasper:fix-error-counting, r…
…=pnkfelix

Fix error counting

Count duplicate errors for `track_errors` and other error counting checks.
Add FIXMEs to make it clear that we should be moving away from this kind of logic.

Closes rust-lang#61663

bors added a commit that referenced this pull request Jun 24, 2019

Auto merge of #62093 - Centril:rollup-lko9zgp, r=Centril
Rollup of 3 pull requests

Successful merges:

 - #62055 (Fix error counting)
 - #62078 (Remove built-in derive macros `Send` and `Sync`)
 - #62085 (Add test for issue-38591)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 25, 2019

Rollup merge of rust-lang#62055 - matthewjasper:fix-error-counting, r…
…=pnkfelix

Fix error counting

Count duplicate errors for `track_errors` and other error counting checks.
Add FIXMEs to make it clear that we should be moving away from this kind of logic.

Closes rust-lang#61663

bors added a commit that referenced this pull request Jun 25, 2019

Auto merge of #62119 - Centril:rollup-el20wu0, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #61814 (Fix an ICE with uninhabited consts)
 - #61987 (rustc: produce AST instead of HIR from `hir::lowering::Resolver` methods.)
 - #62055 (Fix error counting)
 - #62078 (Remove built-in derive macros `Send` and `Sync`)
 - #62085 (Add test for issue-38591)
 - #62091 (HirIdification: almost there)
 - #62096 (Implement From<Local> for Place and PlaceBase)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 25, 2019

Auto merge of #62119 - Centril:rollup-el20wu0, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #61814 (Fix an ICE with uninhabited consts)
 - #61987 (rustc: produce AST instead of HIR from `hir::lowering::Resolver` methods.)
 - #62055 (Fix error counting)
 - #62078 (Remove built-in derive macros `Send` and `Sync`)
 - #62085 (Add test for issue-38591)
 - #62091 (HirIdification: almost there)
 - #62096 (Implement From<Local> for Place and PlaceBase)

Failed merges:

r? @ghost

@bors bors merged commit 95a3215 into rust-lang:master Jun 25, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
pr Build #20190622.26 succeeded
Details
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@rust-lang/compiler We need to get beta backports approved a little earlier this cycle as we're promoting beta->stable on Saturday. If we could get an FCP going for beta-accepted on this PR that'd be great; or if someone wants to unilaterally beta accept that works too.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I'm going to un-nominate this.

The ICEs it resolves do not seem like a critical issue to me: As far as I can tell, the only say these ICEs ever arise is after some legitimate error diagnostic has already been emitted, and we just failed to halt the compilation as soon as we should have. But the compilation should always halt, potentially via an ICE, without generating object code.

@matthewjasper does that sound correct to you? Or is there a scenario where this could somehow generate an error diagnostic, but then end up not having any ICE and end up emitting potentially erroneous object code?

@@ -4376,7 +4376,7 @@ pub fn check_bounds_are_used<'tcx>(tcx: TyCtxt<'tcx>, generics: &ty::Generics, t
} else if let ty::Error = leaf_ty.sty {
// If there is already another error, do not emit
// an error for not using a type Parameter.
assert!(tcx.sess.err_count() > 0);
assert!(tcx.sess.has_errors());

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 26, 2019

Member

Code like this should probably be using delay_span_bug uniformly.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 26, 2019

Contributor

I'd prefer an EmittedError type only created by emitting errors and putting that in ty::Error instead.

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 26, 2019

Member

That's even nicer - we might want to use that in the AST and HIR as well.

But maybe it should be something less temporarily ordered, so if there is some place where ty::Error needs to be created but it's not the same place the errors is to be reported from, we can still create a proof that "some error has happened or will happen" with delay_span_bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.