Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint type_alias_bounds fires unnecessarily for trait bounds that define short-hand projections on the RHS #125709

Closed
danielhenrymantilla opened this issue May 29, 2024 · 6 comments · Fixed by #126575
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. L-type_alias_bounds Lint: type_alias_bounds T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented May 29, 2024

Code

trait Trait { type Assoc; }

type AssocOf<T : ?Sized + Trait> = T::Assoc;

Current output

warning: bounds on generic parameters are not enforced in type aliases
 --> src/lib.rs:3:18
  |
3 | type AssocOf<T : ?Sized + Trait> = T::Assoc;
  |                  ^^^^^^   ^^^^^
  |
help: use fully disambiguated paths (i.e., `<T as Trait>::Assoc`) to refer to associated types in type aliases
 --> src/lib.rs:3:36
  |
3 | type AssocOf<T : ?Sized + Trait> = T::Assoc;
  |                                    ^^^^^^^^
  = note: `#[warn(type_alias_bounds)]` on by default
help: the bound will not be checked when the type alias is used, and should be removed
  |
3 - type AssocOf<T : ?Sized + Trait> = T::Assoc;
3 + type AssocOf<T > = T::Assoc;
  |

Desired output

No warning whatsoever

Rationale and extra context

Whilst it is true that left-hand-side bounds on generic parameters are not enforced in type aliases, { performing a Trait associated type "lookup" on / resolving the Trait associated type of } a given type T does effectively constrain T to be : ?Sized + Trait.

This means that for that specific snippet, the pattern is fine, both presently, and in the future, should these bounds end up enforced.

Whilst it may look like an oddly over-specific example, it is actually a common pattern to define a type alias as a shortcut for a trait associated type lookup.

Granted, the bounds could be skipped, like so:

- type AssocOf<T : ?Sized + Trait> = T::Assoc;
+ type AssocOf<T> = <T as Trait>::Assoc;

but this significantly hinders the quality of the generated documentation (now people need to look at the implementation/"value" of the type alias to try and figure out what the bounds on it are).

Other cases

Bonus points if when : Sized is a supertrait of T, the ?Sized + ends up not being required to prevent the lint from firing.

@danielhenrymantilla danielhenrymantilla added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 29, 2024
@jieyouxu jieyouxu added L-type_alias_bounds Lint: type_alias_bounds A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels May 29, 2024
@fmease
Copy link
Member

fmease commented May 29, 2024

NB By the way, lazy_type_alias implements the desired semantics. See its tracking issue: #112792.

CC #21903.

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented May 29, 2024

Good to know there is progress on the "make type aliases work as expected" front 🙂.

But to avoid confusion, I do want to clarify that this is not what this issue is about, this one is a diagnostics issue, w.r.t. how type aliases currently work. That is, when using <T as Trait>::Assoc in the rhs of a type alias, this already, currently, enforces that T : ?Sized + Trait be true (and when Trait : Sized, we end up with T : Trait).
Given that, we should be allowed to add such bounds without having Rust warn us about our bounds being ignored, because given the rhs, they cannot be.

@fmease
Copy link
Member

fmease commented May 29, 2024

Right, that makes sense (I just had to do some advertising :P).

Yeah, the lint isn't super smart atm. This isn't the only case where bounds on type parameters of (eager) type aliases actually have a user-visible effect. Outlives-bounds for example may affect trait object lifetime defaults (we have an A-lint issue for that somewhere).

@fmease fmease changed the title type_alias_bounds fires unnecessarily Lint type_alias_bounds fires unnecessarily when the LHS contains short-hand projections May 29, 2024
@fmease fmease self-assigned this May 29, 2024
@fmease fmease changed the title Lint type_alias_bounds fires unnecessarily when the LHS contains short-hand projections Lint type_alias_bounds fires unnecessarily when the RHS contains short-hand projections May 29, 2024
@fmease fmease changed the title Lint type_alias_bounds fires unnecessarily when the RHS contains short-hand projections Lint type_alias_bounds fires unnecessarily for trait bounds that define short-hand projections on the RHS May 29, 2024
@danielhenrymantilla
Copy link
Contributor Author

Thanks for improving my poor issue title 😄 @fmease 🙏

@bors bors closed this as completed in ceae371 Jul 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 26, 2024
Rollup merge of rust-lang#126575 - fmease:update-lint-type_alias_bounds, r=compiler-errors

Make it crystal clear what lint `type_alias_bounds` actually signifies

