Introduce CoercePointeeWellformed for coherence checks at typeck stage #136107

dingxiangfei2009 · 2025-01-26T22:24:28Z

This is the first PR to introduce the "wellformedness" check for derive(CoercePointee).

This patch introduces a new error code to cover all the prerequisites of the said macro. The checks that is enforced with this patch is whether the data is indeed struct and whether the layout is set to repr(transparent).

A following series of patch will arrive later to address the following concern.

CoercePointee leaks unstable unsizing impls #135217 so that we would only admit one single coercion on one type parameter, and leave the rest for future consideration in tandem of development of other coercion rules.
Enforcement of data field requirements.

An open question is whether there is a good schema to encode the #[pointee] as well, so that we could also check if the #[pointee] type parameter is indeed ?Sized.

@rustbot label F-derive_coerce_pointee

rustbot · 2025-01-26T22:24:36Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-01-26T22:24:38Z

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

compiler-errors · 2025-01-26T23:55:02Z

compiler/rustc_hir_analysis/src/coherence/builtin.rs

+    };
+    let did = def.did();
+    let span =
+        if let Some(local) = did.as_local() { tcx.source_span(local) } else { tcx.def_span(did) };


What is this as_local check?

The def id always should be local, right? if it's not, then it's an incoherent impl and we should probably just delay a bug.

compiler-errors · 2025-01-26T23:56:43Z

compiler/rustc_hir_analysis/src/coherence/builtin.rs

+    };
+    let did = def.did();
+    let span =
+        if let Some(local) = did.as_local() { tcx.source_span(local) } else { tcx.def_span(did) };


Do not use source_span -- the query says:

. This span is meant for dep-tracking rather than diagnostics. It should not be used outside of rustc_middle::hir::source_map.

compiler-errors · 2025-01-26T23:57:23Z

compiler/rustc_hir_analysis/src/coherence/builtin.rs

@@ -790,3 +794,27 @@ fn visit_implementation_of_pointer_like(checker: &Checker<'_>) -> Result<(), Err
        .with_note(why_disqualified)
        .emit())
 }
+
+fn visit_implementation_of_coerce_pointee_wellformed(


Why are we still doing validation inside of the proc macro? Shouldn't we remove the validation from the proc macro since we are doing it here now?

Speaking from another perspective, rejecting the derivation earlier as well means that we can refuse to generate the unnecessary ASTs that later compilation passes needs to process. Given the current state of the unstable features involved, it seems to me that it could probably be better for us to intercept inadmissible uses of the macro so that we would not inadvertently expose the behaviour of those unstable features that we had not committed to yet.

I am honestly not sure. This can go either way to me.

I do not think we should keep both validation. Please remove it :)

compiler-errors · 2025-01-26T23:58:38Z

library/core/src/marker.rs

+
+#[cfg(not(bootstrap))]
+#[lang = "coerce_pointee_wellformed"]
+#[unstable(feature = "derive_coerce_pointee", issue = "123430")]


This needs to be gated behind a different feature gate, since users should never be able to implement CoercePointeeWellFormed manually. You can follow StructuralPartialEq to see how to gate an internal derive-only trait properly.

compiler-errors · 2025-01-27T00:00:18Z

compiler/rustc_builtin_macros/src/deriving/coerce_pointee.rs

