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

oli-obk · 2023-02-08T16:47:26Z

We now error in case an opaque type gets constrained in a function/item that doesn't have the opaque type in its return type or argument types (Option 5 of #107645):

Any item in the defining scope can register hidden types, but we error out if the TAIT is not in the return type or argument types (post normalization, see below).
We still look at all functions in the defining scope, there's no change in cycle errors.

This is forward compatible to all other designs.

We normalize the argument types and return type before checking for opaque types in order to allow cases like

impl Iterator for Foo {
    type Item = impl Debug;
    fn next(&mut self) -> Option<Self::Item> {
        ...
    }
}

This means if you have a -> <TAIT as Trait>::AssocType that is not a constraining use and will error for now. We can later check both pre and post-normalization to allow that, too. The current design is entirely forward compatible with it.

rustbot · 2023-02-08T16:47:35Z

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

Please see the contribution instructions for more information.

rustbot · 2023-02-08T16:47:39Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

jackh726 · 2023-02-10T01:16:45Z

Can you talk a bit about what's different between this and #107073?

oli-obk · 2023-02-10T08:01:29Z

Require TAITs to appear in the signature of items that register a hidden type #107073
- treat all functions without the TAIT in their signature as non-defining, thus using the opaque type opaquely
this PR
- no change to master in what functions can do in their body, but we then error out if they did end up registering a hidden type but didn't have the TAIT in their signature.

the difference is a bit subtle, but basically this PR avoids any risk of having to do backwards incompatible changes later, as it is forwards compatible with the other PR. So we can in the future decide to go with a more permissive implementation.

The guarantees for IDEs are the same, they don't need to look at any items that don't have the TAIT in its signature.

bors · 2023-02-10T14:34:42Z

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

pnkfelix · 2023-02-14T19:26:49Z

@oli-obk can you put a precise writeup of the semantics for the lang team to review, either on this PR, or as a comment/description-edit on #107645 ?

In particular, "we then error out if they did end up registering a hidden type but didn't have the TAIT in their signature" is open to interpretation about whether the where clauses of a function are part of its signature or not. (At least, I know Niko has sometimes considered the where-clauses to have a somewhat less significant role compared to e.g. the types of the formal parameters or return type.)

oli-obk · 2023-02-16T16:32:04Z

I updated #107645 (which already explicitly mentioned where bounds, but I made the non-where bound option its own option)

rustbot · 2023-03-07T07:49:47Z

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

cjgillot

Sorry for the (very) slow review @oli-obk. I don't have much to say, mostly a few questions about tests.

cjgillot · 2023-03-12T14:15:11Z

compiler/rustc_hir_analysis/src/collect/type_of.rs

