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

improve normalization of Pointee::Metadata #120354

Merged
merged 4 commits into from Feb 9, 2024

Conversation

lukas-code
Copy link
Contributor

@lukas-code lukas-code commented Jan 25, 2024

This PR makes it so that <Wrapper<Tail> as Pointee>::Metadata is normalized to <Tail as Pointee>::Metadata if we don't know Wrapper<Tail>: Sized. With that, the trait solver can prove projection predicates like <Wrapper<Tail> as Pointee>::Metadata == <Tail as Pointee>::Metadata, which makes it possible to use the metadata APIs to cast between the tail and the wrapper:

#![feature(ptr_metadata)]

use std::ptr::{self, Pointee};

fn cast_same_meta<T: ?Sized, U: ?Sized>(ptr: *const T) -> *const U
where
    T: Pointee<Metadata = <U as Pointee>::Metadata>,
{
    let (thin, meta) = ptr.to_raw_parts();
    ptr::from_raw_parts(thin, meta)
}

struct Wrapper<T: ?Sized>(T);

fn cast_to_wrapper<T: ?Sized>(ptr: *const T) -> *const Wrapper<T> {
    cast_same_meta(ptr)
}

Previously, this failed to compile:

error[E0271]: type mismatch resolving `<Wrapper<T> as Pointee>::Metadata == <T as Pointee>::Metadata`
  --> src/lib.rs:16:5
   |
15 | fn cast_to_wrapper<T: ?Sized>(ptr: *const T) -> *const Wrapper<T> {
   |                    - found this type parameter
16 |     cast_same_meta(ptr)
   |     ^^^^^^^^^^^^^^ expected `Wrapper<T>`, found type parameter `T`
   |
   = note: expected associated type `<Wrapper<T> as Pointee>::Metadata`
              found associated type `<T as Pointee>::Metadata`
   = note: an associated type was expected, but a different one was found

(Yes, you can already do this with as casts. But using functions is so much ✨ safer ✨, because you can't change the metadata on accident.)


This PR essentially changes the built-in impls of Pointee from this:

// before

impl Pointee for u8 {
    type Metadata = ();
}

impl Pointee for [u8] {
    type Metadata = usize;
}

// ...

impl Pointee for Wrapper<u8> {
    type Metadata = ();
}

impl Pointee for Wrapper<[u8]> {
    type Metadata = usize;
}

// ...

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T: ?Sized> Pointee for Wrapper<T>
where
    Wrapper<T>: Sized
{
    type Metadata = ();
}

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T /*: Sized */> Pointee for T {
    type Metadata = ();
}

to this:

// after

impl Pointee for u8 {
    type Metadata = ();
}

impl Pointee for [u8] {
    type Metadata = usize;
}

// ...

impl<T: ?Sized> Pointee for Wrapper<T> {
    // in the old solver this will instead project to the "deep" tail directly,
    // e.g. `Wrapper<Wrapper<T>>::Metadata = T::Metadata`
    type Metadata = <T as Pointee>::Metadata;
}

// ...

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T /*: Sized */> Pointee for T {
    type Metadata = ();
}

@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2024

r? @b-naber

(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 Jan 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@compiler-errors
Copy link
Member

r? types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Jan 25, 2024
@rustbot rustbot assigned compiler-errors and unassigned b-naber Jan 25, 2024
@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2024
@lukas-code
Copy link
Contributor Author

I replaced predicate_must_hold_considering_regions with predicate_must_hold_modulo_regions and added some docs to warn about predicate_must_hold_considering_regions.

r? @lcnr
@rustbot ready

@rustbot rustbot assigned lcnr and unassigned compiler-errors Jan 27, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Jan 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lukas-code
Copy link
Contributor Author

I've updated the OP to show the differences between before and after this PR. I'm still not convinced that the "fallback impl" is better than a specialization, but I guess it doesn't matter too much. 🤷

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 6, 2024
miri: normalize struct tail in ABI compat check

fixes #3282
extracted from rust-lang/rust#120354, see rust-lang/rust#120354 (comment) for context

r? ```@RalfJung```
@lcnr
Copy link
Contributor

lcnr commented Feb 6, 2024

I've updated the OP to show the differences between before and after this PR. I'm still not convinced that the "fallback impl" is better than a specialization, but I guess it doesn't matter too much. 🤷

Neither am I. We already had that impl where we check for Sized for aliases and params. It is pretty much just an AliasBound candidate instead of an actual impl.

Pretty much what happens here is that if there's a Sized bound in the environment, either directly via T: Sized or indirectly from an item bound of T: Trait with type Assoc: Sized, we assume this means Pointee<Metadata = ()>. This is something the caller should prove. The only way you have an alias or an opaque as the self type (at least in the new solver) is if they are rigid as normalizing them relies on the environment.

We hackily reimplemented a super trait bound: trait Sized: Pointee<Metadata = ()> {}. I would like us to remove this manual builtin impl and instead add that super trait bound. That doesn't have to happen inside of this PR. I feel like we may have even considered to do so in the past but wasn't able to find anything skimming through previous PRs.

@lukas-code would you like to change it to a super trait right away or should we leave it to a future PR? If you want to not do so rn, please add a FIXME for it to the "fallback impl" arm in both solvers

@lcnr lcnr 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2024
@lukas-code
Copy link
Contributor Author

Making Pointee<Metadata = ()> a supertrait of Sized seems like a really good idea. I'll implement that and see what breaks 😆 .

@lukas-code
Copy link
Contributor Author

lukas-code commented Feb 6, 2024

To answer the question of what breaks, it's this test: (in both solvers)

// check-pass
trait A { type Assoc; }
impl A for () {
// FIXME: it would be nice for this to at least cause a warning.
type Assoc = Foo<()>;
}
struct Foo<T: A>(T::Assoc);
fn main() {}

But that's probably fine, because that test compiling at all seems more like a side-effect of Sized being coinductive rather than intentional behavior. All other tests that rely on Sized being coinductive still pass.

But there are also a ton of diagnostics regressions, mostly complaining about "type mismatch resolving <Foo as Pointee>::Metadata == ()" in addition to "Foo is not sized". Fixing those seems to be non-trivial, so I'd like to move making Pointee<Metadata = ()> a supertrait of Sized to a future PR.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2024
@lcnr
Copy link
Contributor

lcnr commented Feb 8, 2024

thanks for your good work and for staying with me while I tried to figure this out myself ❤️

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2024

📌 Commit 15ffe83 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120351 (Implement SystemTime for UEFI)
 - rust-lang#120354 (improve normalization of `Pointee::Metadata`)
 - rust-lang#120776 (Move path implementations into `sys`)
 - rust-lang#120790 (better error message on download CI LLVM failure)
 - rust-lang#120806 (Clippy subtree update)
 - rust-lang#120815 (Improve `Option::inspect` docs)
 - rust-lang#120822 (Emit more specific diagnostics when enums fail to cast with `as`)
 - rust-lang#120827 (Print image input file and checksum in CI only)
 - rust-lang#120836 (hide impls if trait bound is proven from env)
 - rust-lang#120844 (Build DebugInfo for async closures)
 - rust-lang#120851 (Remove duplicate release note)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120351 (Implement SystemTime for UEFI)
 - rust-lang#120354 (improve normalization of `Pointee::Metadata`)
 - rust-lang#120776 (Move path implementations into `sys`)
 - rust-lang#120790 (better error message on download CI LLVM failure)
 - rust-lang#120806 (Clippy subtree update)
 - rust-lang#120815 (Improve `Option::inspect` docs)
 - rust-lang#120822 (Emit more specific diagnostics when enums fail to cast with `as`)
 - rust-lang#120827 (Print image input file and checksum in CI only)
 - rust-lang#120836 (hide impls if trait bound is proven from env)
 - rust-lang#120844 (Build DebugInfo for async closures)
 - rust-lang#120851 (Remove duplicate release note)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99bafad into rust-lang:master Feb 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
Rollup merge of rust-lang#120354 - lukas-code:metadata-normalize, r=lcnr

improve normalization of `Pointee::Metadata`

This PR makes it so that `<Wrapper<Tail> as Pointee>::Metadata` is normalized to `<Tail as Pointee>::Metadata` if we don't know `Wrapper<Tail>: Sized`. With that, the trait solver can prove projection predicates like `<Wrapper<Tail> as Pointee>::Metadata == <Tail as Pointee>::Metadata`, which makes it possible to use the metadata APIs to cast between the tail and the wrapper:

```rust
#![feature(ptr_metadata)]

use std::ptr::{self, Pointee};

fn cast_same_meta<T: ?Sized, U: ?Sized>(ptr: *const T) -> *const U
where
    T: Pointee<Metadata = <U as Pointee>::Metadata>,
{
    let (thin, meta) = ptr.to_raw_parts();
    ptr::from_raw_parts(thin, meta)
}

struct Wrapper<T: ?Sized>(T);

fn cast_to_wrapper<T: ?Sized>(ptr: *const T) -> *const Wrapper<T> {
    cast_same_meta(ptr)
}
```

Previously, this failed to compile:

```
error[E0271]: type mismatch resolving `<Wrapper<T> as Pointee>::Metadata == <T as Pointee>::Metadata`
  --> src/lib.rs:16:5
   |
15 | fn cast_to_wrapper<T: ?Sized>(ptr: *const T) -> *const Wrapper<T> {
   |                    - found this type parameter
16 |     cast_same_meta(ptr)
   |     ^^^^^^^^^^^^^^ expected `Wrapper<T>`, found type parameter `T`
   |
   = note: expected associated type `<Wrapper<T> as Pointee>::Metadata`
              found associated type `<T as Pointee>::Metadata`
   = note: an associated type was expected, but a different one was found
```

(Yes, you can already do this with `as` casts. But using functions is so much :sparkles: *safer* :sparkles:, because you can't change the metadata on accident.)

---

This PR essentially changes the built-in impls of `Pointee` from this:

```rust
// before

impl Pointee for u8 {
    type Metadata = ();
}

impl Pointee for [u8] {
    type Metadata = usize;
}

// ...

impl Pointee for Wrapper<u8> {
    type Metadata = ();
}

impl Pointee for Wrapper<[u8]> {
    type Metadata = usize;
}

// ...

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T: ?Sized> Pointee for Wrapper<T>
where
    Wrapper<T>: Sized
{
    type Metadata = ();
}

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T /*: Sized */> Pointee for T {
    type Metadata = ();
}
```

to this:

```rust
// after

impl Pointee for u8 {
    type Metadata = ();
}

impl Pointee for [u8] {
    type Metadata = usize;
}

// ...

impl<T: ?Sized> Pointee for Wrapper<T> {
    // in the old solver this will instead project to the "deep" tail directly,
    // e.g. `Wrapper<Wrapper<T>>::Metadata = T::Metadata`
    type Metadata = <T as Pointee>::Metadata;
}

// ...

// This impl is only selected if `T` is a type parameter or unnormalizable projection or opaque type.
fallback impl<T /*: Sized */> Pointee for T {
    type Metadata = ();
}
```
@lukas-code lukas-code deleted the metadata-normalize branch February 10, 2024 23:13
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 7, 2024
…r-projections, r=lcnr

silence mismatched types errors for implied projections

Currently, if a trait bound is not satisfied, then we suppress any errors for the trait's supertraits not being satisfied, but still report errors for super projections not being satisfied.

For example:
```rust
trait Super {
    type Assoc;
}
trait Sub: Super<Assoc = ()> {}
```
Before this PR, if `T: Sub` is not satisfied, then errors for `T: Super` are suppressed, but errors for `<T as Super>::Assoc == ()` are still shown. This PR makes it so that errors about super projections not being satisfied are also suppressed.

The errors are only suppressed if the span of the trait obligation matches the span of the super predicate obligation to avoid silencing error that are not related. This PR removes some differences between the spans of supertraits and super projections to make the suppression work correctly.

This PR fixes the majority of the diagnostics fallout when making `Thin` a supertrait of `Sized` (in a future PR).
cc rust-lang#120354 (comment)
cc `@lcnr`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2024
Rollup merge of rust-lang#121863 - lukas-code:silence-mismatched-super-projections, r=lcnr

silence mismatched types errors for implied projections

Currently, if a trait bound is not satisfied, then we suppress any errors for the trait's supertraits not being satisfied, but still report errors for super projections not being satisfied.

For example:
```rust
trait Super {
    type Assoc;
}
trait Sub: Super<Assoc = ()> {}
```
Before this PR, if `T: Sub` is not satisfied, then errors for `T: Super` are suppressed, but errors for `<T as Super>::Assoc == ()` are still shown. This PR makes it so that errors about super projections not being satisfied are also suppressed.

The errors are only suppressed if the span of the trait obligation matches the span of the super predicate obligation to avoid silencing error that are not related. This PR removes some differences between the spans of supertraits and super projections to make the suppression work correctly.

This PR fixes the majority of the diagnostics fallout when making `Thin` a supertrait of `Sized` (in a future PR).
cc rust-lang#120354 (comment)
cc `@lcnr`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 8, 2024
…ions, r=lcnr

silence mismatched types errors for implied projections

Currently, if a trait bound is not satisfied, then we suppress any errors for the trait's supertraits not being satisfied, but still report errors for super projections not being satisfied.

For example:
```rust
trait Super {
    type Assoc;
}
trait Sub: Super<Assoc = ()> {}
```
Before this PR, if `T: Sub` is not satisfied, then errors for `T: Super` are suppressed, but errors for `<T as Super>::Assoc == ()` are still shown. This PR makes it so that errors about super projections not being satisfied are also suppressed.

The errors are only suppressed if the span of the trait obligation matches the span of the super predicate obligation to avoid silencing error that are not related. This PR removes some differences between the spans of supertraits and super projections to make the suppression work correctly.

This PR fixes the majority of the diagnostics fallout when making `Thin` a supertrait of `Sized` (in a future PR).
cc rust-lang/rust#120354 (comment)
cc `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2024
make `Thin` a supertrait of `Sized`

This is a follow-up to rust-lang#120354 to address rust-lang#120354 (comment) and remove the "fallback impl" hack from `<T as Pointee>::Metadata` normalization in both trait solvers.

We make `Thin` and therefore `Pointee<Metadata = ()>` a supertrait of `Sized`, such that every (implicit or explicit) `T: Sized` bound now also requires `T: Thin` and `<T as Pointee>::Metadata == ()`. These new bounds will be used when normalizing `<T as Pointee>::Metadata` instead of a hacky builtin impl that only applies for type parameters and aliases and overlaps with other builtin impls.

Note that [RFC 2580](https://rust-lang.github.io/rfcs/2580-ptr-meta.html), which introduced `Pointee` and `Thin`, lists these unresolved questions:

> Should `Thin` be added as a supertrait of `Sized`? Or could it ever make sense to have fat pointers to statically-sized types?

For now, they are answered with yes and no respectively. If we end up deciding that we do want types that are `Sized + !Thin`, then a possible alternative is to add make `Thin` a defaulted trait bound that can be relaxed with `T: ?Thin` and remove `Thin` as supertrait from `Sized` before the stabilization of `feature(ptr_metadata)`.

---

The removal of the "fallback impl" also allows us to always project to the shallow struct tail in the old solver, making the implementation in the old solver almost identical to the new solver. This is required to make `<Wrapper<T> as Pointee>::Metadata` work correctly in the presence of trivial bounds, for example if we know `[T]: Thin`, then we should also know `Wrapper<T>: Thin`.

Lastly, this PR implements some diagnostics changes to hide errors about `` type mismatch resolving `<T as Pointee>::Metadata == ()` `` when the actual error comes from `T: Sized`. This is a continuation of rust-lang#121863.

r? `@lcnr`
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. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants