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

Modifies how Arg, Arm, Field, FieldPattern and Variant are visited #63854

Merged
merged 1 commit into from
Aug 25, 2019
Merged

Modifies how Arg, Arm, Field, FieldPattern and Variant are visited #63854

merged 1 commit into from
Aug 25, 2019

Conversation

c410-f3r
Copy link
Contributor

Part of the necessary work to accomplish #63468.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Aug 24, 2019
@c410-f3r c410-f3r mentioned this pull request Aug 24, 2019
Merged
@petrochenkov
Copy link
Contributor

r=me after re-grouping the visiting functions a bit (#63854 (comment)).

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2019
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 24, 2019 •
edited
Loading

@rustbot modify labels to +S-waiting-on-review , -S-waiting-on-author

@rustbot
Copy link
Collaborator

rustbot commented Aug 24, 2019

Error: Parsing label command in comment failed: ...review , -|error: empty label at >| S-waiting...

Please let @rust-lang/release know if you're having trouble with this bot.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 24, 2019
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2019

📌 Commit 6a3d517 has been approved by petrochenkov

@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 Aug 24, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 24, 2019
@Centril
Rollup merge of rust-lang#63854 - c410-f3r:attrs-visit, r=petrochenkov
3b31b25 
Modifies how Arg, Arm, Field, FieldPattern and Variant are visited

Part of the necessary work to accomplish rust-lang#63468.
@Centril Centril mentioned this pull request Aug 24, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Aug 24, 2019
@Centril
Rollup merge of rust-lang#63854 - c410-f3r:attrs-visit, r=petrochenkov
81ec558 
Modifies how Arg, Arm, Field, FieldPattern and Variant are visited

Part of the necessary work to accomplish rust-lang#63468.
@Centril Centril mentioned this pull request Aug 24, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Aug 25, 2019
@Centril
Rollup merge of rust-lang#63854 - c410-f3r:attrs-visit, r=petrochenkov
b0d374a 
Modifies how Arg, Arm, Field, FieldPattern and Variant are visited

Part of the necessary work to accomplish rust-lang#63468.
@Centril Centril mentioned this pull request Aug 25, 2019
Merged
bors added a commit that referenced this pull request Aug 25, 2019
@bors
Auto merge of #63873 - Centril:rollup-jgglypd, r=Centril
9267119 
Rollup of 6 pull requests

Successful merges:

 - #62744 (Refactor `TinyList::contains` and `len` to iterate instead of recurse)
 - #63813 (Do not suggest `.try_into()` on `i32::from(x)`)
 - #63833 (Suggest calling closure with resolved return type when appropriate)
 - #63839 (Ensure miri can do bit ops on pointer values)
 - #63854 (Modifies how Arg, Arm, Field, FieldPattern and Variant are visited)
 - #63859 (Don't unwrap the result of `span_to_snippet`)

Failed merges:

r? @ghost
@bors bors merged commit 6a3d517 into rust-lang:master Aug 25, 2019
@bors
Copy link
Contributor

bors commented Aug 25, 2019

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

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 25, 2019
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 25, 2019
@tesuji
Copy link
Contributor

tesuji commented Aug 25, 2019

Did you forget to modify

rust/src/librustc/hir/intravisit.rs

Lines 335 to 337 in 9267119

fn visit_variant(&mut self, v: &'v Variant, g: &'v Generics, item_id: HirId) {
walk_variant(self, v, g, item_id)
}
?

phansch added a commit to phansch/rust-clippy that referenced this pull request Aug 25, 2019
@phansch
Rustup to rust-lang/rust#63854
818b2cc
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 25, 2019
@bors
Auto merge of #4447 - phansch:fix_build, r=matthiaskrgr
05f603e 
Rustup to rust-lang/rust#63854

changelog: none
@c410-f3r
Copy link
Contributor Author

Did you forget to modify

No. The roots of visit_variant and vist_variant_data are more deeper and this PR only changed the necessary bits to make #63468 viable.

A more complete modification can be provided in another PR

@tesuji
Copy link
Contributor

tesuji commented Aug 25, 2019

Thanks. I asked because clippy broken since this PR. It seems like I need to wait for another PR?

@c410-f3r
Copy link
Contributor Author

It seems like I need to wait for another PR?

I can do this in the next few days

@phansch
Copy link
Member

phansch commented Aug 25, 2019

@lzutao Clippy should already be fixed with rust-lang/rust-clippy#4447

@SimonSapin
Copy link
Contributor

With this PR, an implementation of LateLintPass::check_struct_def doesn’t get a HirId parameter anymore. Is there another way to access that data?

@SimonSapin
Copy link
Contributor

Using check_item instead with if let hir::ItemKind::Struct(def, ..) = &item.node {…} seems to work

@petrochenkov
Copy link
Contributor

with if let hir::ItemKind::Struct(def, ..) = &item.node {…}

Do not forget about ItemKind::Enum and ItemKind::Union, which are also visited by visit_variant_data.

walk_struct_def /check_struct_def use a very outdated naming scheme, btw. They should probably be renamed into walk_variant_data/check_variant_data to avoid confusing people.

@SimonSapin
Copy link
Contributor

This lint was originally implementing check_struct_def and check_variant with comments that suggest the intent was to visit structs and enums (or rather enum variants) respectively.

https://github.com/servo/servo/blob/66e5ad0cb8a7674ba0ff0a5a9a77ccc8dc3f2b0f/components/script_plugins/unrooted_must_root.rs#L145-L199

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

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@c410-f3r @rust-highfive @petrochenkov @rustbot @bors @tesuji @phansch @SimonSapin