Improve lint for type alias bounds #48909

RalfJung · 2018-03-10T12:48:18Z

First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So:

type T1<U: Bound> = U::Assoc; // compiles
type T2<U> = U::Assoc; // fails
type T3<U> = <U as Bound>::Assoc; // "correct" way to write this, maybe?

I am sorry for creating this mess.

This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like T1 above and show a helpful message to the user:

warning: bounds on generic parameters are not enforced in type aliases
  --> $DIR/type-alias-bounds.rs:57:12
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |            ^^^^^
   |
   = help: the bound will not be checked when the type alias is used, and should be removed
help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases
  --> $DIR/type-alias-bounds.rs:57:21
   |
LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases
   |                     ^^^^^^^^

I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait Assoc belongs to above. The lint does not even run if that resolution fails, so I assume that information is available somewhere...

This is a follow-up for (parts of) #48326. Also see #21903.

This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.

…hings Also, tweak the test for ignored type aliases such that replacing the type alias by a newtype struct leads to a well-formed type definition, and errors when used the way the type alias is used.

First of all, the lint is specific for type aliases. Second, it turns out the bounds are not entirely ignored but actually used when accessing associated types. So change the wording of the lint, and adapt its name to reality. The lint has never been on stable or beta, so renaming is safe.

…hout "as", suggest to use the "as" form instead. This is necessary to get rid of the type bound, and hence silence the warning.

rust-highfive · 2018-03-10T12:48:20Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

RalfJung · 2018-03-10T12:52:13Z

I guess there is also the question if we even want to nudge people towards writing type T3 = ::Assoc;. If that is controversial, I suggest I remove the last commit from this MR (so we get the lint wording and name corrected at least).

The alternative to having people write type T3 = ::Assoc; is to detect if a bound is necessary for well-formedness. So, type T3<U: Bound> = U::Assoc; should be accepted but type T3<U: Bound> = Vec; should be linted against. I have no idea how to do that.

petrochenkov · 2018-03-17T16:41:20Z

src/librustc_lint/builtin.rs

+        // Access to associates types should use `<T as Bound>::Assoc`, which does not need a
+        // bound.  Let's see of this type does that.
+
+        // We use an AST visitor to walk the type.


Nit: HIR visitor

petrochenkov · 2018-03-17T16:45:08Z

src/librustc_lint/builtin.rs