This is part of my work on https://github.com/rust-lang/rust/labels/F-lazy_type_alias ([tracking issue](rust-lang#112792)).

---

To recap, the lint `type_alias_bounds` detects bounds on generic parameters and where clauses on (eager) type aliases. These bounds should've never been allowed because they are currently neither enforced[^1] at usage sites of type aliases nor thoroughly checked for correctness at definition sites due to the way type aliases are represented in the compiler. Allowing them was an oversight.

Explicitly label this as a known limitation of the type checker/system and establish the experimental feature `lazy_type_alias` as its eventual proper solution.

Where this becomes a bit tricky (for me as a rustc dev) are the "secondary effects" of these bounds whose existence I sadly can't deny. As a matter of fact, type alias bounds do play some small roles during type checking. However, after a lot of thinking over the last two weeks I've come to the conclusion (not without second-guessing myself though) that these use cases should not trump the fact that these bounds are currently *inherently broken*. Therefore the lint `type_alias_bounds` should and will continue to flag bounds that may have subordinate uses.

The two *known* secondary effects are:

1. They may enable the use of "shorthand" associated type paths `T::Assoc` (as opposed to fully qualified paths `<T as Trait>::Assoc`) where `T` is a type param bounded by some trait `Trait` which defines that assoc ty.
2. They may affect the default lifetime of trait object types passed as a type argument to the type alias. That concept is called (trait) object lifetime default.

The second one is negligible, no question asked. The first one however is actually "kinda nice" (for writability) and comes up in practice from time to time.

So why don't I just special-case trait bounds that "define" shorthand assoc type paths as originally planned in rust-lang#125709?

1. Starting to permit even a tiny subset of bounds would already be enough to send a signal to users that bounds in type aliases have been legitimized and that they can expect to see type alias bounds in the wild from now on (proliferation). This would be actively misleading and dangerous because those bounds don't behave at all like one would expect, they are *not real*[^2]!
   1. Let's take `type A<T: Trait> = T::Proj;` for example. Everywhere else in the language `T: Trait` means `T: Trait + Sized`. For type aliases, that's not the case though: `T: Trait` and `T: Trait + ?Sized` for that matter do neither mean `T: Trait + Sized` nor `T: Trait + ?Sized` (for both!). Instead, whether `T` requires `Sized` or not entirely depends on the definition of `Trait`[^2]. Namely, whether or not it is bounded by `Sized`.
   2. Given `type A<T: Trait<AssocA = ()>> = T::AssocB;`, while `X: Trait` gets checked given `A<X>` (by virtue of projection wfchecking post alias expansion[^2]), the associated type constraint `AssocA = ()` gets dropped entirely! While we could choose to warn on such cases, it would inevitably lead to a huge pile of special cases.
   3. While it's common knowledge that the body / aliased type / RHS of an (eager) type alias does not get checked for well-formedness, I'm not sure if people would realize that that extends to bounds as well. Namely, `type A<T: Trait<[u8]>> = T::Proj;` compiles even if `Trait`'s generic parameter requires `Sized`. Of course, at usage sites `[u8]: Sized` would still end up getting checked[^2], so it's not a huge problem if you have full control over `A`. However, imagine that `A` was actually part of a public API and was never used inside the defining crate (not unreasonable). In such a scenario, downstream users would be presented with an impossible to use type alias! Remember, bounds may grow arbitrarily complex and nuanced in practice.
   4. Even if we allowed trait bounds that "define" shorthand assoc type paths, we would still need to continue to warn in cases where the assoc ty comes from a supertrait despite the fact that the shorthand syntax can be used: `type A<T: Sub> = T::Assoc;` does compile given `trait Sub: Super {}` and `trait Super { type Assoc; }`. However, `A<X>` does not enforce `X: Sub`, only `X: Super`[^2]. All that to say, type alias bounds are simply not real and we shouldn't pretend they are!
   5. Summarizing the points above, we would be legitimizing bounds that are completely broken!
2. It's infeasible to implement: Due to the lack of `TypeckResults` in `ItemCtxt` (and a way to propagate it to other parts of the compiler), the resolution of type-dependent paths in non-`Body` items (most notably type aliases) is not recoverable from the HIR alone which would be necessary because the information of whether an associated type path (projection) is a shorthand is only present pre&in-HIR and doesn't survive HIR ty lowering. Of course, I could rerun parts of HIR ty lowering inside the lint `type_alias_bounds` (namely, `probe_single_ty_param_bound_for_assoc_ty` which would need to be exposed or alternatively a stripped-down version of it). This likely has a performance impact and introduces complexity. In short, the "benefits" are not worth the costs.

