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

Don't carry MIR location in ConstraintCategory::CallArgument #103641

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

compiler-errors
Copy link
Member

It turns out that ConstraintCategory::CallArgument cannot just carry a MIR location in it, since we may bubble them up to totally different MIR bodies.

So instead, revert the commit a6b5f95, and instead just erase regions from the original Option<Ty<'tcx>> that it carried, so that it doesn't ICE with the changes in #103220.

Best reviewed in parts -- the first is just a revert, and the second is where the meaningful changes happen.

Fixes #103624

@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2022

r? @cjgillot

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 27, 2022
= note: requirement occurs because of a function pointer to `foo`
= note: the function `foo` is invariant over the parameter `'a`
= note: requirement occurs because of the type `Type<'_>`, which makes the generic argument `'_` invariant
= note: the struct `Type<'a>` is invariant over the parameter `'a`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does erasing regions change diagnostics?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, good question -- I have no idea. My first suspicion when I put this PR up was due to the Ord implementation on ConstraintCategory and something about the sorting of constraints in best_blame_constraint, but I can look into it further.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's because we're not sorting by variance_info, but only the constraint:

categorized_path.sort_by(|p0, p1| p0.category.cmp(&p1.category));

And we have several constraints that differ only in variant info:

1ms DEBUG rustc_borrowck::region_infer sorted_path=[
│     BlameConstraint {
│         category: CallArgument(
│             Some(
│                 fn(fn() -> Type<'_> {foo::<'_>}, <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output) -> <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output {bar::<fn() -> Type<'_> {foo::<'_>}>},
│             ),
│         ),
│         from_closure: false,
│         cause: ObligationCause {
│             span: src/test/ui/associated-types/cache/project-fn-ret-invariant.rs:42:13: 42:22 (#0),
│             body_id: HirId {
│                 owner: OwnerId {
│                     def_id: DefId(0:0 ~ project_fn_ret_invariant[129b]),
│                 },
│                 local_id: 0,
│             },
│             code: InternedObligationCauseCode {
│                 code: None,
│             },
│         },
│         variance_info: Invariant {
│             ty: Type<'_>,
│             param_index: 0,
│         },
│         outlives_constraint: ('_#19r: '_#6r) due to Single(bb1[8]) (Invariant { ty: Type<'_>, param_index: 0 }) (CallArgument(Some(fn(fn() -> Type<'_> {foo::<'_>}, <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output) -> <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output {bar::<fn() -> Type<'_> {foo::<'_>}>}))),
│     },
│     BlameConstraint {
│         category: CallArgument(
│             Some(
│                 fn(fn() -> Type<'_> {foo::<'_>}, <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output) -> <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output {bar::<fn() -> Type<'_> {foo::<'_>}>},
│             ),
│         ),
│         from_closure: false,
│         cause: ObligationCause {
│             span: src/test/ui/associated-types/cache/project-fn-ret-invariant.rs:42:13: 42:22 (#0),
│             body_id: HirId {
│                 owner: OwnerId {
│                     def_id: DefId(0:0 ~ project_fn_ret_invariant[129b]),
│                 },
│                 local_id: 0,
│             },
│             code: InternedObligationCauseCode {
│                 code: None,
│             },
│         },
│         variance_info: Invariant {
│             ty: fn() -> Type<'_> {foo::<'_>},
│             param_index: 0,
│         },
│         outlives_constraint: ('_#6r: '_#18r) due to Single(bb1[8]) (Invariant { ty: fn() -> Type<'_> {foo::<'_>}, param_index: 0 }) (CallArgument(Some(fn(fn() -> Type<'_> {foo::<'_>}, <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output) -> <fn() -> Type<'_> {foo::<'_>} as std::ops::FnOnce<()>>::Output {bar::<fn() -> Type<'_> {foo::<'_>}>}))),
│     },

[...]

-- so erasing regions probably causes hashes to change which causes the order constraints get ordered out to change.

Copy link
Member Author

@compiler-errors compiler-errors Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The differing variance_info causes this error message to change slightly in the note, as seen above. I think it's harmless imo. Same with all these other errors changing slightly.

@cjgillot
Copy link
Contributor

Well, if it's just an ordering issue.
@bors r+

@bors
Copy link
Contributor

bors commented Oct 27, 2022

📌 Commit 4e0c27b has been approved by cjgillot

It is now in the queue for this repository.

@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 Oct 27, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 28, 2022
…gillot

Don't carry MIR location in `ConstraintCategory::CallArgument`

It turns out that `ConstraintCategory::CallArgument` cannot just carry a MIR location in it, since we may bubble them up to totally different MIR bodies.

So instead, revert the commit a6b5f95, and instead just erase regions from the original `Option<Ty<'tcx>>` that it carried, so that it doesn't ICE with the changes in rust-lang#103220.

Best reviewed in parts -- the first is just a revert, and the second is where the meaningful changes happen.

Fixes rust-lang#103624
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 28, 2022
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#102642 (Add tests for static async functions in traits)
 - rust-lang#103283 (Add suggestions for unsafe impl error codes)
 - rust-lang#103523 (Fix unwanted merge of inline doc comments for impl blocks)
 - rust-lang#103550 (diagnostics: do not suggest static candidates as traits to import)
 - rust-lang#103641 (Don't carry MIR location in `ConstraintCategory::CallArgument`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 84663ce into rust-lang:master Oct 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 28, 2022
@compiler-errors compiler-errors deleted the issue-103624 branch November 2, 2022 02:52
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
Development

Successfully merging this pull request may close these issues.

Compiler error
5 participants