+            fn visit_qpath(&mut self, qpath: &'v hir::QPath, id: NodeId, span: Span) {
+                if TypeAliasBounds::is_type_variable_assoc(qpath) {
+                    self.err.span_help(span,
+                        "use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated \


Absolute paths are ::a::b::c.
IIRC, doc and lang teams had a council at one time and discussed how to call "UFCS paths" (<T as Trait>::Assoc) properly, but I don't remember what they decided. "Fully disambiguated paths" maybe.

Also, missing backquotes around <T as Trait>::Assoc.

petrochenkov · 2018-03-17T16:46:30Z

r=me modulo couple of wording nits
@bors delegate+

bors · 2018-03-17T16:46:32Z

✌️ @RalfJung can now approve this pull request

petrochenkov · 2018-03-17T16:47:39Z

+ Travis says some compile-fail tests are failing

[01:04:45] [compile-fail] compile-fail/issue-17994.rs
[01:04:45] [compile-fail] compile-fail/private-in-public-warn.rs

RalfJung · 2018-03-19T17:07:11Z

Travis says some compile-fail tests are failing

It seems those warnings went away when I moved this from an early to a late lint... is it possible that late (HIR) lints don't run in these cases?

RalfJung · 2018-03-19T21:06:01Z

is it possible that late (HIR) lints don't run in these cases?

@rkruppe confirmed on IRC that's a reasonable assumption, and the UI test looking for these warnings still passes so I think all is good.

@bors r=petrochenkov

bors · 2018-03-19T21:06:03Z

📌 Commit c05d234 has been approved by @petrochenkov

bors · 2018-03-19T21:06:20Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: [beta] Update RLS #49156

bors · 2018-03-19T21:06:20Z

📌 Commit c05d234 has been approved by petrochenkov

…chenkov Improve lint for type alias bounds First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So: ```rust type T1<U: Bound> = U::Assoc; // compiles type T2 = U::Assoc; // fails type T3 = ::Assoc; // "correct" way to write this, maybe? ``` I am sorry for creating this mess. This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like `T1` above and show a helpful message to the user: ``` warning: bounds on generic parameters are not enforced in type aliases --> $DIR/type-alias-bounds.rs:57:12 | LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases | ^^^^^ | = help: the bound will not be checked when the type alias is used, and should be removed help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases --> $DIR/type-alias-bounds.rs:57:21 | LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases | ^^^^^^^^ ``` I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait `Assoc` belongs to above. The lint does not even run if that resolution fails, so I assume that information is available *somewhere*... This is a follow-up for (parts of) rust-lang#48326. Also see rust-lang#21903. This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.

Rollup of 15 pull requests - Successful merges: #48265, #48528, #48552, #48624, #48883, #48909, #49028, #49030, #49102, #49160, #49169, #49203, #49262, #49272, #49295 - Failed merges: #48942, #49035

RalfJung added 3 commits March 10, 2018 11:25

Split param-bounds-ignored into two, it was testing two independent t…

81130ed

…hings Also, tweak the test for ignored type aliases such that replacing the type alias by a newtype struct leads to a well-formed type definition, and errors when used the way the type alias is used.

type_alias_bounds lint: If the type alias uses an associated type wit…

0e6d40a

…hout "as", suggest to use the "as" form instead. This is necessary to get rid of the type bound, and hence silence the warning.

rust-highfive assigned petrochenkov Mar 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2018

RalfJung mentioned this pull request Mar 12, 2018

Trait bounds are not yet enforced in type definitions #21903

Open

petrochenkov reviewed Mar 17, 2018

View reviewed changes

petrochenkov 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 Mar 17, 2018

wording nits

37ff473

update compile-fail tests: fewer warnings because this is now a HIR lint

c05d234

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2018

alexcrichton mentioned this pull request Mar 23, 2018

Rollup of 15 pull requests #49308

Merged

alexcrichton merged commit c05d234 into rust-lang:master Mar 23, 2018

RalfJung mentioned this pull request Mar 28, 2018

Reject bounds in type aliases with edition 2024 #49441

Open

RalfJung deleted the type_alias_bounds branch July 10, 2018 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve lint for type alias bounds #48909

Improve lint for type alias bounds #48909

RalfJung commented Mar 10, 2018 •

edited

Loading

rust-highfive commented Mar 10, 2018

RalfJung commented Mar 10, 2018

petrochenkov Mar 17, 2018

petrochenkov Mar 17, 2018

petrochenkov Mar 17, 2018

petrochenkov commented Mar 17, 2018

bors commented Mar 17, 2018

petrochenkov commented Mar 17, 2018

RalfJung commented Mar 19, 2018

RalfJung commented Mar 19, 2018 •

edited

Loading

bors commented Mar 19, 2018

bors commented Mar 19, 2018

bors commented Mar 19, 2018

Improve lint for type alias bounds #48909

Improve lint for type alias bounds #48909

Conversation

RalfJung commented Mar 10, 2018 • edited Loading

rust-highfive commented Mar 10, 2018

RalfJung commented Mar 10, 2018

petrochenkov Mar 17, 2018

Choose a reason for hiding this comment

petrochenkov Mar 17, 2018

Choose a reason for hiding this comment

petrochenkov Mar 17, 2018

Choose a reason for hiding this comment

petrochenkov commented Mar 17, 2018

bors commented Mar 17, 2018

petrochenkov commented Mar 17, 2018

RalfJung commented Mar 19, 2018

RalfJung commented Mar 19, 2018 • edited Loading

bors commented Mar 19, 2018

bors commented Mar 19, 2018

bors commented Mar 19, 2018

RalfJung commented Mar 10, 2018 •

edited

Loading

RalfJung commented Mar 19, 2018 •

edited

Loading