don't normalize where-clauses when checking well-formedness

[lqd]

WfCheck checks where-clauses after normalization, and we'd like to see what would break if it didn't for rust-lang/trait-system-refactor-initiative#255

r? ghost

[lqd]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: f3d95de (f3d95de532264a1215023baa665c0028aecf74b1, parent: 90b65889799733f21ebdf59d96411aa531c5900a)

[lqd]

@craterbot check

[craterbot]

👌 Experiment pr-148477 created and queued.
🤖 Automatically detected try build f3d95de
🔍 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-148477 is now running

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

[craterbot]

🎉 Experiment pr-148477 is completed!
📊 7 regressed and 7 fixed (730342 total)
📊 1888 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

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

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-148477/retry-regressed-list.txt ↩

[bors]

☔ The latest upstream changes (presumably #139558) made this pull request unmergeable. Please resolve the merge conflicts.

[lcnr]

Affected projects:

2 dependencies of modcholesky, same as rust-lang/trait-system-refactor-initiative#255

pub struct View<A>(A);
pub trait Data {
    type Elem;
}
impl<'a, A> Data for View<&'a A> {
    type Elem = A;
}

pub fn repro<'a, T>()
where
    <View<&'a T> as Data>::Elem: Sized,
{

https://github.com/vilicvane/unipipe/tree/9019a4b03df6ed222975b2774d686d5803860568

pub struct LifetimeTestPipe2<'a, T> {
    prefix: &'a str,
    transformer: &'a dyn Fn(&T) -> String,
    buffer: Vec<String>,
}

impl<'a, T> unipipe::UniPipe for LifetimeTestPipe2<'a, T> {
    type Input = T;
    type Output = String;

    fn next(&mut self, input: Option<Self::Input>) -> impl Into<Output<Self::Output>> {
        // ...
    }
}
#[unipipe::unipipe(iterator, try_iterator)]
impl<'a, T> LifetimeTestPipe2<'a, T> {
    pub fn new(prefix: &'a str, transformer: &'a dyn Fn(&T) -> String) -> Self {
        Self {
            prefix,
            transformer,
            buffer: Vec::new(),
        }
    }
}

expands to something containing

pub trait LifetimeTestPipe2UniPipeIteratorExt<'a, T>:
    Iterator<Item = <LifetimeTestPipe2<'a, T> as ::unipipe::UniPipe>::Input> + Sized
{
    // ...
}
impl<'a, T, TIterator> LifetimeTestPipe2UniPipeIteratorExt<'a, T> for TIterator where
    TIterator: Iterator<Item = <LifetimeTestPipe2<'a, T> as ::unipipe::UniPipe>::Input>
{
}

pub trait LifetimeTestPipe2UniPipeTryIteratorExt<'a, T, TError>:
    Iterator<Item = Result<<LifetimeTestPipe2<'a, T> as ::unipipe::UniPipe>::Input, TError>> + Sized
{
    // ...
}
impl<'a, T, TError, TIterator> LifetimeTestPipe2UniPipeTryIteratorExt<'a, T, TError> for TIterator where
    TIterator:
        Iterator<Item = Result<<LifetimeTestPipe2<'a, T> as ::unipipe::UniPipe>::Input, TError>>
{
}

That one is annoying. The macros can't really know the implied bounds of LifetimeTestPipe2, so I avoiding these errors is really challenging :<

[lcnr]

I think we should also stop normalizing in check_associated_type_bounds before checking that the where-clauses are wf.

rust/compiler/rustc_hir_analysis/src/check/wfcheck.rs

Lines 1154 to 1163 in a7b3715

	let wf_obligations = bounds.iter_identity_copied().flat_map(|(bound, bound_span)| {
	let normalized_bound = wfcx.normalize(span, None, bound);
	traits::wf::clause_obligations(
	wfcx.infcx,
	wfcx.param_env,
	wfcx.body_def_id,
	normalized_bound,
	bound_span,
	)
	});

[lqd]

And probably these

rust/compiler/rustc_hir_analysis/src/check/wfcheck.rs

Line 1490 in cf33ee0

let pred = wfcx.normalize(sp, None, pred);

from the default args as well, right?

[lcnr]

wrt to normalizing obligations for default params before proving them 😅

we do need to normalize obligations before proving them in the old solver and this already doesn't normalize WellFormed obligations.

rust/compiler/rustc_trait_selection/src/traits/normalize.rs

Line 485 in 5dbf406

if p.allow_normalization() && needs_normalization(self.selcx.infcx, &p) {

rust/compiler/rustc_middle/src/ty/predicate.rs

Lines 125 to 127 in 5dbf406

	PredicateKind::Clause(ClauseKind::WellFormed(_)) | PredicateKind::AliasRelate(..) => {
	false
	}

The only "issue" is when normalizing obligations (or types - which we currently sometimes do intentionally because of implied bounds stuff) before checking that the obligation is well-formed, and this only happens before calling clause_obligations. In a sense calling clause_obligations (and proving its nested obligations) is the same as a emitting a ClauseKind::WellFormed(some_where_clause) goal if we were to support that

[lcnr]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 04ea1e1 (04ea1e1f1a12cfa912c228ca278237d92d0bb6df, parent: 5dbf4069dc98bbbca98dd600a65f50c258fbfd56)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-148477-1 created and queued.
🤖 Automatically detected try build 04ea1e1
🔍 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

[lcnr]

@rfcbot fcp merge types

Summary

This remove the normalization of where-clauses before checking that they are well-formed, causing the following pattern to now error:

pub struct View<A>(A);
pub trait Data {
    type Elem;
}
impl<'a, A> Data for View<&'a A> {
    type Elem = A;
}

pub fn repro<'a, T>()
where
    <View<&'a T> as Data>::Elem: Sized, // ERRROR: requires `T: 'a`
{
}

We generally do not want to check well-formedness after normalization. Normalization expects its input to be well-formed and we break that invariant by normalizing before we ever check well-formedness.

Due to reasons we currently do sometimes normalize before checking well-formedness. Notably we do so for impl headers. We are only able to use the normalized impl header for implied bounds, so checking that the unnormalized impl header is well-formed results in undesirable errors. Note that this is currently unsound, cc #100051.

Other parts of the type system do check well-formedness before normalizing, e.g. function signatures are checked for well-formedness before normalization inside of MIR borrowck. We also check WF of types inside of bodies before normalization.

We generally want to move towards always checking WF before normalization. While it currently can result in WF types in the source being accepted, e.g. <RequireStatic<'a> as ToUnit>::Unit gets accepted, with const generics this will actually be unsound by normalizing to something containing a value whose invariants are checked as part of its constructor, not of its type, cc #142239.

We should move everything to check well-formedness before normalization once we've got proper implied bounds support, as that allows us to use implied bounds from unnormalized types in the signature without being unsound. Until then, I would also like to move as much code as possible to check WF before normalization as long as the resulting breakage is acceptable.

I spent some time looking at the relevant code and it's somewhat of a mess and expect us to be able to do a lot here even before then. E.g. we should start checking also checking the type of constants for WF before normalizing it #100041.

I do think this is a step in the right direction and would like to merge this as is. I am open to mentoring somebody already somewhat experienced here. We should probably start by collecting and documenting the status quo: where do we check WF before normalization and where do we not so yet.

This fixes the divergence between the old and new solver from rust-lang/trait-system-refactor-initiative#255 by also emitting an error in the old solver.

What is stabilized n/a

What isn't stabilized n/a

Design

Reference

The reference does not describe whether and how we normalize before checking well-formedness.

RFC history n/a

Answers to unresolved questions n/a

Post-RFC changes n/a

Key points n/a

Nightly extensions n/a

Doors closed n/a

Feedback

Call for testing n/a

Nightly use n/a

Implementation

Major parts n/a

Coverage

@lqd, please add the test from rust-lang/trait-system-refactor-initiative#255

Outstanding bugs

Our handling of well-formedness is generally something of a mess right now. While some of these issues require proper implied bounds on binders support, not all of them do and we should look into fixing some of them.

We don't check WF before normalization in places where we should, e.g. #100041. Even when only getting implied bounds after normalization, our handling of them is still unsound #100051.

We also sometimes don't check WF at all, e.g. #123312 or #104478.

Outstanding FIXMEs n/a

Tool changes n/a

Breaking changes

Crater report: #148477 (comment)
Crater analysis: #148477 (comment)

We've got modcholesky with the same failure as in the PR description and https://github.com/vilicvane/unipipe which generates macros which only reference an impl self-type in the where-bounds.

modcholesky can be fixed by replacing the where-bound with it's normalized form.

we should reach out to https://github.com/vilicvane/unipipe to suggest moving the self-type as a trait argument of the generated traits, see #t-types/nominated > #148477: crater: don't normalize where-clauses when checkin… @ 💬. I think we should opening a PR for them before landing this. If that fix does not work out I would still land this breakage regardless.

Type system, opsem

Compile-time checks n/a

Type system rules n/a

Sound by default? n/a

Breaks the AM? n/a

Common interactions

Temporaries n/a

Drop order n/a

Pre-expansion / post-expansion n/a

Edition hygiene n/a

SemVer implications n/a

Exposing other features n/a

History

Would be nice, so much effort though and I don't want to block this PR on that 😅

Acknowledgments n/a

Open items

• open fixes to the two affected projects
• add ui test for this change

References n/a

[lcnr]

@rfcbot ping

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lcnr]

#148477 (comment)

@rfcbot fcp merge types

[rust-rfcbot]

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[jackh726]

think we should opening a PR for them before landing this. If that fix does not work out I would still land this breakage regardless.

@lcnr can you say more here? Do you mean, if the fix "works" but isn't what they "want", we should still land the breakage? Or even if the fix doesn't work? Or if they just don't land it, etc.?

I would like to land this, but I feel mildy uncomfortable landing without a fix that actually works. I'm okay to land if @lqd's fix works and they don't land it for whatever reason, but less okay if the fix doesn't actually work.

[lqd]

@jackh726 To be clear, I have working fixes for both crates. The maintainers could improve upon them for sure, but they are at least one way where the crates won't be broken by this PR, and that was the important point to me to minimize the breakage.

[lcnr]

@lcnr can you say more here? Do you mean, if the fix "works" but isn't what they "want", we should still land the breakage? Or even if the fix doesn't work? Or if they just don't land it, etc.?

I would landed this even if we were unable to come up with a localized fix, though @lqd ended up with a working fix, so that point is moot :>