Require TAITs to appear in the signature of items that register a hidden type #107073

oli-obk · 2023-01-19T14:51:37Z

This PR changes which items can register hidden types for TAITs. The logic enables individual sites and rejects all other sites by default, so we can't accidentally allow something, even if we can accidentally forbid something. It is not backwards compatible to add or remove sites later, as adding more sites may cause additional cycle errors where there were none before, so we need to get all the sites right before stabilization.

At present we allow

constants and statics that mention the opaque type in their type
functions that mention the opaque type in their signature or where bounds
bodies within which the TAIT is declared (e.g. fn foo() { type Foo = impl Send; let _: Foo = (); }

Note that for associated items this may require you to repeat where bounds found on the associated item's container, as we are not looking into parents' bounds or signatures.

Discussion: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/TAITs.20and.20lazy.20parsing

Tracking issue: #63063

Corresponding stabilization concern: #63063 (comment)

rustbot · 2023-01-19T14:51:49Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nilstrieb (or someone else) soon.

Please see the contribution instructions for more information.

rustbot · 2023-01-19T15:20:38Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

tests/ui/feature-gates/feature-gate-type_alias_impl_trait.rs

…den type

WaffleLapkin

So my main question is

What exactly counts as a "mention" of a TAIT?

I think there are 3 ways to "mention" a type:

Directly — Tait: Bound, ...: Bound<Tait>, arg: Tait, -> Tait
Syntactically — as an unused generic of an alias
Semantically — as a field of another type

Examples:

// "Direct"
fn f() -> Tait { 17 }

// "Syntactic"
type DropType<T> = ();
fn g() -> DropType<Tait> { let _: Tait = 42; }

// "Semantic"
struct Wrap(Tait);
fn h() -> Wrap { Wrap(f()) }

Which of these are considered as mention of the tait with the current implementation and which of these do we want to consider as such (cc @Veykril)?

tests/ui/impl-trait/issues/issue-74282.rs

tests/ui/type-alias-impl-trait/issue-63263-closure-return.rs

tests/ui/type-alias-impl-trait/nested_type_alias_impl_trait.rs

oli-obk · 2023-01-19T17:04:36Z

Which of these are considered as mention of the tait with the current implementation

All of these are considered as mentions of the TAIT

WaffleLapkin · 2023-01-19T18:36:47Z

After some thought: this is probably the only reasonable way to define mentiondness for this use-case. Disallowing "direct" and "syntactic" doesn't help anyone I don't think and disallowing "semantic" cuts into real use-cases, so probably a bad idea too.

Veykril · 2023-01-20T11:06:34Z

One thing I just realized, what about the following case:

type Foo = impl Send;
struct Struct<
    const C: usize = {
        let _: Foo = ();
        0
    }
>;

Are we considering const parameter defaults as part of the signature? (I would hope not, since this is another body we would otherwise have to look into). Hence raising this just to make sure that this isn't being missed. (It does fail to compile on current nightly)

oli-obk · 2023-01-20T13:16:58Z

This PR only rejects more defining use sites. The only cases that didn't compile before and do now are

cycle errors if the auto trait of a Tait is requested in a function without the Tait in the signature.
TAITs where one TAIT is the hidden type of another can work if only one of the TAITs is in the signature.

WaffleLapkin

I would like to see more tests (which for example explicitly tests the cases we've discussed and the new restrictions), but generally this looks good.

The part that we can't change the restrictions either way is a bit scary, but I guess there is no way around it. Also, we could probably make the change over an edition (and basically have enum TaitDefScopesStyle { V1, V2 }), given that the changes to def scopes are crate (and even module) local.

r=me when the tests are ready

oli-obk · 2023-01-23T17:53:28Z

During testing I realized I missed fixing the main cycle error that I wanted to fix (others were fixed).

Have another look?

@rustbot review

jackh726 · 2023-01-23T18:37:28Z

I think we should do a crater run of this to assess the impact (even though I think we probably land it either way).

oli-obk · 2023-01-23T20:25:09Z

I think we should do a crater run of this to assess the impact (even though I think we probably land it either way).

To check how this affects unstable users?

veber-alex · 2023-01-23T21:35:04Z

What about the following use case? I assume it will stop working now without a helper function?

#![feature(type_alias_impl_trait)]

type MyIter = impl Iterator<Item = i32>;

struct Foo {
    iter: MyIter,
}

impl Foo {
    fn set_iter(&mut self) {
        self.iter = [1, 2, 3].into_iter();
    }
}

jackh726 · 2023-01-24T01:13:23Z

To check how this affects unstable users?

Somewhat, more so to find out if there are any use cases that we're missing that get used in the wild.

What about the following use case? I assume it will stop working now without a helper function?

The problem is that somewhere, you will need to have constructed a Foo (the one getting passed as &mut self), and during that construction, you would have had to have returned a Foo.

That being said, would have to check, but this might count as a defining use function, since &mut self should desugar to self: &mut Self, which is self: &mut Foo, which is the "semantic mention" talked about above. Would be a good test...

WaffleLapkin · 2023-01-24T08:30:43Z

@veber-alex it should continue to work (it's a good candidate for a test tho), since the signature does mention MyIter through self.

The problem is that somewhere, you will need to have constructed a Foo (the one getting passed as &mut self), and during that construction, you would have had to have returned a Foo.

You could change it to iter: Option<MyIter> and initialize it with None originally, as an example.

WaffleLapkin · 2023-01-24T08:37:45Z

re: crater, do we have any way to filter targets to only include the ones which include #![feature(type_alias_impl_trait)], so we don't waste time on unrelated crates?

oli-obk · 2023-01-24T10:38:16Z

@bors try

bors · 2023-01-24T10:38:26Z

⌛ Trying commit 28c5fe7 with merge db398c5410e92f4a42a260668961bb25e5d2b858...

bors · 2023-01-24T12:49:02Z

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

oli-obk · 2023-01-24T13:39:38Z

@craterbot check

craterbot · 2023-01-24T13:39:49Z

👌 Experiment pr-107073 created and queued.
🤖 Automatically detected try build db398c5410e92f4a42a260668961bb25e5d2b858
🔍 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 · 2023-01-24T13:40:19Z

🚧 Experiment pr-107073 is now running

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

craterbot · 2023-01-25T10:19:40Z

🎉 Experiment pr-107073 is completed!
📊 23 regressed and 18 fixed (253470 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

oli-obk · 2023-01-25T10:31:34Z

There was one legit failure: bind_it generates TAITs with proc macros, and I don't see why it should stop working. Need to look at the desugarings.

WaffleLapkin · 2023-01-24T10:05:45Z

tests/ui/type-alias-impl-trait/inference-cycle.rs

+    fn baz() -> Foo {
        let f: Foo = 22_u32;
+        //~^ ERROR concrete type differs
+        f
    }


Why baz is here in the first place? The cycle is reproducible without it: [play].

Also the comments are now outdated! :3

WaffleLapkin · 2023-01-24T10:08:06Z

tests/ui/type-alias-impl-trait/impl_trait_for_tait_bound2.rs

+    type Foo = impl Debug;
    is_yay::<Foo>(); //~ ERROR: the trait bound `Foo: Yay` is not satisfied


It's weird that this type doesn't define Foo anywhere, was it meant to do

type Foo = impl Debug; let _: Foo = 0u32;

In order to check that even if Foo's internal type implements a trait, Foo itself doesn't?

WaffleLapkin · 2023-01-24T10:13:10Z

tests/ui/type-alias-impl-trait/nested.rs

@@ -1,6 +1,7 @@
 #![feature(type_alias_impl_trait)]

 type Foo = impl std::fmt::Debug;
+//~^ ERROR: unconstrained opaque type


How was it constrained before? O_o

WaffleLapkin · 2023-01-27T11:06:32Z

tests/ui/type-alias-impl-trait/defining-scopes-syntactic.rs

+// Check that we cannot syntactically mention a TAIT without
+// it also being part of the type.
+
+type DropType<T> = ();


TBH this doesn't really make sense as a test, since type DropType<T> = (); does not compile.

Other interesting-ish types to check would be projections I think (<Foo as Id>::Out, <() as Map<Foo>>::Out).

I wanted to keep it as a sanity check

Noratrieb · 2023-02-02T10:58:32Z

you wanted waffle to review it in the first place and assigned me on accident, so
r? WaffleLapkin

vlad20012 · 2023-02-04T09:11:51Z

Does this PR forbid register hidden type for a TAIT via a nested function return type while the TAIT is not mentioned in the root function signature?

#![feature(type_alias_impl_trait)]
type Alias = impl Default;

pub fn foo() {
    #[derive(Default)] struct Foo;
    fn _f() -> Alias { Foo }
}

pub fn bar() -> Alias {
    Alias::default()
}

oli-obk · 2023-02-04T10:51:48Z

Does this PR forbid register hidden type for a TAIT via a nested function return type while the TAIT is not mentioned in the root function signature?

no, that will compile

Veykril · 2023-02-04T11:54:46Z

That makes this PR kinda moot in regards to lazy parsing, since you still have to look into all bodies to check for defining uses due to local items.

oli-obk · 2023-02-04T12:00:29Z

That makes this PR kinda moot in regards to lazy parsing, since you still have to look into all bodies to check for defining uses due to local items.

I assumed that nested items were problematic like that anyway out of other reasons.

Veykril · 2023-02-04T12:03:25Z

They are, I guess that part for TAITs should be addressed by rust-lang/rfcs#3373 then, right.

WaffleLapkin · 2023-02-10T07:19:36Z

no, that will compile

I assumed that nested items were problematic like that anyway out of other reasons.

@oli-obk while that's true, I think it's sensible to not introduce new problems, especially given that we might resolve the old ones. I'd like that snippet to not compile, at least for the initial stabilization.

oli-obk · 2023-03-10T15:41:12Z

closing in favor of #107809 as per the lang team decision on #107645

rustbot assigned Noratrieb Jan 19, 2023

oli-obk force-pushed the tait_defining_scopes_in_sig branch from 5e391a5 to 1a8d017 Compare January 19, 2023 15:20

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 19, 2023

jackh726 reviewed Jan 19, 2023

View reviewed changes

tests/ui/feature-gates/feature-gate-type_alias_impl_trait.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Require TAITs to appear in the signature of items that register a hid…

b232bb8

…den type

oli-obk force-pushed the tait_defining_scopes_in_sig branch from 1a8d017 to b232bb8 Compare January 19, 2023 16:24

WaffleLapkin reviewed Jan 19, 2023

View reviewed changes

tests/ui/impl-trait/issues/issue-74282.rs Outdated Show resolved Hide resolved

tests/ui/type-alias-impl-trait/issue-63263-closure-return.rs Outdated Show resolved Hide resolved

tests/ui/type-alias-impl-trait/nested_type_alias_impl_trait.rs Show resolved Hide resolved

WaffleLapkin approved these changes Jan 20, 2023

View reviewed changes

oli-obk added 4 commits January 23, 2023 16:50

Add some more tests

3f0b21f

Remove an unnecessary argument

ddfdbe0

Remove another unneeded argument

6e354fb

Actually prevent cycle errors

b105e2a

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2023

This comment has been minimized.

Sign in to view

oli-obk added 2 commits January 24, 2023 10:33

Generalize infinite recursion avoidance logic

0d6dcd0

Add a regression test

28c5fe7

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 Jan 24, 2023

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 Jan 25, 2023

WaffleLapkin reviewed Jan 27, 2023

View reviewed changes

WaffleLapkin 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 Jan 27, 2023

rustbot assigned WaffleLapkin and unassigned Noratrieb Feb 2, 2023

oli-obk mentioned this pull request Feb 3, 2023

TAIT defining scope options #107645

Open

vlad20012 mentioned this pull request Feb 4, 2023

Avoid non-local definitions in functions rust-lang/rfcs#3373

Merged

oli-obk mentioned this pull request Feb 8, 2023

Require TAITs to appear in the signature of items that register a hidden type #107809

Closed

oli-obk closed this Mar 10, 2023

		type Foo = impl Debug;
		is_yay::<Foo>(); //~ ERROR: the trait bound `Foo: Yay` is not satisfied

Require TAITs to appear in the signature of items that register a hidden type #107073

Require TAITs to appear in the signature of items that register a hidden type #107073

Conversation

oli-obk commented Jan 19, 2023 • edited Loading

rustbot commented Jan 19, 2023

rustbot commented Jan 19, 2023

This comment has been minimized.

WaffleLapkin left a comment

Choose a reason for hiding this comment

oli-obk commented Jan 19, 2023

WaffleLapkin commented Jan 19, 2023

Veykril commented Jan 20, 2023 • edited Loading

oli-obk commented Jan 20, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

oli-obk commented Jan 23, 2023

jackh726 commented Jan 23, 2023

This comment has been minimized.

oli-obk commented Jan 23, 2023

veber-alex commented Jan 23, 2023

jackh726 commented Jan 24, 2023

WaffleLapkin commented Jan 24, 2023 • edited Loading

WaffleLapkin commented Jan 24, 2023

oli-obk commented Jan 24, 2023

bors commented Jan 24, 2023

bors commented Jan 24, 2023

oli-obk commented Jan 24, 2023

craterbot commented Jan 24, 2023

craterbot commented Jan 24, 2023

craterbot commented Jan 25, 2023

oli-obk commented Jan 25, 2023

WaffleLapkin Jan 24, 2023

Choose a reason for hiding this comment

WaffleLapkin Jan 24, 2023

Choose a reason for hiding this comment

WaffleLapkin Jan 24, 2023

Choose a reason for hiding this comment

WaffleLapkin Jan 27, 2023

Choose a reason for hiding this comment

oli-obk Feb 2, 2023

Choose a reason for hiding this comment

Noratrieb commented Feb 2, 2023

vlad20012 commented Feb 4, 2023 • edited Loading

oli-obk commented Feb 4, 2023

Veykril commented Feb 4, 2023 • edited Loading

oli-obk commented Feb 4, 2023

Veykril commented Feb 4, 2023

WaffleLapkin commented Feb 10, 2023 • edited Loading

oli-obk commented Mar 10, 2023

oli-obk commented Jan 19, 2023 •

edited

Loading

Veykril commented Jan 20, 2023 •

edited

Loading

WaffleLapkin commented Jan 24, 2023 •

edited

Loading

vlad20012 commented Feb 4, 2023 •

edited

Loading

Veykril commented Feb 4, 2023 •

edited

Loading

WaffleLapkin commented Feb 10, 2023 •

edited

Loading