---

* 3rd commit: Update a diagnostic to avoid suggesting type alias bounds
* 4th commit: Flag type alias bounds even if the RHS contains inherent associated types.
  * I started to allow them at some point in the past which was not correct (see commit for details)
* 5th commit: Allow type alias bounds if the RHS contains const projections and GCEs are enabled
  * (and add a `FIXME(generic_const_exprs)` to be revisited before (M)GCE's stabilization)
  * As a matter of fact type alias bounds are enforced in this case because the contained AnonConsts do get checked for well-formedness and crucially they inherit the generics and predicates of their parent item (here: the type alias)
* Remaining commits: Improve the lint `type_alias_bounds` itself

---

Fixes rust-lang#125789 (sugg diag fix).
Fixes rust-lang#125709 (wontfix, acknowledgement, sugg diag applic fix).
Fixes rust-lang#104918 (sugg diag applic fix).
Fixes rust-lang#100270 (wontfix, acknowledgement, sugg diag applic fix).
Fixes rust-lang#94398 (true fix).

r? `@compiler-errors` `@oli-obk`

[^1]: From the perspective of the trait solver.
[^2]: Given `type A<T: Trait> = T::Proj;`, the reason why the trait bound "`T: Trait`" gets *seemingly* enforced at usage sites of the type alias `A` is simply because `A<X>` gets expanded to "`<X as Trait>::Proj`" very early on and it's the *expansion* that gets checked for well-formedness, not the type alias reference.
@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Jul 27, 2024

Should this have been "closed as completed"? The issue remains, we have a diagnostic firing unnecessarily:

Before #126575

warning: bounds on generic parameters are not enforced in type aliases
 --> src/lib.rs:3:18
  |
3 | type AssocOf<T : ?Sized + Trait> = T::Assoc;
  |                  ^^^^^^   ^^^^^
  |
help: use fully disambiguated paths (i.e., `<T as Trait>::Assoc`) to refer to associated types in type aliases
 --> src/lib.rs:3:36
  |
3 | type AssocOf<T : ?Sized + Trait> = T::Assoc;
  |                                    ^^^^^^^^
  = note: `#[warn(type_alias_bounds)]` on by default
help: the bound will not be checked when the type alias is used, and should be removed
  |
3 - type AssocOf<T : ?Sized + Trait> = T::Assoc;
3 + type AssocOf<T > = T::Assoc;
  |

After #126575

warning: bounds on generic parameters in type aliases are not enforced
 --> src/lib.rs:3:18
  |
3 | type AssocOf<T : ?Sized + Trait> = T::Assoc;
  |                  ^^^^^^   ^^^^^ will not be checked at usage sites of the type alias
  |
  = note: this is a known limitation of the type checker that may be lifted in a future edition.
          see issue #112792 <https://github.com/rust-lang/rust/issues/112792> for more information
  = help: add `#![feature(lazy_type_alias)]` to the crate attributes to enable the desired semantics
  = note: `#[warn(type_alias_bounds)]` on by default
help: remove this bound
  |
3 - type AssocOf<T : ?Sized + Trait> = T::Assoc;
3 + type AssocOf<T > = T::Assoc;
  |
help: fully qualify this associated type
  |
3 | type AssocOf<T : ?Sized + Trait> = <T as /* Trait */>::Assoc;
  |                                    +  +++++++++++++++

The PR in question mentions a wontfix:

  • if so, the label ought to be added to this issue, which should be "grey-closed" rather than "closed as completed";
  • I can understand it not being worth, currently, to make the lint smarter not to fire in these cases, but should such a situation warrant closing the issue altogether? I feel like by having this one closed, it's only a matter of time until a new similar issue arises 🤷

@fmease
Copy link
Member

fmease commented Jul 27, 2024

WONTFIX as per #126575. While my initial stance was to "true-fix" this issue, I spent two weeks thinking about it and I stand by my new view as laid out in my "manifesto" (#126575's PR description). I sympathize with anyone who might be annoyed by my decision. However, I think it's the best way forward. lazy_type_alias is the proper solution for this problem. CC #112792. I'm the current owner of this initiative and it's in my best interest to make type aliases more ergonomic and correct -- even if that means it takes more time.

@fmease fmease closed this as not planned Won't fix, can't repro, duplicate, stale Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. L-type_alias_bounds Lint: type_alias_bounds T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants