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

Consistently use subtyping in method resolution #126128

Merged
merged 2 commits into from
Jun 17, 2024
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 7, 2024

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
Copy link
Collaborator

rustbot commented Jun 7, 2024

changes to the core type system

cc @compiler-errors, @lcnr

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

compiler-errors commented Jun 7, 2024

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
Copy link
Member

compiler-errors commented Jun 7, 2024

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
Copy link
Contributor Author

oli-obk commented Jun 7, 2024

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

done

@compiler-errors
Copy link
Member

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

@rustbot rustbot assigned compiler-errors and unassigned lcnr Jun 7, 2024
@oli-obk 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
}
res
Ok(variances)
Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

@compiler-errors compiler-errors left a comment

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(|_| ty::Invariant)),
Copy link
Member

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

Copy link
Member

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)
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

nvm this is irrelevant

}
res
Ok(variances)
Copy link
Member

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()?;
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Contributor

lcnr commented Jun 10, 2024

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
Copy link
Contributor Author

oli-obk commented Jun 10, 2024

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.

//[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)
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor

lcnr commented Jun 10, 2024

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
Copy link
Contributor Author

oli-obk commented Jun 10, 2024

#122317 (fully) switched from eq to subtyping

@lcnr
Copy link
Contributor

lcnr commented Jun 10, 2024

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125722 (Indicate in `non_local_defs` lint that the macro needs to change)
 - rust-lang#125829 (rustc_span: Add conveniences for working with span formats)
 - rust-lang#126128 (Consistently use subtyping in method resolution)
 - rust-lang#126192 (Various Redox OS fixes and add i686 Redox OS target)
 - rust-lang#126352 (ci: Update centos:7 to use vault repos)
 - rust-lang#126354 (Use `Variance` glob imported variants everywhere)
 - rust-lang#126469 (MIR Shl/Shr: the offset can be computed with rem_euclid)
 - rust-lang#126472 (build `libcxx-version` only when it doesn't exist)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r-
#126480 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 14, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 17, 2024

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Jun 17, 2024

📌 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
Copy link
Contributor

bors commented Jun 17, 2024

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

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 17, 2024
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
Copy link
Contributor

bors commented Jun 17, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 17, 2024
@bors bors merged commit 9b584a6 into rust-lang:master Jun 17, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 17, 2024
@rust-timer
Copy link
Collaborator

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

@oli-obk oli-obk deleted the method_ice branch June 17, 2024 16:49
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
…=<try>

Relate receiver invariantly in method probe for `Mode::Path`

Reverts rust-lang#126128
Fixes the part of rust-lang#122317 which always used subtyping instead of eq
Fixes rust-lang#126227 (TODO: test)

WIP: description

