Consistently use subtyping in method resolution by oli-obk · Pull Request #126128 · rust-lang/rust

oli-obk · 2024-06-07T15:17:21Z

An earlier version of this PR modified how we compute variance, but the root cause was an inconsistency between the usage of eq and sub, where we assumed that the latter passing implies the former will pass.

r? @compiler-errors

rustbot · 2024-06-07T15:17:31Z

changes to the core type system

cc @compiler-errors, @lcnr

compiler-errors · 2024-06-07T15:44:59Z

I don't personally feel comfortable about adding a new error variance. Seems really strange imo.

I think we could easily just force the error variance to Invariant rather than Bivariant, which would prevent these ICEs at risk of slightly more awkward errors, but at the benefit of not having it be its own thing that actually need to be handled in relations.

r? lcnr

compiler-errors · 2024-06-07T15:48:05Z

More thoughts:

So the underlying issue is the inconsistency we have with the xform implementation, where:

invariant xform bivariant = invariant
covariant xform bivariant = bivariant

... and the fact we're using equality during method lookup and subtyping during confirmation, leading to a mismatch in the result of those relations.

This seems like a kind of roundabout way of fixing this, since the real underlying issue is that the way we handle bivariance in the compiler is just kind of awkward.

Not to repeat myself, but I think forcing invariance would just fix things in a simpler way.

oli-obk · 2024-06-07T16:01:57Z

Not to repeat myself, but I think forcing invariance would just fix things in a simpler way.

done

compiler-errors · 2024-06-07T16:10:48Z

OK -- will review this then. Thanks for considering my feedback.

If lcnr strongly disagrees about forcing invariance, then it shouldn't be hard to do a follow-up and actually introduce the error variance later.

r? compiler-errors

compiler-errors · 2024-06-07T17:59:38Z

+        variances = tcx.arena.alloc_slice(&v);
    }
-    res
+    Ok(variances)


Does this function ever return Err?

Unless I'm being particularly unobservant, maybe let's rename this fn to check_variances_for_type_defn_and_set_unconstrained_to_invariant

(long name but there's only a few callsites -- happy to workshop the name lol

edit: nvm I guess lol

I could return a vec of erroneous indices instead of doing the modification and the checks. Seems cleaner to separate these things

compiler-errors

some random thoughts -- pls forgive me for commenting on things that were modified subsequently in a follow-up commit lol

compiler-errors · 2024-06-07T18:00:03Z

-    variances
+    match crate::check::wfcheck::check_variances_for_type_defn(tcx, item_def_id, variances) {
+        Ok(variances) => variances,
+        Err(_) => tcx.arena.alloc_from_iter(variances.iter().map(|_| ty::Invariant)),


This branch is unreachable rn

compiler-errors · 2024-06-07T18:01:01Z

-            check_variances_for_type_defn(tcx, item, hir_generics);
-            res
-        }
+        hir::ItemKind::Struct(_, hir_generics) => check_type_defn(tcx, item, false)


can we use ? like:

check_type_defn()?; check_variances_for_type_defn()?;

then at the bottom we can Ok(())?

Seems to simplify control flow a bit

nvm this is irrelevant

compiler-errors · 2024-06-07T18:01:34Z

+        variances = tcx.arena.alloc_slice(&v);
    }
-    res
+    Ok(variances)


edit: nvm I guess lol

compiler-errors · 2024-06-07T18:04:00Z

-                if field.ty(tcx, identity_args).references_error() {
-                    return;
-                }
+                field.ty(tcx, identity_args).error_reported()?;


I wonder if we should just use error_reported to delay the error message below but don't early-return (using emit_unless). I kinda feel bad that we're setting all of the substs to bivariant even if they are constrained otherwise (e.g. to covariance) somewhere else in the definition.

Yea, these early aborts were preexisting. I didn't want to touch them in the previously-big PR (I'm assuming they got added to avoid ICEs), but now it's reasonable to investigate.

lcnr · 2024-06-10T08:11:58Z

More thoughts:

So the underlying issue is the inconsistency we have with the xform implementation, where:
* invariant xform bivariant = invariant

* covariant xform bivariant = bivariant
... and the fact we're using equality during method lookup and subtyping during confirmation, leading to a mismatch in the result of those relations.

We get subtyping to succeed and equate to fail and that causes an ICE because we expect "subtype implies equal"? The underlying issue is just that, isn't it? We have types which are subtypes of each other but not equal. Apart from variance this is also the case with the leak check. This can't currently trigger as we just put the nested obligations in the FnCtxt without trying them, even in the new solver, but if we were to eagerly compute nested obligations, the following test would ICE https://rust.godbolt.org/z/KxGeqevaW.

I also don't see how this PR fixes the underlying issue with bivariance:

trait Proj {
    type Assoc;
}
impl<T> Proj for T {
    type Assoc = T;
}

struct Fail<T: Proj<Assoc = U>, U>(T);

impl Fail<i32, i32> {
    const C: () = ();
}

fn main() {
    Fail::<i32, u32>::C
    // expected: WF error
    // actual: ICE
}

oli-obk · 2024-06-10T10:25:20Z

We get subtyping to succeed and equate to fail and that causes an ICE because we expect "subtype implies equal"? The underlying issue is just that, isn't it?

yes, but I did not want to touch that behaviour, because I really can't tell the implications. Almost nothing should be affected, because we already generate fresh inference vars during method resolution, so all we're doing is constraining inference vars that have no other constraints.

I did the change (see latest force push), and the only affected test is never type fallback, which checks with my analysis above.

lcnr · 2024-06-10T11:17:32Z

-    //[nofallback]~^ ERROR trait bound `(): std::error::Error` is not satisfied
+    // Subtyping during method resolution will generate new inference vars and
+    // subtype them. Thus fallback will not fall back to `!`, but `()` instead.
+    Box::<_ /* ! */>::new(x)


why this change?

Instead of merging the two infer vars, we now create a subtyping relation for them, so they aren't the same. Never type fallback will still fall back to ! for the one type var, but the other type var which is a subtype, will only fall back to (). I haven't debugged this deeply, since never type fallback is cursed and debugging it will probably yield no new information beyond all the edge cases that already are know for never type fallback.

oh lel, you didn't change the UI test, you just reformatted it

lcnr · 2024-06-10T11:34:03Z

yes, but I did not want to touch that behaviour, because I really can't tell the implications.

I think non-method calls should use eq instead of sub everywhere 🤔 it seems surprising that Type::<Subtype>::CONST works? though i'd like @compiler-errors input on that

oli-obk · 2024-06-10T13:02:33Z

#122317 (fully) switched from eq to subtyping

lcnr · 2024-06-10T13:24:58Z

oh well, that's ugly 🤔

struct B<T>(T);

impl B<fn(&'static ())> {
    fn method(self) {
        println!("hey");
    }
}

fn foo(x: B<for<'a> fn(&'a ())>) {
    x.method();
}

fn main() {
    B::<for<'a> fn(&'a ())>::method(B(|&()| ()));
}

i expected us to only use subtyping when looking up the self type, not when using a path 🤔 that's kinda meh. But yeah, I guess sub is fine here, opening an issue as I would like to try and change that

…subtyping

oli-obk · 2024-06-17T10:58:11Z

@bors r=lcnr

bors · 2024-06-17T10:58:13Z

📌 Commit 3e6e6b1 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-06-17T12:08:20Z

⌛ Testing commit 3e6e6b1 with merge 9b584a6...

Consistently use subtyping in method resolution fixes rust-lang#126062 An earlier version of this PR modified how we compute variance, but the root cause was an inconsistency between the usage of `eq` and `sub`, where we assumed that the latter passing implies the former will pass. r? `@compiler-errors`

bors · 2024-06-17T14:21:03Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 9b584a6 to master...

ghost · 2024-06-17T16:44:46Z

Finished benchmarking commit (9b584a6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary 5.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.0%	[5.0%, 5.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.0%	[5.0%, 5.0%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.445s -> 670.422s (0.15%)
Artifact size: 320.51 MiB -> 319.89 MiB (-0.19%)

rustbot assigned compiler-errors Jun 7, 2024

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 Jun 7, 2024

oli-obk force-pushed the method_ice branch from 4d5f434 to 026183d Compare June 7, 2024 15:32

This comment has been minimized.

Sign in to view

compiler-errors reviewed Jun 7, 2024

View reviewed changes

Comment thread compiler/rustc_type_ir/src/lib.rs Outdated

rustbot assigned lcnr and unassigned compiler-errors Jun 7, 2024

oli-obk force-pushed the method_ice branch from 026183d to b80c1ae Compare June 7, 2024 15:46

oli-obk force-pushed the method_ice branch from b80c1ae to 8a6b4d5 Compare June 7, 2024 16:01

rustbot assigned compiler-errors and unassigned lcnr Jun 7, 2024

oli-obk changed the title ~~Add an error variance and use it to silence follow-up errors~~ set erroneous bivariant generic params to invariant to silence follow-up errors Jun 7, 2024

compiler-errors reviewed Jun 7, 2024

View reviewed changes

oli-obk force-pushed the method_ice branch from 8a6b4d5 to 4aa34df Compare June 10, 2024 10:24

lcnr reviewed Jun 10, 2024

View reviewed changes

Comment thread tests/ui/associated-consts/wrong-projection-self-ty-on-invalid-type2.rs

lcnr reviewed Jun 10, 2024

View reviewed changes

lcnr mentioned this pull request Jun 10, 2024

inherent method selection uses subtyping even when using paths #126227

Closed

oli-obk force-pushed the method_ice branch from 4aa34df to a18d67f Compare June 13, 2024 09:27

This comment has been minimized.

Sign in to view

oli-obk force-pushed the method_ice branch from 9802e15 to 14b6223 Compare June 17, 2024 09:54

This comment has been minimized.

Sign in to view

oli-obk added 2 commits June 17, 2024 10:57

Use subtyping instead of equality, since method resolution also uses …

94f5495

…subtyping

Remove windows-specific copy of test

3e6e6b1

oli-obk force-pushed the method_ice branch from 14b6223 to 3e6e6b1 Compare June 17, 2024 10:58

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2024

GuillaumeGomez mentioned this pull request Jun 17, 2024

Rollup of 3 pull requests #126590

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 17, 2024

bors merged commit 9b584a6 into rust-lang:master Jun 17, 2024

rustbot added this to the 1.81.0 milestone Jun 17, 2024

oli-obk deleted the method_ice branch June 17, 2024 16:49

This was referenced Aug 9, 2024

regression: overflow evaluating the requirement #128887

Closed

Relate receiver invariantly in method probe for Mode::Path #129073

Merged

colinjneville mentioned this pull request Nov 18, 2024

Crate no longer compiles with version 1.81.0 colinjneville/triangulate#4

Closed

Uh oh!

Uh oh!

Conversation

oli-obk commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 7, 2024

Uh oh!

This comment has been minimized.

Uh oh!

compiler-errors commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compiler-errors commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jun 7, 2024

Uh oh!

compiler-errors commented Jun 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jun 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr commented Jun 10, 2024

Uh oh!

oli-obk commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented Jun 10, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jun 17, 2024

Uh oh!

bors commented Jun 17, 2024

Uh oh!

bors commented Jun 17, 2024

Uh oh!

bors commented Jun 17, 2024

Uh oh!

ghost commented Jun 17, 2024

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

oli-obk commented Jun 7, 2024 •

edited

Loading

compiler-errors commented Jun 7, 2024 •

edited

Loading

compiler-errors commented Jun 7, 2024 •

edited

Loading

lcnr commented Jun 10, 2024 •

edited

Loading

oli-obk commented Jun 10, 2024 •

edited

Loading