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

Handle generic bounds in a uniform way in HIR #93803

Merged
merged 7 commits into from
Apr 30, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Feb 9, 2022

Generic bounds in HIR used to be split between bounds in the parameter definition and bounds in a where clause. This PR attempts to store all of those as where predicates.

This effectively desugars

fn foo<T: Default, U>(x: impl Copy) where U: Clone

into

fn foo<T, U, _V>(x: _V) where T: Default, U: Clone, _V: Copy

(where _V is actually hidden and called "impl Copy").

I managed to make compiler warnings more uniform.
About rustdoc: is making this desugaring user-visible acceptable?
About clippy: I don't understand the subtle logic in the needless-lifetimes lint.

r? @estebank

@rust-highfive
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

rustdoc-json-types is a public (although nightly-only) API. Consider changing src/librustdoc/json/conversions.rs instead; otherwise, make sure you update format_version.

cc @CraftSpider,@aDotInTheVoid

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 9, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2022
@petrochenkov
Copy link
Contributor

About rustdoc: is making this desugaring user-visible acceptable?

I'm not sure how important this is, but AFAIR this is the primary reason why it wasn't done before.

@rust-log-analyzer

This comment has been minimized.

@CraftSpider
Copy link
Contributor

I'm not sure I'd mind seeing all bounds as where, but I'd definitely defer to other rustdoc members on that. If this is going to merge though, please ensure you also bump FORMAT_VERSION in the rustdoc-json-types module. It needs to be increased on every breaking change.

@camelid
Copy link
Member

camelid commented Feb 9, 2022

I'm not sure I'd mind seeing all bounds as where, but I'd definitely defer to other rustdoc members on that

I've kind of thought of doing this anyway, so I'm slightly in favor. This would also improve consistency since for some reason, cross-crate re-exports always display their bounds as where clauses IIRC. Maybe because the ty representation already has this desugaring?

@aDotInTheVoid
Copy link
Member

I think its important to show that their is a difference in rustdoc html, as impl Trait cannot be used with the turbofish, although AFAIKT the pr still does that.

The JSON output also has a significant regression in that pub fn by_impl_trait(x: impl Foo) {} is given arguments ["x", {"inner": "impl Foo", "kind": "generic"}], and no mension of the id of Foo anywhere.

Unrelated, but we should probably merge src/etc/check_missing_items.py into jsondocck

@bors

This comment was marked as outdated.

@camelid
Copy link
Member

camelid commented Feb 10, 2022

impl Trait should definitely not be shown as a generic.

@cjgillot
Copy link
Contributor Author

@aDotInTheVoid how do I run the json tests?

@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Member

Urgau commented Feb 10, 2022

@aDotInTheVoid how do I run the json tests?

./x.py test src/test/rustdoc-json

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to go over the code changes, but the output changes look reasonable to me (with some nitpicking for things we could improve) so it's likely that the code changes themselves are too. I'll read the changes now.

@estebank
Copy link
Contributor

I agree with @camelid, if the signature uses impl Trait, then the docs and error messages need to refer to impl Trait and not X: Trait in all cases. The internal representation can change so that they are handled more gracefully though.

@flip1995
Copy link
Member

About clippy: I don't understand the subtle logic in the needless-lifetimes lint.

I don't see any changes to Clippy's tests. The changes to lifetimes.rs seem reasonable AFAICT. Do you get test failures in Clippy?

@cjgillot
Copy link
Contributor Author

I agree with @camelid, if the signature uses impl Trait, then the docs and error messages need to refer to impl Trait and not X: Trait in all cases. The internal representation can change so that they are handled more gracefully though.

Of course, the desugaring will not appear in docs and errors. Only the internal representation is changed by this PR.

@@ -6,12 +6,6 @@ LL | fn unused_lt<'a>(x: u8) {}
|
= note: `-D clippy::extra-unused-lifetimes` implied by `-D warnings`

error: this lifetime isn't used in the function definition
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flip1995 I mean this change, and the one below. I don't understand how clippy use to account for lifetime uses in bounds and not where clauses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really tell what would cause this from just looking at the Clippy changes. I'll have to review and understand the other changes to rustc in this PR. This might take me a few days until I get to that though.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 11, 2022

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

