Matthew's work on improving NLL's "higher-ranked subtype error"s #86700

lqd · 2021-06-28T19:55:12Z

This PR rebases @matthewjasper's branch which has great work to fix the obscure higher-ranked subtype errors that are tracked in #57374.

These are a blocker to turning full NLLs on, and doing some internal cleanups to remove some of the old region code.

The goal is so @nikomatsakis can take a look at this early, and I'll then do my best to help do the changes and followup work to land this work, and move closer to turning off the migration mode.

I've only updated the branch and made it compile, removed a warning or two.

r? @nikomatsakis

(Here's the zulip topic to discuss this that Niko wanted)

estebank

I'm still looking at the two big "adding" files, but otherwise it looks great so far! Only a couple of nitpicks that I noticed, but very little that would need addressing on this PR.

estebank · 2021-07-06T18:33:36Z

src/test/ui/associated-types/higher-ranked-projection.bad.nll.stderr

+   |     ^^^^^^^ one type is more general than the other
+   |
+   = note:   expected type `&'a ()`
+           found reference `&()`


It would be nice if we could reach parity with the non-nll output:

error[E0308]: mismatched types --> $DIR/higher-ranked-projection.rs:25:5 | LL | foo(()); | ^^^ lifetime mismatch | = note: expected type `&'a ()` found type `&()` note: the lifetime requirement is introduced here --> $DIR/higher-ranked-projection.rs:15:33 | LL | where for<'a> &'a T: Mirror<Image=U> | ^^^^^^^

I have some good news and less good news here. ~~The good news is, this PR uses basically the same piece of code that emits the error above, the "nice region errors"~~ (I take that back the error above is not a nice region error 😅 but some of the others in the blessed test expectations are), so it's at least not impossible to reach parity. I believe this was purposeful and one of Matthew's goals (I'm only the slightly-informed messenger here).

The "bad" news is the data: I don't think I have access to the actual same ObligationCauses in this case (or couldn't find them when I looked) so if you or @nikomatsakis (I know he won't see this notification, it's more of a message for when/if he also reviews this PR) can point me in the right direction to add them, I'll be glad to work on this more.

To be more specific, the migrate-mode error note above is emitted by report_placeholder_failure which specifically looks for a BindingObligation cause.

With this PR, this is an UpperBoundUniverseConflict but with types value pairs, so the nice region error is not actually emitted as these placeholder conflicts are only about placeholder and traits mismatches. The obligation available is however a MiscObligation here. (I believe it's this dummy one).

Funnily enough, report_placeholder_failure would emit what this PR emits, in such fallback cases.

Hmm. I'm not sure off hand how to get that obligation cause, it's probably going to take a bit of threading.

Understood, thank you. I will look into it when I can.

Do you and @estebank agree that we could leave this topic for follow-up improvement PRs ? (Esteban has mentioned this on other suggestions, and I presume it would be ok for this suggestion as well)

As long as we don't turn NLL on without addressing this, I'm ok with it.

estebank · 2021-07-06T18:35:39Z

src/test/ui/higher-rank-trait-bounds/issue-59311.stderr

  --> $DIR/issue-59311.rs:17:9
   |
 LL |     v.t(|| {});
   |         ^^^^^
+   |
+   = note: could not prove for<'a> &'a V: 'static


Ideally we would point at the where clause as well. (not for this PR)

This should not actually be an error - see #89409. However, it occurs both with and without #![feature(nll)], so I don't think we need to address this before turning on NLL mode.

estebank · 2021-07-06T18:37:13Z

src/test/ui/hrtb/hrtb-conflate-regions.nll.stderr

+   = note: `SomeStruct` must implement `Foo<(&'0 isize, &'1 isize)>`, for any two lifetimes `'0` and `'1`...
+   = note: ...but it actually implements `Foo<(&'2 isize, &'2 isize)>`, for some specific lifetime `'2`


Ideally we would point at the impl and the where clause in want_foo2.

In both NLL and non-nll mode, we don't point at the impl or where clause. I think this could be an separate diagnostics issue, independent of the NLL transition.

compiler/rustc_mir/src/borrow_check/region_infer/values.rs

estebank · 2021-07-06T18:46:24Z

compiler/rustc_mir/src/borrow_check/type_check/relate_tys.rs

    fn next_placeholder_region(&mut self, placeholder: ty::PlaceholderRegion) -> ty::Region<'tcx> {
-        if let Some(borrowck_context) = &mut self.borrowck_context {
-            borrowck_context.constraints.placeholder_region(self.infcx, placeholder)
-        } else {
-            self.infcx.tcx.lifetimes.re_erased
-        }
+        self.borrowck_context.constraints.placeholder_region(self.infcx, placeholder)
    }


This change doesn't break anything?

I don't expect it to, this Option was only used with a Some, but it's easier to see in the individual commit

compiler/rustc_trait_selection/src/traits/query/type_op/implied_outlives_bounds.rs

nikomatsakis

OK, I reviewed the PR. I understand what it's doing. I'm mildly worried about the lack of DRY, though, I'd like it if it the query impl and the code to "re-run the query and report errors" were combined more clearly-- or at least in the same file or something!

I'm imagining it's probably possible to have some kind of helper function that both of them can call?

nikomatsakis · 2021-07-30T12:44:31Z

compiler/rustc_mir/src/borrow_check/diagnostics/bound_region_errors.rs

+        placeholder_region: ty::Region<'tcx>,
+        error_region: Option<ty::Region<'tcx>>,
+    ) -> Option<DiagnosticBuilder<'tcx>> {
+        tcx.infer_ctxt().enter_with_canonical(span, &self.canonical_query, |ref infcx, key, _| {


Hmm, this seems awfully "non-DRY", right? It'd be nice if this shared some of the same code as the query itself..?

On the one hand, I agree the duplication is to be avoided, so I added a helper function to share most of the query code for type_op_prove_predicate. Let me know if that's more to your liking.

On the other hand, these type op queries are super small and have not changed in the last 3 years, and since the normalize step to get the obligations in the diagnostics is different from the query's, I have not done this for the other type op type_op::Normalize: it would only de-dupe a single register_predicate_obligations call.

I think the functional difference between the two is the query fails to normalize this (but maybe it's some subtle interaction with the leak-check there, or maybe the difference would be erased by some of the fixes in #85499, I don't know).

I can add a FIXME to at least track this in the list of remaining tasks to be done to turn full NLLs on, if that's OK with you ?

Yes, that's ok

nikomatsakis · 2021-07-30T12:45:51Z

src/test/ui/associated-types/higher-ranked-projection.bad.nll.stderr

+   |     ^^^^^^^ one type is more general than the other
+   |
+   = note:   expected type `&'a ()`
+           found reference `&()`


Hmm. I'm not sure off hand how to get that obligation cause, it's probably going to take a bit of threading.

matthewjasper

Thanks for pushing this forwards.

matthewjasper · 2021-08-15T23:27:15Z

compiler/rustc_mir/src/borrow_check/diagnostics/bound_region_errors.rs

+{
+    fn to_universe_info(self, _base_universe: ty::UniverseIndex) -> UniverseInfo<'tcx> {
+        // Ascribe user type isn't usually called on types that have different
+        // bound regions.


Note for a future PR: I believe there is one test case that is affected by this

Maybe it was in one of the last few remaining instances of the old "higher-ranked subtype" errors involving type ascription ?

hrtb/due-to-where-clause.rs

hrtb-cache-issue-54302.rs

hrtb-just-for-static.rs

issue-54302.rs

After checking this, it's all of those.

compiler/rustc_mir/src/borrow_check/diagnostics/bound_region_errors.rs

Co-authored-by: matthewjasper <20113453+matthewjasper@users.noreply.github.com>

nikomatsakis · 2021-08-17T23:49:18Z

@bors r+

bors · 2021-08-17T23:49:19Z

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove [WIP] from this PR's title when it is ready for review.

nikomatsakis · 2021-08-17T23:49:37Z

r=me @lqd when not WIP :)

lqd · 2021-08-18T08:24:09Z

WIP removed, FIXME added. I'll trigger bors once CI passes.

lqd · 2021-08-18T09:11:02Z

@bors r=nikomatsakis

I'll update the tracking issue to mention the actual state of the errors, and the known work remaining to be done (matching the non-NLL output, cleaning up the normalize type op query, improve errors for the type ascription type op query as Matthew mentioned)

bors · 2021-08-18T09:11:03Z

📌 Commit 8343806 has been approved by nikomatsakis

bors · 2021-08-18T15:55:08Z

⌛ Testing commit 8343806 with merge 3d0774d...

bors · 2021-08-18T18:39:31Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 3d0774d to master...

…sakis Handle type ascription type ops in NLL HRTB diagnostics Currently, there are still a few cases of the "higher-ranked subtype error" of yore, 4 of which are related to type ascription. This PR is a follow-up to rust-lang#86700, adding support for type ascription type ops, and makes 3 of these tests output the same diagnostics in NLL mode as the migrate mode (and 1 is now much closer, especially if you ignore that it already outputs an additional error in NLL mode -- which could be a duplicate caused by a lack of normalization like [these comments point out](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/compiler/rustc_traits/src/type_op.rs#L122-L157), or an imprecision in some parts of normalization as [described here](rust-lang#86700 (comment))). Since we discussed these recently: - [here](rust-lang#86700 (comment)), cc `@matthewjasper,` - and [here](rust-lang#57374 (comment)), cc `@Aaron1011.` It should only leave [this TAIT test](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.rs) as still emitting [the terse error](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.nll.stderr). r? `@estebank` (so that they shake their fist at NLL's general direction less often) or `@nikomatsakis` or matthew or aaron, the more the merrier.

…sakis Handle type ascription type ops in NLL HRTB diagnostics Currently, there are still a few cases of the "higher-ranked subtype error" of yore, 4 of which are related to type ascription. This PR is a follow-up to rust-lang#86700, adding support for type ascription type ops, and makes 3 of these tests output the same diagnostics in NLL mode as the migrate mode (and 1 is now much closer, especially if you ignore that it already outputs an additional error in NLL mode -- which could be a duplicate caused by a lack of normalization like [these comments point out](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/compiler/rustc_traits/src/type_op.rs#L122-L157), or an imprecision in some parts of normalization as [described here](rust-lang#86700 (comment))). Since we discussed these recently: - [here](rust-lang#86700 (comment)), cc ``@matthewjasper,`` - and [here](rust-lang#57374 (comment)), cc ``@Aaron1011.`` It should only leave [this TAIT test](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.rs) as still emitting [the terse error](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.nll.stderr). r? ``@estebank`` (so that they shake their fist at NLL's general direction less often) or ``@nikomatsakis`` or matthew or aaron, the more the merrier.

…sakis Handle type ascription type ops in NLL HRTB diagnostics Currently, there are still a few cases of the "higher-ranked subtype error" of yore, 4 of which are related to type ascription. This PR is a follow-up to rust-lang#86700, adding support for type ascription type ops, and makes 3 of these tests output the same diagnostics in NLL mode as the migrate mode (and 1 is now much closer, especially if you ignore that it already outputs an additional error in NLL mode -- which could be a duplicate caused by a lack of normalization like [these comments point out](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/compiler/rustc_traits/src/type_op.rs#L122-L157), or an imprecision in some parts of normalization as [described here](rust-lang#86700 (comment))). Since we discussed these recently: - [here](rust-lang#86700 (comment)), cc ```@matthewjasper,``` - and [here](rust-lang#57374 (comment)), cc ```@Aaron1011.``` It should only leave [this TAIT test](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.rs) as still emitting [the terse error](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.nll.stderr). r? ```@estebank``` (so that they shake their fist at NLL's general direction less often) or ```@nikomatsakis``` or matthew or aaron, the more the merrier.

…sakis Handle type ascription type ops in NLL HRTB diagnostics Currently, there are still a few cases of the "higher-ranked subtype error" of yore, 4 of which are related to type ascription. This PR is a follow-up to rust-lang#86700, adding support for type ascription type ops, and makes 3 of these tests output the same diagnostics in NLL mode as the migrate mode (and 1 is now much closer, especially if you ignore that it already outputs an additional error in NLL mode -- which could be a duplicate caused by a lack of normalization like [these comments point out](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/compiler/rustc_traits/src/type_op.rs#L122-L157), or an imprecision in some parts of normalization as [described here](rust-lang#86700 (comment))). Since we discussed these recently: - [here](rust-lang#86700 (comment)), cc ````@matthewjasper,```` - and [here](rust-lang#57374 (comment)), cc ````@Aaron1011.```` It should only leave [this TAIT test](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.rs) as still emitting [the terse error](https://github.com/rust-lang/rust/blob/9583fd1bdd0127328e25e5b8c24dff575ec2c86b/src/test/ui/type-alias-impl-trait/issue-57611-trait-alias.nll.stderr). r? ````@estebank```` (so that they shake their fist at NLL's general direction less often) or ````@nikomatsakis```` or matthew or aaron, the more the merrier.

rust-highfive assigned nikomatsakis Jun 28, 2021

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

lqd mentioned this pull request Jun 29, 2021

ICE: OutputTypeParameterMismatch error under -Clink-dead-code #86703

Closed

estebank reviewed Jul 6, 2021

View reviewed changes

lqd force-pushed the matthews-nll-hrtb-errors branch from 056e631 to eaa934b Compare July 12, 2021 23:05

oli-obk mentioned this pull request Jul 20, 2021

GATs and min_type_alias_impl_trait cause ICE #87258

Closed

nikomatsakis requested changes Jul 30, 2021

View reviewed changes

bjorn3 mentioned this pull request Aug 5, 2021

Alloc features cleanup #87769

Merged

This comment has been minimized.

Sign in to view

matthewjasper and others added 11 commits August 15, 2021 08:44

Remove unused Option from NllTypeRelatingDelegate

5d52c0e

Show bound lifetimes when comparing types in diagnostics

842f059

Return the canonicalized query from type ops

4421dd1

Simplify BoundUniversalRegionError

e271383

Track causes for universes created during borrowck

5e6027c

Report mismatched type errors for bound region errors in NLL

0c388b0

Report nicer errors for HRTB NLL errors from queries

d563a63

Fix dyn trait warning

5cf6edb

Slight cleanup

19dfea5

don't derive Copy for RegionElement

a69c7cc

De-dupe NLL HRTB diagnostics' use of type_op_prove_predicate

2cf4b87

lqd force-pushed the matthews-nll-hrtb-errors branch from eaa934b to 2cf4b87 Compare August 15, 2021 12:50

matthewjasper reviewed Aug 15, 2021

View reviewed changes

fix typo in bound_region_errors.rs

311578d

Co-authored-by: matthewjasper <20113453+matthewjasper@users.noreply.github.com>

lqd changed the title ~~[WIP] Matthew's work on improving NLL's "higher-ranked subtype error"s~~ Matthew's work on improving NLL's "higher-ranked subtype error"s Aug 18, 2021

add fixme about the type_op_normalize query in NLL HRTB diagnostics

8343806

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 Aug 18, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 18, 2021

bors merged commit 3d0774d into rust-lang:master Aug 18, 2021

rustbot added this to the 1.56.0 milestone Aug 18, 2021

lqd deleted the matthews-nll-hrtb-errors branch August 18, 2021 18:48

Aaron1011 mentioned this pull request Aug 18, 2021

[NLL] Bad higher ranked subtype error #57374

Open

lqd mentioned this pull request Aug 23, 2021

Handle type ascription type ops in NLL HRTB diagnostics #88270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matthew's work on improving NLL's "higher-ranked subtype error"s #86700

Matthew's work on improving NLL's "higher-ranked subtype error"s #86700

lqd commented Jun 28, 2021 •

edited

Loading

estebank left a comment

estebank Jul 6, 2021

lqd Jul 12, 2021 •

edited

Loading

nikomatsakis Jul 30, 2021

lqd Aug 15, 2021

estebank Aug 18, 2021

estebank Jul 6, 2021

Aaron1011 Sep 30, 2021

estebank Jul 6, 2021

Aaron1011 Sep 30, 2021

estebank Jul 6, 2021

lqd Jul 12, 2021 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Jul 30, 2021

lqd Aug 15, 2021 •

edited

Loading

nikomatsakis Aug 17, 2021

nikomatsakis Jul 30, 2021

This comment has been minimized.

matthewjasper left a comment

matthewjasper Aug 15, 2021

lqd Aug 16, 2021

matthewjasper Aug 22, 2021

nikomatsakis commented Aug 17, 2021

bors commented Aug 17, 2021

nikomatsakis commented Aug 17, 2021

lqd commented Aug 18, 2021

lqd commented Aug 18, 2021 •

edited

Loading

bors commented Aug 18, 2021

bors commented Aug 18, 2021

bors commented Aug 18, 2021

		= note: `SomeStruct` must implement `Foo<(&'0 isize, &'1 isize)>`, for any two lifetimes `'0` and `'1`...
		= note: ...but it actually implements `Foo<(&'2 isize, &'2 isize)>`, for some specific lifetime `'2`

Matthew's work on improving NLL's "higher-ranked subtype error"s #86700

Matthew's work on improving NLL's "higher-ranked subtype error"s #86700

Conversation

lqd commented Jun 28, 2021 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqd Jul 12, 2021 • 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

lqd Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqd Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

matthewjasper left a 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

nikomatsakis commented Aug 17, 2021

bors commented Aug 17, 2021

nikomatsakis commented Aug 17, 2021

lqd commented Aug 18, 2021

lqd commented Aug 18, 2021 • edited Loading

bors commented Aug 18, 2021

bors commented Aug 18, 2021

bors commented Aug 18, 2021

lqd commented Jun 28, 2021 •

edited

Loading

lqd Jul 12, 2021 •

edited

Loading

lqd Jul 12, 2021 •

edited

Loading

lqd Aug 15, 2021 •

edited

Loading

lqd commented Aug 18, 2021 •

edited

Loading