Redact CoercePointee target type

[dingxiangfei2009]

Fix #134846

The issue here is that __S is actually an implementation detail of derive(CoercePointee). This should not appear in our diagnostics. A better way to present this message is to designate it as <pointee generic type> {unsized} as opposed to the undecipherable __S here.

r? @compiler-errors

[dingxiangfei2009]

@rustbot label +F-derive_coerce_pointee

[dingxiangfei2009]

I will mark as ready when the base PR is is ready to go as well

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[compiler-errors]

This PR changes introduces quite a lot of machinery just to rename __S to T {unsized}. I don't actually think that's very different, and not worth the changes.

If we're going to do anything here, then it needs to be about changing the wording of the error message to explain what this Unsize obligation is about.

[dingxiangfei2009]

I would like to quote #134846 that this PR attempts to fix:

Desired output
Something that does not mention internal things like Unsize or __S

So I am thinking, should we instead redact all errors from unfulfilled Unsize obligation entirely and replace them with messages to say unsizing <some type> is not permitted, or something along this line?

Concretely, if we encounter a fulfillment error for Ty1: Unsize<Ty2>, we write out a message saying unsizing Ty1 is not permitted and forget about Ty2, since Ty2 may contain internal stuffs like __S in most cases and redacting those internal type names individually is now considered harmful. Is this what we are going after here?

[compiler-errors]

Sure. I think all this PR needs to do is intercept the errors in:

rust/compiler/rustc_hir_analysis/src/coherence/builtin.rs

Lines 560 to 562 in 06a24e9

	if !errors.is_empty() {
	infcx.err_ctxt().report_fulfillment_errors(errors);
	}

And:

rust/compiler/rustc_hir_analysis/src/coherence/builtin.rs

Lines 348 to 350 in 06a24e9

	if !errors.is_empty() {
	res = Err(infcx.err_ctxt().report_fulfillment_errors(errors));
	}

And just say something like: "In order for ADT to have a valid implementation of CoerceUnsized, you need to be able to unsize &W<T>."

Most importantly, it needs to explain why the unsize isn't valid. I know you're interested in fixing what was requested in #134846, but please think about the bigger picture, and also please think about minimizing the changes to the compiler while trying to fix the issue.

[compiler-errors]

See comment. If we're going to touch these error messages at all, then they should be heavily reworked to explain why the Unsize goal exists at all.

[compiler-errors]

no function-local glob imports please

[Darksonn]

When you get this error: "the trait bound X: Unsize<Y> is not satisfied"

I think the real error you need to emit is something like:

error: derive(CoercePointee) requires that you can unsize `X`

help: it can only be unsized if it stores exactly one copy of T at the end of the structure
help: the T must be stored without additional pointer indirection

this lets you hide the __S by not printing Y.

If you instead get this error: "the trait bound X: CoerceUnsized<Y> is not satisfied", then you need to emit an error along these lines:

error: the field must be a stdlib pointer type, or a user-defined type using the derive(CoercePointee) macro

[compiler-errors]

This implementation still needs a lot of clean up and simplification, but I don't want to leave review comments until it's clear that it's ready for review. S

pecifically, please check that the code has no changes that are unrelated to fixing this issue; all changes should either be reverted or exercised by tests.

Please mark it as S-waiting-on-review when it's ready.

[dingxiangfei2009]

@rustbot ready

• We now have new tailored diagnostics for Unsize and CoerceUnsized obligation errors originated from derive(CoercePointee).
• We expand the spans to point into the offending fields in the struct definition.
• Revert more chunks about the mini-cores, by making the #[pointee] extraction optional

[compiler-errors]

Please revert this. It's not useful.

[dingxiangfei2009]

source binding is shadowed later. However, the diagnostics depends on this Self type.

[compiler-errors]

Why not just rename the variable? The reassignment feels a bit awkward.

[compiler-errors]

Same here.

[compiler-errors]

Is this exercised by a test?

[compiler-errors]

Unless there is a good reason to have this, please revert it.

[dingxiangfei2009]

It is exercised in https://github.com/rust-lang/rust/pull/136796/files#diff-a7fbdfccbc550bad2fc7b7a60f3a17237203e2848f1589723bc49a64f0dbabb3R129