@bors

This comment was marked as outdated.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d201c81): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 11 4 0 0 11
mean2 0.3% 0.3% N/A N/A 0.3%
max 0.5% 0.3% N/A N/A 0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@Mark-Simulacrum
Copy link
Member

@cjgillot It looks like the pre-merge perf justification in #93803 (comment) was that the improvements balanced regressions, but post-merge we are not seeing any improvements. I think the (presumably) improve code quality/maintenance may still justify this change, but it would be good to re-assess from your perspective.

@jackh726
Copy link
Member

jackh726 commented May 6, 2022

I must have missed this or forgotten about it.

This seems counterintuitive to me in the context of the TAIT refactoring. Of course, @oli-obk knows more than me on this, but surprising nonetheless.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
…ession, r=notriddle

Fix jump to def regression

rust-lang#93803 introduced a regression in the "jump to def" feature. This fixes it.

Nice side-effect: it adds a new regression test. :)

I also used this opportunity to add documentation about this unstable feature in the rustdoc book.

cc `@cjgillot`
r? `@notriddle`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 7, 2022
…ession, r=notriddle

Fix jump to def regression

rust-lang#93803 introduced a regression in the "jump to def" feature. This fixes it.

Nice side-effect: it adds a new regression test. :)

I also used this opportunity to add documentation about this unstable feature in the rustdoc book.

cc `@cjgillot`
r? `@notriddle`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 7, 2022
…ession, r=notriddle

Fix jump to def regression

rust-lang#93803 introduced a regression in the "jump to def" feature. This fixes it.

Nice side-effect: it adds a new regression test. :)

I also used this opportunity to add documentation about this unstable feature in the rustdoc book.

cc ``@cjgillot``
r? ``@notriddle``
flip1995 added a commit to flip1995/rust that referenced this pull request May 7, 2022
With rust-lang#93803 `impl Trait` function arguments get desugared to hidden
where bounds. However, Clippy needs to know if a bound was originally a
impl Trait or an actual bound. This adds a field to the
`WhereBoundPredicate` struct to keep track of this information during
HIR lowering.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2022
…cjgillot

Track if a where bound comes from a impl Trait desugar

With rust-lang#93803 `impl Trait` function arguments get desugared to hidden where bounds. However, Clippy needs to know if a bound was originally a `impl Trait` or an actual bound. This adds a field to the `WhereBoundPredicate` struct to keep track of this information during AST->HIR lowering.

r? `@cjgillot`

cc `@estebank` (as the reviewer of rust-lang#93803)
bors added a commit to rust-lang-ci/rust that referenced this pull request May 10, 2022
…-errors

Fortify handing of where bounds on trait & trait alias definitions

Closes rust-lang#96664
Closes rust-lang#96665

Since rust-lang#93803, when listing all bounds and predicates we now need to account for the possible presence of predicates on any of the generic parameters.  Both bugs were hidden by the special handling of bounds at  the generic parameter declaration position.

Trait alias expansion used to confuse predicates on `Self` and where predicates.
Exiting too late when listing all the bounds caused a cycle error.
xFrednet pushed a commit to xFrednet/rust-clippy that referenced this pull request May 15, 2022
Track if a where bound comes from a impl Trait desugar

With rust-lang/rust#93803 `impl Trait` function arguments get desugared to hidden where bounds. However, Clippy needs to know if a bound was originally a `impl Trait` or an actual bound. This adds a field to the `WhereBoundPredicate` struct to keep track of this information during AST->HIR lowering.

r? `@cjgillot`

cc `@estebank` (as the reviewer of #93803)
xFrednet pushed a commit to xFrednet/rust that referenced this pull request May 21, 2022
With rust-lang#93803 `impl Trait` function arguments get desugared to hidden
where bounds. However, Clippy needs to know if a bound was originally a
impl Trait or an actual bound. This adds a field to the
`WhereBoundPredicate` struct to keep track of this information during
HIR lowering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.