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

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation43 Commits2 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

oli-obk

fixes #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

@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

Jun 7, 2024

@rustbot

@rust-log-analyzer

This comment has been minimized.

compiler-errors

@compiler-errors

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

More thoughts:

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

... 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

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

done

@compiler-errors

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

@oli-obk oli-obk changed the titleAdd 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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

edit: nvm I guess lol

Choose a reason for hiding this comment

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

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

compiler-errors

Choose a reason for hiding this comment

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

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

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(|_

Choose a reason for hiding this comment

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

This branch is unreachable rn

Choose a reason for hiding this comment

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

nvm

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

nvm this is irrelevant

Choose a reason for hiding this comment

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

edit: nvm I guess lol

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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 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

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

lcnr

//[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)

Choose a reason for hiding this comment

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

why this change?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

@lcnr

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

#122317 (fully) switched from eq to subtyping

@lcnr

oh well, that's ugly 🤔

struct B(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

@lcnr lcnr mentioned this pull request

Jun 10, 2024

@bors bors added the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Jun 14, 2024

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk

@oli-obk

@oli-obk

@bors

📌 Commit 3e6e6b1 has been approved by lcnr

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jun 17, 2024

@bors

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

Jun 17, 2024

@GuillaumeGomez

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

@rust-timer

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%)

This was referenced

Aug 9, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 14, 2024

@bors

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Sep 11, 2024

@tmeijn

This MR contains the following updates:

Package Update Change
rust minor 1.80.1 -> 1.81.0

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.81.0

Compare Source

==========================

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request

Sep 17, 2024

@fee1-dead

… r=lcnr

Relate receiver invariantly in method probe for Mode::Path

Effectively reverts part of rust-lang#126128 Fixes rust-lang#126227

This PR changes method probing to use equality for fully path-based method lookup, and subtyping for receiver . method lookup.

r? lcnr

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 17, 2024

@bors

…=lcnr

Relate receiver invariantly in method probe for Mode::Path

Effectively reverts part of rust-lang#126128 Fixes rust-lang#126227

This PR changes method probing to use equality for fully path-based method lookup, and subtyping for receiver . method lookup.

r? lcnr

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Sep 22, 2024

@he32

Pkgsrc changes:

Upstream changes:

Version 1.81.0 (2024-09-05)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Nov 24, 2024

@he32

Pkgsrc changes:

Upstream changes:

Version 1.81.0 (2024-09-05)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Labels

merged-by-bors

This PR was explicitly merged by bors.

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.