@@ -110,6 +110,52 @@ pub(crate) fn expand_deriving_coerce_pointee(
    // Declare helper function that adds implementation blocks.
    // FIXME(dingxiangfei2009): Investigate the set of attributes on target struct to be propagated to impls
    let attrs = thin_vec![cx.attr_word(sym::automatically_derived, span),];
+    // # Wellformed-ness assertion


Please elaborate what well-formedness means.

I think you may also want to strongly consider using a different term than "well-formed". We already use the term within the type system for a different meaning. I think it's more correct to call this something like "DeriveValidity" or something, to make it clear that we use this impl to validate the correcness of the derive.

compiler-errors

Still needs fixing?

compiler-errors · 2025-02-06T18:02:38Z

library/core/src/marker.rs

+/// and shall not ever be stabilised.
+#[cfg(not(bootstrap))]
+#[lang = "coerce_pointee_validated"]
+#[unstable(feature = "coerce_pointee_validated", issue = "123430")]


This doesn't need a tracking issue since it's never going to be stabilized. Just put issue = "none" I think.

rustbot · 2025-02-09T17:43:02Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

compiler-errors · 2025-02-09T19:22:44Z

I'm a bit confused why this PR has gotten so large. It's now introducing a query, interacting with specialization graph somehow, and doing a lot more than it originally set out to do, which as far as I could tell is just a check for the pointee ADT being transparent.

I think you should revert most of this PR and explain more clearly in a follow-up issue or zulip thread what you intend to achieve with this validation before committing something that is very difficult to review 😓

@rustbot author

dingxiangfei2009 · 2025-02-09T20:03:19Z

@compiler-errors Thank you for reviewing! I am going to truncate the commit logs and focus only on the repr(transparent) validation first.

I am moving the discussion to Zulip.

dingxiangfei2009 · 2025-02-09T22:53:58Z

@rustbot ready

Unwinded work to include fix only for CoercePointee leaks unstable unsizing impls #135217
Defer the redaction work into another PR Redact CoercePointee target type #136796

rustbot assigned Nadrieril Jan 26, 2025

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 26, 2025

rustbot added T-libs Relevant to the library team, which will review and decide on the PR/issue. F-derive_coerce_pointee Feature: RFC 3621's oft-renamed implementation labels Jan 26, 2025

This comment has been minimized.

Sign in to view

compiler-errors requested changes Jan 27, 2025

View reviewed changes

compiler-errors assigned compiler-errors and unassigned Nadrieril Jan 27, 2025

ojeda mentioned this pull request Jan 27, 2025

Rust unstable features needed for the kernel Rust-for-Linux/linux#2

Open

90 tasks

This comment has been minimized.

Sign in to view

Darksonn mentioned this pull request Jan 29, 2025

Confusing error when using CoercePointee #134846

Open

compiler-errors reviewed Feb 6, 2025

View reviewed changes

dingxiangfei2009 added 3 commits February 9, 2025 20:40

introduce CoercePointeeWellformed for coherence checks at typeck stage

de405dc

rename the trait to validity and place a feature gate afront

c067324

move repr(transparent) checks to coherence

b943505

dingxiangfei2009 force-pushed the coerce-pointee-wellformed branch from 4c42131 to 645b231 Compare February 9, 2025 12:44

This comment has been minimized.

Sign in to view

dingxiangfei2009 force-pushed the coerce-pointee-wellformed branch from 645b231 to e55e437 Compare February 9, 2025 16:09

This comment has been minimized.

Sign in to view

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 Feb 9, 2025

dingxiangfei2009 mentioned this pull request Feb 9, 2025

Perform more validation on CoercePointee #136789

Draft

dingxiangfei2009 added 2 commits February 10, 2025 04:36

block coerce_pointee_validated for stabilization

1848343

fix the error code document

aea5595

dingxiangfei2009 force-pushed the coerce-pointee-wellformed branch from 5324992 to aea5595 Compare February 9, 2025 20:39

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 9, 2025

dingxiangfei2009 requested a review from compiler-errors February 9, 2025 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CoercePointeeWellformed for coherence checks at typeck stage #136107

Introduce CoercePointeeWellformed for coherence checks at typeck stage #136107

dingxiangfei2009 commented Jan 26, 2025

rustbot commented Jan 26, 2025

rustbot commented Jan 26, 2025

This comment has been minimized.

compiler-errors Jan 26, 2025

compiler-errors Jan 26, 2025

compiler-errors Jan 26, 2025

dingxiangfei2009 Jan 27, 2025

compiler-errors Jan 27, 2025

compiler-errors Jan 26, 2025

compiler-errors Jan 27, 2025

This comment has been minimized.

compiler-errors left a comment

compiler-errors Feb 6, 2025

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 9, 2025

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Feb 9, 2025 •

edited

Loading

dingxiangfei2009 commented Feb 9, 2025

dingxiangfei2009 commented Feb 9, 2025

Introduce CoercePointeeWellformed for coherence checks at typeck stage #136107

Are you sure you want to change the base?

Introduce CoercePointeeWellformed for coherence checks at typeck stage #136107

Conversation

dingxiangfei2009 commented Jan 26, 2025

rustbot commented Jan 26, 2025

rustbot commented Jan 26, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 9, 2025

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Feb 9, 2025 • edited Loading

dingxiangfei2009 commented Feb 9, 2025

dingxiangfei2009 commented Feb 9, 2025

compiler-errors commented Feb 9, 2025 •

edited

Loading