[compiler-errors]

Is this exercised by a test?

[dingxiangfei2009]

It is exercised in https://github.com/rust-lang/rust/pull/136796/files#diff-a7fbdfccbc550bad2fc7b7a60f3a17237203e2848f1589723bc49a64f0dbabb3R147

[compiler-errors]

Is this exercised by a test?

[compiler-errors]

Why is this implementation "micro-optimized"? It shouldn't matter to override all these functions, and it makes the visitor much larger than it needs to be.

[compiler-errors]

Why is this two nested if statements when the next one is &&? Can you choose one?

[compiler-errors]

Suggested change

	hir_analysis_coerce_pointee_cannot_unsize = `{$ty}` cannot be coerced to any unsized value
	hir_analysis_coerce_pointee_cannot_unsize = `{$ty}` cannot be coerced to an unsized value

[compiler-errors]

This note is not exercised by any test, I think?

[dingxiangfei2009]

I just realised that the note cannot be exercised because the span falls within the macro expansion.

Instead it is overwritten by this message.

note: this error originates in the derive macro `CoercePointee` (in Nightly builds, run with -Z macro-backtrace for more info)

This happens still after adding the missing #[note] and #[help] attributes to the diagnostics.

[dingxiangfei2009]

Maybe I need to switch to the span of the ADT definition.

[compiler-errors]

Similar, this note is not exercised by any test?

[compiler-errors]

OK -- so I think I understand my problem with this PR in its current state. Specifically, it's trying to do two things:

1. Change the error messages when fields violate the implementation validity *structurally, like:

#![feature(derive_coerce_pointee)]
use std::marker::CoercePointee;

#[derive(CoercePointee)]
#[repr(transparent)]
struct P<T: 'static + ?Sized> {
    x: &'static T,
    y: &'static T,
}

2. Improve the error messages when trait solving fails to satisfy the CoercePointee or DispatchFromDyn goal needed for validity:

#![feature(derive_coerce_pointee)]
use std::marker::CoercePointee;

#[derive(CoercePointee)]
#[repr(transparent)]
struct P<T: 'static + ?Sized> {
    x: (T,),
}

I'm somewhat hesitant that they need to be fixed at the same time, and I dislike the fact that the improvements relies on extract_coerce_pointee_data doing so much work.

For example, for (1.) we could simply just detect if the impl is coming from an expansion (by looking at the span of the impl) and change the error message (like DispatchFromDynSingle, DispatchFromDynMulti) into more tailored versions that don't need to mention the "right-hand-side type", like &'static S__.

For the trait error reporting, we could simply emit a more tailored fulfillment error if there is only one fulfillment error and it ends up failing an Unsize or CoerceUnsized goal, rather than relying on a somewhat pervasive control flow invariant that extract_coerce_pointee_data will emit an error.

TL;DR is that I think this PR is trying to change a lot all at once, and I'm somewhat overwhelmed with the state of it right now.

[dingxiangfei2009]

@compiler-errors I sense that it is preferable to break this PR into two and improve the diagnostics in 1. and 2. separately. Shall we start from there?

pervasive control flow invariant that extract_coerce_pointee_data will emit an error

I couldn't t follow unfortunately. It could be the naming at fault here and I owe the explanation, nevertheless I found the purpose of extract_coerce_pointee_data straightforward. It only deals with deciding whether derive(CoercePointee) is applied and, in that case, extract the location of #[pointee] in the list of generics of the struct.

by looking at the span of the impl

It is where I can see intricacy. I am afraid that span information is brittle and unreliable because derive(CoercePointee) will not be the only source of impl DispatchFromDyn/CoerceUnsized. Even #[derive(CoercePointee)] itself can originate from expansion from other (proc-)macros.

if there is only one fulfillment error

This gets me thinking. What shall be the treatment when there are more than just one, and among those fulfilment errors are some involves __S but not about CoerceUnsized/Unsize/DispatchFromDyn, rather lifetime errors? I will leave the question in the second PR or beyond.

[compiler-errors]

I opened an alternative PR in #137289 that is a bit less complicated. I think it can be improved and refined in the future, but it seems to me easier to land than this one.