+        return true;
+    }
+
+    if tcx.is_closure(def_id.to_def_id()) {


Should this only apply for closures, or also for inline consts? IOW, should we check is_typeck_child here?

Is there a test for the ban on closure signatures? I saw tests/ui/type-alias-impl-trait/issue-63263-closure-return.rs is changed to accommodate it, but I missed the "fail" test.

You're right on all accounts. There were no tests, and there were especially none for inline consts. I added them and fixed it so that inline consts actually work at all.

cjgillot · 2023-03-12T14:24:20Z

compiler/rustc_hir_typeck/src/lib.rs

+        // In case we are inferring the return signature (via `_` types), ignore defining use
+        // rules, as we'll error out anyway. This helps improve diagnostics, which otherwise
+        // may just see `ty::Error` instead of `ty::Alias(Opaque, _)` and not produce better errors.
+        if fn_sig_infer { DefiningAnchor::Bubble } else { DefiningAnchor::Bind(def_id) }


Which are the diagnostics changed by this?

An example is

type Pointer<T> = impl std::ops::Deref<Target=T>; fn test() -> Pointer<_> { //~^ ERROR: the placeholder `_` is not allowed within types Box::new(1) }

which produces the following error:

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types --> $DIR/issue-77179.rs:7:22 | LL | fn test() -> Pointer<_> { | --------^- | | | | | not allowed in type signatures | help: replace with the correct return type: `Pointer<i32>`

but without the change it would not see Pointer<i32> but just TyKind::Error and thus not report any suggestion, even if the main error would still be emitted

isn't using Pointer<i32> forbidden here anyways, it currently results in the following (bad) error: playground

error[E0792]: expected generic type parameter, found `i32` --> src/lib.rs:5:5 | 2 | type Pointer<T> = impl std::ops::Deref<Target = T>; | - this generic parameter must be used with a generic type parameter ... 5 | Box::new(1) | ^^^^^^^^^^^

i personally feel like this is not worth it as it imo makes the code noticeably more confusing for a fairly small benefit.

bors · 2023-03-14T14:27:51Z

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

…en type

…l feature gate for the other cases

…e that is supposed to be restricted

lcnr

by looking into adts you can probably get an infinite loop if you have the following type in the signature

struct Foo<T: 'static> {
    field: &'static Foo<(T,)>,
    other_field: T,
}

compiler/rustc_hir_analysis/src/collect/type_of.rs

lcnr · 2023-03-21T10:34:17Z

compiler/rustc_hir_analysis/src/collect/type_of.rs

+                    tcx: self.tcx,
+                    ct_op: |c| c,
+                    ty_op: |t| t,
+                    lt_op: |_| self.tcx.lifetimes.re_erased,


shouldn't erase bound regions here, only escaping bound regions 🤔

you could implement fn fold_binder to call https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.replace_bound_vars_uncached, replacing bound vars with self.tcx.lifetime.re_erased and eagerly replace all free regions that way

lcnr · 2023-03-21T10:36:03Z

compiler/rustc_hir_analysis/src/collect/type_of.rs

+                    }
+                    // Types that have opaque type fields must get walked manually, they
+                    // would not be seen by the type visitor otherwise.
+                    ty::Adt(adt_def, substs) => {


so the type can be contained in an Adt? please add that to the PR description.

also, this can be adt_def.all_fields instead of the variants -> fields nested loop

lcnr · 2023-03-21T10:40:59Z

compiler/rustc_hir_typeck/src/lib.rs

+        // In case we are inferring the return signature (via `_` types), ignore defining use
+        // rules, as we'll error out anyway. This helps improve diagnostics, which otherwise
+        // may just see `ty::Error` instead of `ty::Alias(Opaque, _)` and not produce better errors.
+        if fn_sig_infer { DefiningAnchor::Bubble } else { DefiningAnchor::Bind(def_id) }


isn't using Pointer<i32> forbidden here anyways, it currently results in the following (bad) error: playground

error[E0792]: expected generic type parameter, found `i32` --> src/lib.rs:5:5 | 2 | type Pointer<T> = impl std::ops::Deref<Target = T>; | - this generic parameter must be used with a generic type parameter ... 5 | Box::new(1) | ^^^^^^^^^^^

i personally feel like this is not worth it as it imo makes the code noticeably more confusing for a fairly small benefit.

compiler/rustc_hir_analysis/src/collect/type_of.rs

oli-obk · 2023-04-06T16:21:15Z

Closing in favor of #110010

rustbot assigned cjgillot Feb 8, 2023

oli-obk mentioned this pull request Feb 16, 2023

TAIT defining scope options #107645

Open

oli-obk force-pushed the reject_tait_defining_scopes_unless_in_sig branch from 7798e7c to 8552b71 Compare March 7, 2023 07:49

oli-obk force-pushed the reject_tait_defining_scopes_unless_in_sig branch 2 times, most recently from ec0331c to c83b23d Compare March 7, 2023 08:29

oli-obk mentioned this pull request Mar 10, 2023

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

Closed

cjgillot reviewed Mar 12, 2023

View reviewed changes

oli-obk force-pushed the reject_tait_defining_scopes_unless_in_sig branch from 9119da8 to b465336 Compare March 14, 2023 09:47

oli-obk added 4 commits March 14, 2023 15:08

equire TAITs to appear in the signature of items that register a hidd…

73cfef3

…en type

Restrict TAITs to argument and return types only and add an additiona…

0e5d7b8

…l feature gate for the other cases

Add a test for sibling items in trait impls

86a360d

Make closures and anon consts report that their parent item is the on…

375dfa4

…e that is supposed to be restricted

oli-obk force-pushed the reject_tait_defining_scopes_unless_in_sig branch from b465336 to 375dfa4 Compare March 14, 2023 15:09

oli-obk added 2 commits March 14, 2023 21:29

Remove a bogus file

bd86dcb

Fix a typo

d715644

This comment has been minimized.

Sign in to view

Prevent defining opaque types in nested items

733e444

oli-obk force-pushed the reject_tait_defining_scopes_unless_in_sig branch from 4c4d729 to 733e444 Compare March 15, 2023 11:57

lcnr reviewed Mar 21, 2023

View reviewed changes

oli-obk mentioned this pull request Apr 6, 2023

Require a #[defines] attribute for type-alias-impl-trait in order for a function to be able to register hidden types. #110010

Closed

6 tasks

oli-obk closed this Apr 6, 2023

traviscross mentioned this pull request Apr 25, 2023

TAIT defining scope options rust-lang/lang-team#206

Closed

oli-obk deleted the reject_tait_defining_scopes_unless_in_sig branch May 30, 2023 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oli-obk commented Feb 8, 2023 •

edited

Loading

rustbot commented Feb 8, 2023

rustbot commented Feb 8, 2023

jackh726 commented Feb 10, 2023

oli-obk commented Feb 10, 2023

bors commented Feb 10, 2023

pnkfelix commented Feb 14, 2023

oli-obk commented Feb 16, 2023

rustbot commented Mar 7, 2023

cjgillot left a comment

cjgillot Mar 12, 2023

cjgillot Mar 12, 2023

oli-obk Mar 14, 2023

cjgillot Mar 12, 2023

oli-obk Mar 14, 2023

lcnr Mar 21, 2023

bors commented Mar 14, 2023

This comment has been minimized.

lcnr left a comment

lcnr Mar 21, 2023

lcnr Mar 21, 2023

lcnr Mar 21, 2023

lcnr Mar 21, 2023

oli-obk commented Apr 6, 2023

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

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

Conversation

oli-obk commented Feb 8, 2023 • edited Loading

rustbot commented Feb 8, 2023

rustbot commented Feb 8, 2023

jackh726 commented Feb 10, 2023

oli-obk commented Feb 10, 2023

bors commented Feb 10, 2023

pnkfelix commented Feb 14, 2023

oli-obk commented Feb 16, 2023

rustbot commented Mar 7, 2023

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 14, 2023

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Apr 6, 2023

oli-obk commented Feb 8, 2023 •

edited

Loading