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.

Sign up for GitHub

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

Jump to bottom

Emit a note that static bounds from HRTBs are a bug #101433

Merged
merged 4 commits into from Sep 14, 2022
Merged

Emit a note that static bounds from HRTBs are a bug #101433

merged 4 commits into from Sep 14, 2022

Conversation

jackh726
Copy link
Contributor

@jackh726 jackh726 commented Sep 5, 2022

This note isn't perfect, but opening this to either 1) land as is or 2) get some feedback on how to improve it

Let r? @compiler-errors and cc. @nikomatsakis

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 5, 2022

rustc_error_messages was changed

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2022
jackh726
jackh726 commented Sep 5, 2022
View reviewed changes
src/test/ui/generic-associated-types/bugs/hrtb-implied-1.stderr
note: due to current limitations in the borrow checker, this implies a `'static` lifetime
--> $DIR/hrtb-implied-1.rs:28:26
|
LL | for<'a> I::Item<'a>: Debug,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estebank maybe you have some thoughts on how to make this point to the whole clause?

Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit about this since you had mentioned it earlier -- it's probably not generalizable to point to the whole clause because we can have things like where Ty: Trait + OtherTrait -- the choice for the spans in predicates_of to point to just the trait part was probably made for that reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it's not simply enough to say "point to the entire clause. Instead, I'm imagining something like:

for<'a> T: Trait<'a>
^^^^^^^^^^^^^^^^^^^^

for<'a> T: Send + Trait<'a>
^^^^^^^^^^        ^^^^^^^^^

for<'a> T: Trait<'a> + Send
^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we currently have no way (that I know of) to even get that span info from an InstantiatedPredicate. We lose that when generating the GenericPredicates, and there's no mapping back, AFAICT. I was looking a bit at instead storing like the owner and which predicate it is, but that also doesn't quite work, since lowering is done in a bit more complicated fashion.

Copy link
Contributor

@estebank estebank Sep 12, 2022 •
edited

Choose a reason for hiding this comment

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

Yeah, we do lose too much metadata when dealing with predicates :-/

Ideally the spans should be something along the lines of

for<'a> T: Send + Trait<'a>
^^^^^^^           ^^^^^^^^^

but it's perfectly fine to land as is.

@jackh726 jackh726 removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 5, 2022
@bors
Copy link
Contributor

bors commented Sep 8, 2022

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

compiler-errors
compiler-errors approved these changes Sep 11, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after rebased past conflict

compiler/rustc_middle/src/ty/structural_impls.rs
@@ -645,6 +645,20 @@ impl<'a, 'tcx> Lift<'tcx> for ty::InstanceDef<'a> {
}
}

impl<'tcx> Lift<'tcx> for Field {
Copy link
Member

Choose a reason for hiding this comment

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

nit: there's some trivial lift macro i think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not (that I could find)

Copy link
Member

Choose a reason for hiding this comment

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

TrivialTypeTraversalAndLiftImpls is what it's called, which itself uses CloneLiftImpls if you don't want to derive the traversal/folder traits. But up to you to keep this manual impl.

@jackh726 jackh726 force-pushed the better-static-placeholder-error branch from 96e6985 to 6fd6283 Compare September 12, 2022 01:46
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 12, 2022
@jackh726 jackh726 force-pushed the better-static-placeholder-error branch from 6fd6283 to bdd64c6 Compare September 12, 2022 01:47
@jackh726 jackh726 removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 12, 2022
@jackh726
Copy link
Contributor Author

I'm going to wait for the stabilization PR to merge, since they're going to conflict and this will be easier to rebase.

@jackh726 jackh726 force-pushed the better-static-placeholder-error branch from bdd64c6 to cdd18af Compare September 14, 2022 00:02
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 14, 2022
@jackh726 jackh726 removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 14, 2022
@jackh726 jackh726 force-pushed the better-static-placeholder-error branch from cdd18af to aae37f8 Compare September 14, 2022 00:20
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 14, 2022
@jackh726
Copy link
Contributor Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Sep 14, 2022

📌 Commit aae37f8 has been approved by compiler-errors

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 Sep 14, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Sep 14, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2022
@bors
Auto merge of rust-lang#101805 - Dylan-DPC:rollup-mpdlbin, r=Dylan-DPC
a926696 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#101433 (Emit a note that static bounds from HRTBs are a bug)
 - rust-lang#101684 (smol grammar changes to README.md)
 - rust-lang#101769 (rustdoc: remove redundant CSS `.out-of-band > span.since { position }`)
 - rust-lang#101772 (Also replace the placeholder for the stable_features lint)
 - rust-lang#101773 (rustdoc: remove outdated CSS `.content table` etc)
 - rust-lang#101779 (Update test output for drop tracking)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4301231 into rust-lang:master Sep 14, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 14, 2022
@jackh726 jackh726 deleted the better-static-placeholder-error branch September 14, 2022 18:41
@lqd lqd mentioned this pull request Sep 15, 2022
Closed
@lcnr lcnr mentioned this pull request Sep 16, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2022
@bors
Auto merge of rust-lang#101902 - jackh726:revert-static-hrtb-error, r…
4d4e51e 
…=nikomatsakis

Partially revert rust-lang#101433

reverts rust-lang#101433 to fix rust-lang#101844

We should get this into the beta cut, since the ICE is getting hit quite a bit.
@Mark-Simulacrum Mark-Simulacrum added the perf-regression Performance regression. label Sep 20, 2022
@Mark-Simulacrum
Copy link
Member

Marking as a perf regression per results from this PR's merge in the rollup (#101805 (comment)). (For clarity: the results are from just this PR in that comment, and match the effects of the rollup as a whole).

It looks like we partially reverted in #101902, but that didn't fully recover the performance here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jackh726 @rustbot @bors @Mark-Simulacrum @estebank @compiler-errors @rust-highfive