wf-check user types before normalization #104746

aliemjay · 2022-11-22T22:04:11Z

Builds on #101947.
See the last two commits.

We delay projection normalization to further stages in order to register user type annotations before normalization in HIR typeck. There are two consumers of astconv: ItemCtxt and FnCtxt. The former already expects unnormalized types from astconv, see its AstConv trait impl. The latter needs `RawTy` for a cleaner interface. Unfortunately astconv still needs the normalization machinery in order to resolve enum variants that have projections in the self type, e.g. `<<T as Trait>::Assoc>::StructVariant {}`. This is why `AstConv::normalize_ty_2` is necessary.

Projection types in user annotations may contain inference variables. This makes the normalization depend on the unification with the actual type and thus requires a separate TypeOp to track the obligations. Otherwise simply calling `TypeChecker::normalize` would ICE with "unexpected ambiguity"

aliemjay · 2022-11-22T22:06:50Z

@rustbot label -S-waiting-on-review +S-blocked

lqd · 2022-11-23T09:08:15Z

@bors try

bors · 2022-11-23T09:08:25Z

⌛ Trying commit 3ba10e2 with merge 4c35efa5bf9c956a3bd39021cc9699d125584977...

bors · 2022-11-23T11:17:03Z

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

lcnr · 2022-11-23T12:02:22Z

@craterbot check

craterbot · 2022-11-23T12:02:32Z

👌 Experiment pr-104746 created and queued.
🤖 Automatically detected try build 4c35efa5bf9c956a3bd39021cc9699d125584977
🔍 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 · 2022-11-23T12:03:01Z

🚧 Experiment pr-104746 is now running

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

craterbot · 2022-11-24T09:50:31Z

