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

Rollup of 7 pull requests #128222

Merged
merged 28 commits into from
Jul 26, 2024
Merged

Rollup of 7 pull requests #128222

merged 28 commits into from
Jul 26, 2024

Conversation

tgross35
Copy link
Contributor

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

mu001999 and others added 28 commits June 27, 2024 14:11
…ds, 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.
…nkfelix

Extend rules of dead code analysis for impls for adts to impls for types refer to adts

The rules of dead code analysis for impl blocks can be extended to self types which refer to adts.

So that we can lint the following unused struct and trait:
```rust
struct Foo; //~ ERROR struct `Foo` is never constructed

trait Trait { //~ ERROR trait `Trait` is never used
    fn foo(&self) {}
}

impl Trait for &Foo {}
```

r? `@pnkfelix`
Migrate `dump-ice-to-disk` and `panic-abort-eh_frame` `run-make` tests to rmake

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

Please try:

try-job: x86_64-msvc
try-job: i686-mingw
…errors

Add a label to point to the lacking macro name definition

Fixes rust-lang#126694, but adopts the suggestion from rust-lang#118295

```
 --> src/main.rs:1:14
  |
1 | macro_rules! {
  |            ^ put a macro name here
```
Migrate `interdependent-c-libraries`, `compiler-rt-works-on-mingw` and `incr-foreign-head-span` `run-make` tests to rmake

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

try-job: aarch64-apple
try-job: armhf-gnu
try-job: test-various
try-job: x86_64-mingw
try-job: x86_64-msvc
try-job: x86_64-gnu-llvm-18
…ates-rmake, r=Kobzol

migrate tests/run-make/extern-flag-disambiguates to rmake
Make Clone::clone a lang item

I want to absorb all the logic for picking whether an Instance is LocalCopy or GloballyShared into one place. As part of this, I wanted to identify Clone shims inside `cross_crate_inlinable` and found that rather tricky. `@compiler-errors` suggested that I add a lang item for `Clone::clone` because that would produce other cleanups in the compiler.

That sounds good to me, but I have looked and I've only been able to find one.

r? compiler-errors
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc labels Jul 26, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Jul 26, 2024
@tgross35
Copy link
Contributor Author

@bors r+ rollup=never p=5

Seems like there is a medium chance that the ICE dump test fails, so iffy rollup. But it was next in the queue anyway so we're no worse off if it does.

@bors
Copy link
Contributor

bors commented Jul 26, 2024

📌 Commit 97eade4 has been approved by tgross35

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2024
@bors
Copy link
Contributor

bors commented Jul 26, 2024

⌛ Testing commit 97eade4 with merge 83d6768...

@bors
Copy link
Contributor

bors commented Jul 26, 2024

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 83d6768 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 26, 2024
@bors bors merged commit 83d6768 into rust-lang:master Jul 26, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 26, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#126575 Make it crystal clear what lint type_alias_bounds actuall… eac4883667b01f6cb424f0fa1df222acb72d7796 (link)
#127017 Extend rules of dead code analysis for impls for adts to im… e7d2bda1e5d072a5f92e1e4c6d25ff11b4299abd (link)
#127523 Migrate dump-ice-to-disk and panic-abort-eh_frame `run-… 76a98e2f3386151f041b7428d38bafc4604576a3 (link)
#127557 Add a label to point to the lacking macro name definition aeb8cdf7b2d7aa5a1ad443b263bc2320d8a6d7d5 (link)
#127989 Migrate interdependent-c-libraries, `compiler-rt-works-on… a0b15fafd13bf6c3cc50a30cefb6c9fe06e56057 (link)
#128099 migrate tests/run-make/extern-flag-disambiguates to rmake 0cc163fc59975831c6da727c3e097cc5a672b414 (link)
#128170 Make Clone::clone a lang item 3aebfb8e0c37cedf97abf7a369c7830f7ffe35b3 (link)

previous master: 6ef11b81c2

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (83d6768): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 5.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
5.3% [5.3%, 5.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 5.3% [5.3%, 5.3%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.248s -> 771.116s (0.11%)
Artifact size: 329.00 MiB -> 328.99 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants