Don't normalize in AstConv

[aliemjay]

See individual commits.

Fixes #101350
Fixes #54940

[rust-highfive]

r? @jackh726

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

[the8472]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 82fa1136489005893be02555b865ca6a90d923f3 with merge 1ab52f9b015fe3b11f0862dd6cbf44c426995a28...

[bors]

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

[rust-timer]

Queued 1ab52f9b015fe3b11f0862dd6cbf44c426995a28 with parent 44fcfb0, future comparison URL.

[rust-timer]

Finished benchmarking commit (1ab52f9b015fe3b11f0862dd6cbf44c426995a28): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

[aliemjay]

r? @lcnr

[bors]

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

[lcnr]

recording types with escaping bound vars is pretty cursed anyways 🤔 ideally we just don't™. Do you know where that currently happens?

[aliemjay]

This happens in nested hir::Ty's, e.g. &'a str in let _: for<'a> fn(&'a str). But I don't think they're relevant to typeck since the outer type has no escaping bound vars. One place where it actually matters is in closures - underscore lifetimes are always late-bound regions (e.g. |_: &'_ str| {}) but we currently handle this in closure.rs by normalizing the complete signature rather than each input type separately.

[lcnr]

this means that self_ctor_substs are already normalized, but are used as substs_raw further down. I would expect this to be a bug (where we again ignore region obligations from user written types)

[aliemjay]

SelfCtor type is already checked by item wfcheck, so I don't think we will miss region obligations when normalizing.

[lcnr]

hmm, we can't rely on probe_adt here because we also need the substs 🤔 i guess that's fine then.

While I am not sure whether we could land this, can you add something like the following here?

FIXME: Try to change this to use the unnormalized type here and check the resulting breakage.

[aliemjay]

This was indeed problematic. I have fixed it in e05158d.

[lcnr]

no, we shouldn't imo

imo we should have never relied on normalization during ast lowering. We won't be able to remove the existing normalization due to backwards compatibility. I think we should just live with the existing inconsistency, potentially even linting when normalization is required to remove it 2030 or whatever

[aliemjay]

I tend to agree. The fact that path resolution depends on normalization may be an unintended behavior.

[lcnr]

cc @rust-lang/types I generally don't think we should normalize during astconv to resolve stuff like Self::A if Self is a projection. Examples include

trait Identity {
    type Id;
}
impl<T> Identity for T {
    type Id = T;
}

enum Foo {
    A,
    B,
}

trait Trait {
    fn return_a() -> Self;
}

impl Trait for <Foo as Identity>::Id {
    fn return_a() -> Self {
        Self::A
    }
}

fn main() {
    let x = <<Foo as Identity>::Id>::A;
}

We do have to keep supporting this for backwards compatibility however.

I therefore think that we should 1) remove the FIXME here and add a comment for "we cannot normalize inside of a binder" 2) rename this method to probe_adt_normalize_for_backcompat to prevent us from at least adding further uses of that method.

edit: we do normalize for resolve_fully_qualified_call as well it's not as simple as only warning on this method 🤔 idk

[aliemjay]

This looks like T-lang issue. Can we open a new issue and nominate it for t-lang?

Anyway I toned down the FIXME comment and opened a new ticket for it #104767. See also #103640.

[lcnr]

Thanks ❤️

As this is a breaking change, going to run crater and then go through a t-types FCP. r=me on the impl after some nits

@bors try

[bors]

⌛ Trying commit a1a29a30952de6b1aed11075b0979825595c5275 with merge c4ca63acfd7111dc43e5bc36534474f323ca449a...

[bors]

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

[lcnr]

@bors r+ rollup=never

[bors]

📌 Commit bf228ac has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit bf228ac with merge af58fc8...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing af58fc8 to master...

[rust-timer]

Finished benchmarking commit (af58fc8): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	3
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-0.8%	[-0.9%, -0.8%]	7
All ❌✅ (primary)	-0.5%	[-1.0%, 0.1%]	2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

[Mark-Simulacrum]

Mixed results, but overall small in magnitude and relatively scattered. Marking as triaged -- I don't think we need to dig in further. This change fixes a couple bugs, so it's worthwhile to accept small regressions like this.