Skip to content

Replace NestedVisitorMap with generic NestedFilter #90986

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 17, 2022
Merged

Replace NestedVisitorMap with generic NestedFilter #90986

merged 3 commits into from
Jan 17, 2022

Conversation

camsteffen
Copy link
Contributor

This is an attempt to make the intravisit::Visitor API simpler and "more const" with regard to nested visiting.

With this change, intravisit::Visitor does not visit nested things by default, unless you specify type NestedFilter = nested_filter::OnlyBodies (or All). nested_visit_map returns Self::Map instead of NestedVisitorMap<Self::Map>. It panics by default (unreachable if type NestedFilter is omitted).

One somewhat trixty thing here is that nested_filter::{OnlyBodies, All} live in rustc_middle so that they may have type Map = map::Map and so that impl Visitors never need to specify type Map - it has a default of Self::NestedFilter::Map.

@rust-highfive
Copy link
Contributor

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Contributor

r? @jackh726

(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 Nov 17, 2021
@camsteffen
Copy link
Contributor Author

Whoops I meant to have 0 clippy changes until this gets a green light. Reverted the clippy change for now.

@rust-log-analyzer

This comment has been minimized.

# to view
@camsteffen camsteffen closed this Nov 17, 2021
@camsteffen camsteffen reopened this Nov 17, 2021
@rust-log-analyzer

This comment has been minimized.

# to view
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2021
@cjgillot
Copy link
Contributor

Seems like a great idea!
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2021
@bors
Copy link
Collaborator

bors commented Nov 19, 2021

⌛ Trying commit d0c899050fb25547aa086cf84cb380f1b60e28c5 with merge 3e581354f769b575052258b34e488fb27f0d619e...

@bors
Copy link
Collaborator

bors commented Nov 19, 2021

💔 Test failed - checks-actions

@bors bors 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 Nov 19, 2021
@rust-log-analyzer

This comment has been minimized.

# to view
@jackh726
Copy link
Member

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 Nov 20, 2021
@camsteffen
Copy link
Contributor Author

We can't do a perf run without fixing Clippy?

@cjgillot
Copy link
Contributor

I don't think so, sadly.

@camsteffen
Copy link
Contributor Author

shucks

@Mark-Simulacrum
Copy link
Member

You can, just needs some manual adjustment to bootstrap - probably enough to comment Clippy out everywhere in src/bootstrap/dist.rs, roughly.

@bors
Copy link
Collaborator

bors commented Nov 22, 2021

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

@cjgillot cjgillot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 4, 2021
@matthiaskrgr
Copy link
Member

We can't do a perf run without fixing Clippy?

Maybe if you add a commit that removes most of clippy (remove the tests, make it an empty fn main() {} project, so that clippy tests are still run but they are all empty?

@camsteffen
Copy link
Contributor Author

@bors r=cjgillot (can I do this?)

@flip1995
Copy link
Member

As a Clippy team member, you should be able to do so. But apparently it didn't work.

@camsteffen
Copy link
Contributor Author

@bors r=cjgillot

@flip1995

This comment has been minimized.

# to view
@camsteffen
Copy link
Contributor Author

lol

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Jan 17, 2022 •
edited
Loading

Something was strange with bors, there were ~400+ PRs not listed in the queue. I have synchronized it, let's see if this helps
@bors r=cjgillot
edit: it's not done syncing yet apparently

@matthiaskrgr
Copy link
Member

@bors r=cjgillot

@bors
Copy link
Collaborator

bors commented Jan 17, 2022

📌 Commit be6d693 has been approved by cjgillot

@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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2022
@bors
Copy link
Collaborator

bors commented Jan 17, 2022

⌛ Testing commit be6d693 with merge ee5d8d3...

@bors
Copy link
Collaborator

bors commented Jan 17, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing ee5d8d3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2022
@bors bors merged commit ee5d8d3 into rust-lang:master Jan 17, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 17, 2022
This was referenced Jan 17, 2022
Merged
Merged
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ee5d8d3): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
@bors
Auto merge of rust-lang#90986 - camsteffen:nested-filter, r=cjgillot
8f48116 
Replace `NestedVisitorMap` with generic `NestedFilter`

This is an attempt to make the `intravisit::Visitor` API simpler and "more const" with regard to nested visiting.

With this change, `intravisit::Visitor` does not visit nested things by default, unless you specify `type NestedFilter = nested_filter::OnlyBodies` (or `All`). `nested_visit_map` returns `Self::Map` instead of `NestedVisitorMap<Self::Map>`. It panics by default (unreachable if `type NestedFilter` is omitted).

One somewhat trixty thing here is that `nested_filter::{OnlyBodies, All}` live in `rustc_middle` so that they may have `type Map = map::Map` and so that `impl Visitor`s never need to specify `type Map` - it has a default of `Self::NestedFilter::Map`.
@steffahn
Copy link
Member

steffahn commented Feb 3, 2022

The documentation (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/intravisit/trait.Visitor.html) is still outdated, talking about things like “see NestedVisitorMap for details” or “override nested_visit_map to return other than None”.

@camsteffen camsteffen mentioned this pull request Feb 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2022
@bors
Auto merge of rust-lang#93815 - camsteffen:intravisit-docs, r=cjgillot
2c858a7 
Update intravisit docs

Follow-up to rust-lang#90986.

r? `@cjgillot`
@camsteffen camsteffen deleted the nested-filter branch November 19, 2022 14:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@cjgillot cjgillot cjgillot approved these changes

Assignees

@cjgillot cjgillot

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.