🎉 Experiment pr-104746 is completed!
📊 102 regressed and 16 fixed (248749 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

lqd · 2022-11-24T10:18:12Z

As the queue is empty, let's do another quick run to filter out transient regressions

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-104746/retry-regressed-list.txt

craterbot · 2022-11-24T10:18:18Z

👌 Experiment pr-104746-1 created and queued.
🤖 Automatically detected try build 4c35efa5bf9c956a3bd39021cc9699d125584977
🔍 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 · 2022-11-24T10:18:22Z

🚧 Experiment pr-104746-1 is now running

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

craterbot · 2022-11-24T10:30:48Z

🎉 Experiment pr-104746-1 is completed!
📊 34 regressed and 1 fixed (102 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

JohnCSimon · 2023-03-17T04:12:16Z

@aliemjay

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

aliemjay · 2023-11-13T22:44:26Z

For the record, apart from cellular4 which needs more investigation, all other crater regressions can be mitigated by fixing #98852 first and the basic pattern boils down to Self::AssocTy not being WF within static associated functions:

trait Trait {
    type Assoc;
    fn assoc();
}

impl<T> Trait for &'static T {
    type Assoc = ();
    fn assoc() {
        let _: Self::Assoc = (); //~ ERROR
        // cannot prove T: 'static
    }
}

fix fn/const items implied bounds and wf check These are two distinct changes (edit: actually three, see below): 1. Wf-check all fn item args. This is a soundness fix. Fixes rust-lang#104005 2. Use implied bounds from impl header in borrowck of associated functions/consts. This strictly accepts more code and helps to mitigate the impact of other breaking changes. Fixes rust-lang#98852 Fixes rust-lang#102611 The first is a breaking change and will likely have a big impact without the the second one. See the first commit for how it breaks libstd. Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as: ```rust use core::fmt::Display; trait ExtendLt<Witness> { fn extend(self) -> Box<dyn Display>; } impl<T: Display> ExtendLt<&'static T> for T { fn extend(self) -> Box<dyn Display> { Box::new(self) } } fn main() { let val = (&String::new()).extend(); println!("{val}"); } ``` The third change is to to check WF of user type annotations before normalizing them (fixes rust-lang#104764, fixes rust-lang#104763). It is mutually dependent on the second change above: an attempt to land it separately in rust-lang#104746 caused several crater regressions that can all be mitigated by using the implied from the impl header. It is also necessary for the soundness of associated consts that use the implied bounds of impl header. See rust-lang#104763 and how the third commit fixes the soundness issue in `tests/ui/wf/wf-associated-const.rs` that was introduces by the previous commit. cc `@lcnr` r? types

fix fn/const items implied bounds and wf check (rebase) A rebase of rust-lang#104098, see that PR for discussion. This is pretty much entirely the work of `@aliemjay.` I received his permission for this rebase. --- These are two distinct changes (edit: actually three, see below): 1. Wf-check all fn item args. This is a soundness fix. Fixes rust-lang#104005 2. Use implied bounds from impl header in borrowck of associated functions/consts. This strictly accepts more code and helps to mitigate the impact of other breaking changes. Fixes rust-lang#98852 Fixes rust-lang#102611 The first is a breaking change and will likely have a big impact without the the second one. See the first commit for how it breaks libstd. Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as: ```rust use core::fmt::Display; trait ExtendLt<Witness> { fn extend(self) -> Box<dyn Display>; } impl<T: Display> ExtendLt<&'static T> for T { fn extend(self) -> Box<dyn Display> { Box::new(self) } } fn main() { let val = (&String::new()).extend(); println!("{val}"); } ``` The third change is to to check WF of user type annotations before normalizing them (fixes rust-lang#104764, fixes rust-lang#104763). It is mutually dependent on the second change above: an attempt to land it separately in rust-lang#104746 caused several crater regressions that can all be mitigated by using the implied from the impl header. It is also necessary for the soundness of associated consts that use the implied bounds of impl header. See rust-lang#104763 and how the third commit fixes the soundness issue in `tests/ui/wf/wf-associated-const.rs` that was introduces by the previous commit. r? types

aliemjay added 8 commits November 22, 2022 15:41

introduce AstConv::probe_adt

4327a79

fix struct path

32b4b8a

fix method substs

2ee771f

remove unnecessary normalize call

e8cde83

check wf before normalization

e6dc882

tests

8e40dde

rustbot assigned lcnr Nov 22, 2022

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2022

This comment has been minimized.

Sign in to view

aliemjay force-pushed the wf-check-prior branch from 6ff9244 to 66f4d02 Compare November 23, 2022 04:44

quick fixes

3ba10e2

aliemjay force-pushed the wf-check-prior branch from 66f4d02 to 3ba10e2 Compare November 23, 2022 05:01

lcnr mentioned this pull request Nov 23, 2022

Don't normalize in AstConv #101947

Merged

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 23, 2022

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 24, 2022

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2022

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 24, 2022

This comment was marked as off-topic.

Sign in to view

rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023

lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 15, 2023

JohnCSimon closed this Mar 17, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 17, 2023

aliemjay deleted the wf-check-prior branch May 10, 2023 15:22

aliemjay mentioned this pull request Nov 7, 2023

Don't check GAT bounds in normalization #117682

Draft

aliemjay mentioned this pull request Nov 13, 2023

fix fn/const items implied bounds and wf check #104098

Closed

lcnr mentioned this pull request Jan 16, 2024

fix fn/const items implied bounds and wf check (rebase) #120019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wf-check user types before normalization #104746

wf-check user types before normalization #104746

aliemjay commented Nov 22, 2022 •

edited

Loading

aliemjay commented Nov 22, 2022

This comment has been minimized.

lqd commented Nov 23, 2022

bors commented Nov 23, 2022

bors commented Nov 23, 2022

lcnr commented Nov 23, 2022

craterbot commented Nov 23, 2022

craterbot commented Nov 23, 2022

craterbot commented Nov 24, 2022

lqd commented Nov 24, 2022

craterbot commented Nov 24, 2022

craterbot commented Nov 24, 2022

craterbot commented Nov 24, 2022

This comment was marked as off-topic.

JohnCSimon commented Mar 17, 2023

aliemjay commented Nov 13, 2023

wf-check user types before normalization #104746

wf-check user types before normalization #104746

Conversation

aliemjay commented Nov 22, 2022 • edited Loading

aliemjay commented Nov 22, 2022

This comment has been minimized.

lqd commented Nov 23, 2022

bors commented Nov 23, 2022

bors commented Nov 23, 2022

lcnr commented Nov 23, 2022

craterbot commented Nov 23, 2022

craterbot commented Nov 23, 2022

craterbot commented Nov 24, 2022

lqd commented Nov 24, 2022

craterbot commented Nov 24, 2022

craterbot commented Nov 24, 2022

craterbot commented Nov 24, 2022

This comment was marked as off-topic.

JohnCSimon commented Mar 17, 2023

aliemjay commented Nov 13, 2023

aliemjay commented Nov 22, 2022 •

edited

Loading