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

Compute is_late_bound_map query separately from lifetime resolution #97415

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

cjgillot
Copy link
Contributor

This query is actually very simple, and is only useful for functions and method. It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on #96296

@rust-highfive
Copy link
Collaborator

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 May 26, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2022
@jackh726
Copy link
Member

This is a good candidate to split out independently, if you can. I'd r+ quickly.

@cjgillot
Copy link
Contributor Author

Splitting this means reimplementing ae05823. I'd like to avoid it, as #96296 FCP ends soon. I can remove d738f62 though.

Comment on lines 13 to 17
'x: loop { $e }
//~^ WARNING shadows a label name that is already in scope
//~| WARNING shadows a label name that is already in scope
//~| WARNING shadows a label name that is already in scope
//~| WARNING shadows a label name that is already in scope
Copy link
Contributor

@estebank estebank May 26, 2022

Choose a reason for hiding this comment

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

Wait, this isn't a regression? The new behavior is what this should have always been, right?

If so, let's change these files to be // check-build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explicitly proposed to lang team in #96296.

Comment on lines 30 to 48
warning: label name `'a` shadows a label name that is already in scope
--> $DIR/label_break_value_invalid.rs:22:13
|
LL | let x: u8 = 'a: {
| -- first declared here
...
LL | 'a: {
| ^^ label `'a` already in scope
...
LL | let x: u8 = mac3!('b: {
| _________________-
LL | | if true {
LL | | break 'a 3;
LL | | }
LL | | 0
LL | | });
| |______- in this macro invocation
|
= note: this warning originates in the macro `mac3` (in Nightly builds, run with -Z macro-backtrace for more info)
Copy link
Contributor

Choose a reason for hiding this comment

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

These on the other hand, are a regression, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise.

Comment on lines 9 to 15
error[E0392]: parameter `'a` is never used
--> $DIR/regions-name-duplicated.rs:1:12
|
LL | struct Foo<'a, 'a> {
| ^^ unused parameter
|
= help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd take this to be a regression... Could we expand E0392 to check for the duplicated case so that we don't emit this redundant error?

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 change needs to be in a totally different place. I'll do that in a follow-up PR.

Comment on lines +367 to +370
OwnerNode::ForeignItem(ForeignItem {
kind: ForeignItemKind::Fn(_, _, generics),
..
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially break existing (invalid) code? Likely worth it to land anyways, but might need a crater run to gauge the fallout.

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 an inconsistency here between hir().get_generics and the custom implementation in generics_of. I am tempted to correct it by adding more cases to get_generics and use it in generics_of. I don't see how it could break code, but I'll need to review the calls before giving an definite answer.

@@ -128,8 +128,8 @@ fn label_break_macro() {
0
};
assert_eq!(x, 0);
let x: u8 = 'a: { //~ WARNING `'a` shadows a label
'b: { //~ WARNING `'b` shadows a label
Copy link
Contributor

Choose a reason for hiding this comment

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

Marc file as check-pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already run-pass because it checks that the correct labels are selected in case of hygienic shadowing.

@@ -20,14 +20,11 @@ fn lbv_macro_test_hygiene_respected() {
macro_rules! mac3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

mark file as check-pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file still errs on undeclared labels at lines 7 and 29.

Comment on lines +2208 to +2198
let name = self.tcx.item_name(def_id.to_def_id());
let span = self.tcx.def_ident_span(def_id.to_def_id())?;
Some((name, span))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be the same as getting the ident and passing that through?

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 point of this commit was to stop storing the ident of parameters in the ribs. As we only need them in the case of diagnostics, we fetch them at the last moment.
Anyway, this code will go away with #97313.

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.

r=me with confirmation that all the tests that should be check-pass, are

@cjgillot cjgillot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2022
@estebank
Copy link
Contributor

estebank commented Jun 1, 2022

r=me after making CI happy

@bors
Copy link
Contributor

bors commented Jun 2, 2022

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

The computation is actually much simpler, and can be done by directly
fetching the HIR for the `FnDecl` and its generics.
@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 3, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Jun 3, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jun 3, 2022

📌 Commit ba40fe9 has been approved by estebank

@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 Jun 3, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 3, 2022
…bank

Compute `is_late_bound_map` query separately from lifetime resolution

This query is actually very simple, and is only useful for functions and method.  It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on rust-lang#96296
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2022
Rollup of 3 pull requests

Successful merges:

 - rust-lang#97415 (Compute `is_late_bound_map` query separately from lifetime resolution)
 - rust-lang#97471 (Provide more context when denying invalid type params )
 - rust-lang#97681 (Add more eslint checks)

Failed merges:

 - rust-lang#97446 (Make hir().get_generics and generics_of consistent.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 53ab3b2 into rust-lang:master Jun 3, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 3, 2022
@cjgillot cjgillot deleted the is-late-bound-solo branch June 3, 2022 20:48
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
…bank

Compute `is_late_bound_map` query separately from lifetime resolution

This query is actually very simple, and is only useful for functions and method.  It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on rust-lang#96296
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 5, 2022
Fail gracefully when encountering an HRTB in APIT.

Fixes rust-lang#96954

~The first commit will be merged as part of rust-lang#97415
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 6, 2022
Fail gracefully when encountering an HRTB in APIT.

Fixes rust-lang#96954

~The first commit will be merged as part of rust-lang#97415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

6 participants