Handle projections as uncovered types during coherence check

[atsuzaki]

Fixes #99554. Update code to handle projections as uncovered types during coherence check/orphan check & update error messages accordingly.

[rust-highfive]

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

[atsuzaki]

r? @lcnr

[lcnr]

there are still 2 tests where the error messages aren't quite right

please fix the error messages for them and add them as additional tests

going to start a crater run for this

[lcnr]

@bors try

[bors]

⌛ Trying commit ba88709461a684be5ef433e2da892492ebde60b0 with merge df0acfe6d8f079d1fdd8e6b7447569d81b00f126...

[bors]

☀️ Try build successful - checks-actions
Build commit: df0acfe6d8f079d1fdd8e6b7447569d81b00f126 (df0acfe6d8f079d1fdd8e6b7447569d81b00f126)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-100555 created and queued.
🤖 Automatically detected try build df0acfe6d8f079d1fdd8e6b7447569d81b00f126
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-100555 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-100555 is completed!
📊 208 regressed and 4 fixed (241311 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

ok, we have more regressions than I would like. Most of them look like they use fully concrete projections though, and these are fine.

I see two ways forward here:

• simply check whether the projection contains any generic parameters and fail in that case, or
  - actually recursively orphan check each projection using InCrate::Remote and consider them as non-local type if the orphan check fails and as uncovered projection/param if that orphan check succeeds.

I think recursively orphan checking projections seems like a better solution to me, so I would go with that. When encountering a projection in the type visitor, call orphan_check_trait_ref for that projection, if that fails, treat the projection as a non-local type, if it succeeds, treat it as an uncovered type.

edit: The second approach sadly does not work, as even a local impl can use a type parameter as an associated type if it's used as in input.

[atsuzaki]

@lcnr This is one of the last (visual) kinks... I was able to detect the fundamental type for cases like impl<T> Remote for Box<T> where the T is at idx=0, and I can look at self_ty which is a rustc_middle::Ty.

But in this case, I only have a hir::Ty and can't figure out how it's a fundamental type from that.

[atsuzaki]

urgh the link broke after rebase+force push, https://github.com/rust-lang/rust/pull/100555/files#diff-fddbd2e2a2a86430f2f013d5d787d7ad6c85427cd81a2282ac0fcc7d641733bc

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@lcnr can you give a bit more details about the wasmer crate? I feel like we ought to reach out to them to discuss possible solutions, as that is a pretty broadly used crate.

[nikomatsakis]

cc @matklad @nagisa -- two owners of the wasmer-types crate

[lcnr]

The root cause seems to be the Deserialize derive from https://github.com/rkyv/rkyv.

This derive emits the following impl:

#[derive(Deserialize, Archive)]
struct Foo<T>(T);

impl<T, D> Deserialize<Foo<T>, D> for <Foo<T> as Archive>::Archived {
    // ...
}

impl<T> Archive for Foo<T> {
   type Assoc = LocalType;
}

See https://gist.github.com/lcnr/75d99bd2900f9dd2eaa558b3c68c42e2 for the full (cleaned up) output.

This could be adapted to pass the new orphan rules by switching the argument ordering of Deserialize: instead of ARCHIVE: Deserialize<TYPE, DESERIALIZER> it could be TYPE: Deserialize<ARCHIVE, DESERIALIZER>. This would be probably be the easiest fix (even if it is a breaking change for rkyv).

There are hacks which would only require a minor update of rkyv while still allowing us to error on unsound impls, e.g. by adding some #[no_uncovered_param] attribute to associated types (in the trait definition) which causes us to consider that associated type to always only be a non-local type and requires all impls to not directly use T or Fundamental<T> for it. I don't think that idea is worth it and can't think of one which is.

[matklad]

cc @djkoloski

[djkoloski]

If I'm understanding this problem correctly, this could also be addressed by emitting a concrete ARCHIVE type instead of referring to it indirectly through the Archive trait's associated type. Is that accurate? I'd have to check and make sure, but that seems more achievable for rkyv.

[lcnr]

yes, using the concrete type for the archive instead of an associated type works as well

[djkoloski]

Great, I'll take action on rkyv/rkyv#289 and cut a new release as soon as possible.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[oli-obk]

@atsuzaki could you rebase the PR and fix the conflict? I think we're ready to merge

[lcnr]

we still have to convert this to a future-compat lint before merging this, intend to meet with @atsuzaki to figure out how exactly this should be done

[apiraino]

Hi, how is this progressing? Any updates to report cc @lcnr @atsuzaki ? Thanks!

[atsuzaki]

@apiraino hi, I sadly no longer have time to work on this. I've spoken to @lcnr about them/someone possibly taking over this?

[JohnCSimon]

@atsuzaki

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive