Don't allow opaque ty aliases in ADT fields

[b-naber]

Fixes #86732

[rust-highfive]

r? @davidtwco

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

[davidtwco]

Looking good, some minor changes.

[davidtwco]

It might be worthwhile to put the name of the type alias in this message, if the alias is part of some larger nested type then identifying it might be helpful.

e.g. this field contains a type alias impl trait: `X`

[jackh726]

So this diagnostic should only be emitted in min_type_alias_impl_trait, but not in the full version.

[b-naber]

@davidtwco Thanks for the review and sorry for the late update. Previously this didn't allow nested type alias impl trait types to be caught, I added a visitor for this. Also tried to improve the error labels for the nested types.

[b-naber]

@jackh726 Since rejecting impl trait type aliases requires min_type_alias_impl_trait to be used, this is implicitly behind the feature gate.

[davidtwco]

LGTM, I've left some nitpicks regarding the exact error messages that we're adding here.

I wasn't sure if "type alias impl traits are not allowed as field types in enums" is something someone who is new to Rust and have stumbled upon this error when trying things out would understand. If nothing else, I believe we normally refer to "impl traits" as `impl Trait` in diagnostics, and it would be good if that were consistent. You don't need to accept these exact suggestions if you have better alternatives.

[b-naber]

@davidtwco Fixed.

[davidtwco]

@bors r+

[bors]

📌 Commit 21548b4 has been approved by davidtwco

[jackh726]

@bors r-

Hold on, this still isn't correct. We only want to emit this when there is only min_type_alias_impl_trait and not when there is also type_alias_impl_trait (i.e. we don't allow this in min, but do allow this in full; that is what #86732 is for)

For example, src/test/ui/mir/issue-75053.rs emits this error even in the full_tait revision.

[b-naber]

Isn't the functionality in min_type_alias_impl_trait a subset of that in type_alias_impl_trait? How could we then throw an error in min_type_alias_trait but not in type_alias_impl_trait?

[jackh726]

Yes. Just check if the type_alias_impl_trait feature gate is active. If it is, don't do this check.

[b-naber]

Ok did this. Can anybody maybe help me with the attributes in the tests, so that the error is not thrown in the full_tait version of the test. E.g. in src/test/ui/mir/issue-75053.rs we currently use:

// revisions: min_tait full_tait in_bindings
#![feature(min_type_alias_impl_trait, rustc_attrs)]
#![cfg_attr(full_tait, feature(type_alias_impl_trait))]
//[full_tait]~^ WARN incomplete
#![cfg_attr(in_bindings, feature(impl_trait_in_bindings))]
//[in_bindings]~^ WARN incomplete

How can I not include min_type_alias_impl_trait in the full_tait version of the test?

I currently use //[min_tait]~^ ERROR type aliases of impl Trait are not allowed as field types in structs for the error annotation.

[jackh726]

@b-naber I think that looks correct. In the min revision, we would see an error. in the full_tait revision, if it passes otherwise, you'll need a rustc_error attribute so it also fails.

[b-naber]

Doesn't work. I suppose what you want is the field error not to be thrown if only the type_alias_impl_trait feature attribute is included and not min_type_alias_impl_trait, is that correct?

There doesn't seem to be a way to conditionally include a feature attribute.

[jackh726]

As it currently is, if you want to use type Foo = impl Trait at all, you have to have min_type_alias_impl_trait. However, there are a number of features (including this one) that you also need to have type_alias_impl_trait.

In the tests, you either have min or min + full. In min-only, we want an error. In min + full, we do not. There shouldn't be a test/revision where we enable full but not min.

I'm confused what you're having problems with?

[b-naber]

if you want to use type Foo = impl Trait at all, you have to have min_type_alias_impl_trait

Yes, that's my point. I don't see how min + full could possibly not throw an error then. We throw the field error when min_type_alias_impl_trait is active.

[jackh726]

So, whether than (just) checking in min is active, you should explicitly check that full is not active.

[b-naber]

@jackh726 That would make #tait_full tests compile, but it still doesn't seem correct to me. If we handle it like that it's only implicitly behind the min_type_alias_impl_trait feature gate. I don't understand the logic behind handling it like this. Suppose we transition from min_type_alias_impl_trait to type_alias_impl_trait and still want to perform this check in type_alias_impl_trait, then this implementation would avoid this check in a very implicit way when you use the type_alias_impl_trait feature attribute.

[jackh726]

@jackh726 That would make #tait_full tests compile, but it still doesn't seem correct to me. If we handle it like that it's only implicitly behind the min_type_alias_impl_trait feature gate. I don't understand the logic behind handling it like this. Suppose we transition from min_type_alias_impl_trait to type_alias_impl_trait and still want to perform this check in type_alias_impl_trait, then this implementation would avoid this check in a very implicit way when you use the type_alias_impl_trait feature attribute.

I'm really not following. Full tait is a superset of min tait. As such, there are things that will be implemented but not allowed with only min. This is one of those things. In fact, this feature is only allowed when full tait is enabled. There is no extra condition regarding min tait (it being on, stabilized, or off doesn't affect this PR).

[b-naber]

there are things that will be implemented but not allowed with only min. This is one of those things. In fact, this feature is only allowed when full tait is enabled

Ok, I think my confusion stems from not knowing the reason why we want to reject these opaque ty aliases in fields in the first place or rather what features of full tait would later make it ok to use them in fields. I was assuming that if we reject them in min tait, we would also have to do that in full tait.

I'll update the PR later, don't have time right now.

[b-naber]

Updated. One unexpected thing I encountered is that the following now throws an error in the full_tait version:

extern "C" {
    pub fn lint_me() -> A;
    //[full_tait]~^ ERROR `extern` block uses type `impl Baz`, which is not FFI-safe
}

[bors]

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

[jackh726]

Oof, I missed that this was updated. Unfortunately, looks like there's been some movement on the TAIT work that essentially means that min_type_alias_impl_trait will be no more. (#87501 removes the feature gate, but there's some discussion on zulip).

So...we should probably just close this. Sorry @b-naber! I really appreciate the effort here :/