r? lcnr
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Sep 11, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [rust](https://github.com/rust-lang/rust) | minor | `1.80.1` -> `1.81.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>rust-lang/rust (rust)</summary>

### [`v1.81.0`](https://github.com/rust-lang/rust/blob/HEAD/RELEASES.md#Version-1810-2024-09-05)

[Compare Source](rust-lang/rust@1.80.1...1.81.0)

\==========================

<a id="1.81.0-Language"></a>

## Language

-   [Abort on uncaught panics in `extern "C"` functions.](rust-lang/rust#116088)
-   [Fix ambiguous cases of multiple `&` in elided self lifetimes.](rust-lang/rust#117967)
-   [Stabilize `#[expect]` for lints (RFC 2383),](rust-lang/rust#120924) like `#[allow]` with a warning if the lint is *not* fulfilled.
-   [Change method resolution to constrain hidden types instead of rejecting method candidates.](rust-lang/rust#123962)
-   [Bump `elided_lifetimes_in_associated_constant` to deny.](rust-lang/rust#124211)
-   [`offset_from`: always allow pointers to point to the same address.](rust-lang/rust#124921)
-   [Allow constraining opaque types during subtyping in the trait system.](rust-lang/rust#125447)
-   [Allow constraining opaque types during various unsizing casts.](rust-lang/rust#125610)
-   [Deny keyword lifetimes pre-expansion.](rust-lang/rust#126762)

<a id="1.81.0-Compiler"></a>

## Compiler

-   [Make casts of pointers to trait objects stricter.](rust-lang/rust#120248)
-   [Check alias args for well-formedness even if they have escaping bound vars.](rust-lang/rust#123737)
-   [Deprecate no-op codegen option `-Cinline-threshold=...`.](rust-lang/rust#124712)
-   [Re-implement a type-size based limit.](rust-lang/rust#125507)
-   [Properly account for alignment in `transmute` size checks.](rust-lang/rust#125740)
-   [Remove the `box_pointers` lint.](rust-lang/rust#126018)
-   [Ensure the interpreter checks bool/char for validity when they are used in a cast.](rust-lang/rust#126265)
-   [Improve coverage instrumentation for functions containing nested items.](rust-lang/rust#127199)
-   Target changes:
    -   [Add Tier 3 `no_std` Xtensa targets:](rust-lang/rust#125141) `xtensa-esp32-none-elf`, `xtensa-esp32s2-none-elf`, `xtensa-esp32s3-none-elf`
    -   [Add Tier 3 `std` Xtensa targets:](rust-lang/rust#126380) `xtensa-esp32-espidf`, `xtensa-esp32s2-espidf`, `xtensa-esp32s3-espidf`
    -   [Add Tier 3 i686 Redox OS target:](rust-lang/rust#126192) `i686-unknown-redox`
    -   [Promote `arm64ec-pc-windows-msvc` to Tier 2.](rust-lang/rust#126039)
    -   [Promote `loongarch64-unknown-linux-musl` to Tier 2 with host tools.](rust-lang/rust#126298)
    -   [Enable full tools and profiler for LoongArch Linux targets.](rust-lang/rust#127078)
    -   [Unconditionally warn on usage of `wasm32-wasi`.](rust-lang/rust#126662) (see compatibility note below)
    -   Refer to Rust's \[platform support page]\[platform-support-doc] for more information on Rust's tiered platform support.

<a id="1.81.0-Libraries"></a>

## Libraries

-   [Split core's `PanicInfo` and std's `PanicInfo`.](rust-lang/rust#115974) (see compatibility note below)
-   [Generalize `{Rc,Arc}::make_mut()` to unsized types.](rust-lang/rust#116113)
-   [Replace sort implementations with stable `driftsort` and unstable `ipnsort`.](rust-lang/rust#124032) All `slice::sort*` and `slice::select_nth*` methods are expected to see significant performance improvements. See the [research project](https://github.com/Voultapher/sort-research-rs) for more details.
-   [Document behavior of `create_dir_all` with respect to empty paths.](rust-lang/rust#125112)
-   [Fix interleaved output in the default panic hook when multiple threads panic simultaneously.](rust-lang/rust#127397)

<a id="1.81.0-Stabilized-APIs"></a>

## Stabilized APIs

-   [`core::error`](https://doc.rust-lang.org/stable/core/error/index.html)
-   [`hint::assert_unchecked`](https://doc.rust-lang.org/stable/core/hint/fn.assert_unchecked.html)
-   [`fs::exists`](https://doc.rust-lang.org/stable/std/fs/fn.exists.html)
-   [`AtomicBool::fetch_not`](https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicBool.html#method.fetch_not)
-   [`Duration::abs_diff`](https://doc.rust-lang.org/stable/core/time/struct.Duration.html#method.abs_diff)
-   [`IoSlice::advance`](https://doc.rust-lang.org/stable/std/io/struct.IoSlice.html#method.advance)
-   [`IoSlice::advance_slices`](https://doc.rust-lang.org/stable/std/io/struct.IoSlice.html#method.advance_slices)
-   [`IoSliceMut::advance`](https://doc.rust-lang.org/stable/std/io/struct.IoSliceMut.html#method.advance)
-   [`IoSliceMut::advance_slices`](https://doc.rust-lang.org/stable/std/io/struct.IoSliceMut.html#method.advance_slices)
-   [`PanicHookInfo`](https://doc.rust-lang.org/stable/std/panic/struct.PanicHookInfo.html)
-   [`PanicInfo::message`](https://doc.rust-lang.org/stable/core/panic/struct.PanicInfo.html#method.message)
-   [`PanicMessage`](https://doc.rust-lang.org/stable/core/panic/struct.PanicMessage.html)

These APIs are now stable in const contexts:

-   [`char::from_u32_unchecked`](https://doc.rust-lang.org/stable/core/char/fn.from_u32\_unchecked.html) (function)
-   [`char::from_u32_unchecked`](https://doc.rust-lang.org/stable/core/primitive.char.html#method.from_u32\_unchecked) (method)
-   [`CStr::count_bytes`](https://doc.rust-lang.org/stable/core/ffi/c_str/struct.CStr.html#method.count_bytes)
-   [`CStr::from_ptr`](https://doc.rust-lang.org/stable/core/ffi/c_str/struct.CStr.html#method.from_ptr)

<a id="1.81.0-Cargo"></a>

## Cargo

-   [Generated `.cargo_vcs_info.json` is always included, even when `--allow-dirty` is passed.](rust-lang/cargo#13960)
-   [Disallow `package.license-file` and `package.readme` pointing to non-existent files during packaging.](rust-lang/cargo#13921)
-   [Disallow passing `--release`/`--debug` flag along with the `--profile` flag.](rust-lang/cargo#13971)
-   [Remove `lib.plugin` key support in `Cargo.toml`. Rust plugin support has been deprecated for four years and was removed in 1.75.0.](rust-lang/cargo#13902)

<a id="1.81.0-Compatibility-Notes"></a>

## Compatibility Notes

-   Usage of the `wasm32-wasi` target will now issue a compiler warning and request users switch to the `wasm32-wasip1` target instead. Both targets are the same, `wasm32-wasi` is only being renamed, and this [change to the WASI target](https://blog.rust-lang.org/2024/04/09/updates-to-rusts-wasi-targets.html) is being done to enable removing `wasm32-wasi` in January 2025.

-   We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0.

    `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*.

    The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.

-   The new sort implementations may panic if a type's implementation of [`Ord`](https://doc.rust-lang.org/std/cmp/trait.Ord.html) (or the given comparison function) does not implement a [total order](https://en.wikipedia.org/wiki/Total_order) as the trait requires. `Ord`'s supertraits (`PartialOrd`, `Eq`, and `PartialEq`) must also be consistent. The previous implementations would not "notice" any problem, but the new implementations have a good chance of detecting inconsistencies, throwing a panic rather than returning knowingly unsorted data.

-   [In very rare cases, a change in the internal evaluation order of the trait
    solver may result in new fatal overflow errors.](rust-lang/rust#126128)

<a id="1.81.0-Internal-Changes"></a>

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

-   [Add a Rust-for Linux `auto` CI job to check kernel builds.](rust-lang/rust#125209)

</details>

---

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

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Sep 17, 2024
… 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
…=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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: instantiate_value_path: (UFCS) Fail<T/#0> was a subtype of Fail<i32> but now is not?
8 participants