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

Simplify ExprUseVisitor #64874

Merged
merged 2 commits into from
Oct 4, 2019
Merged

Simplify ExprUseVisitor #64874

merged 2 commits into from
Oct 4, 2019

Conversation

matthewjasper
Copy link
Contributor

  • Remove HIR const qualification
  • Remove parts of ExprUseVisitor that aren't being used

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2019
@eddyb
Copy link
Member

eddyb commented Sep 28, 2019

I just noticed the old regionck is still around - do we have a timeline for removing it?

@@ -2,25 +2,20 @@
//! normal visitor, which just walks the entire body in one shot, the
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check whether clippy still works after your PR? We use this visitor a lot.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... anything using EUV should've transitioned a long time ago to being based on MIR, IMO. I wish I brought this up to clippy people earlier :/

@@ -19,12 +19,10 @@ use rustc::ty::query::Providers;
pub mod error_codes;

pub mod ast_validation;
pub mod rvalue_promotion;
pub mod hir_stats;
pub mod layout_test;
pub mod loops;
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this crate is becoming quite pointless.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could split them out to a crate for each pass. :)

Copy link
Member

Choose a reason for hiding this comment

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

They are tiny though... a bunch of them belong in rustc::hir which could become a crate (with only the data types remaining in rustc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever we do, let's please not inflate the size of the rustc crate. :)

@@ -582,48 +582,13 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
}

impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
Copy link
Member

@eddyb eddyb Sep 28, 2019

Choose a reason for hiding this comment

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

Is this the only user left? Can its reliance on MC be simplified too?

@matthewjasper
Copy link
Contributor Author

The old regionck is still used for items. A lot of it can be removed now, but that's another PR.

Notes for anyone trying to update Clippy after this lands:

  • utils/usage.rs - doesn't use anything removed here
  • loops.rs - needs to change from the span passed in to the span of the cmt.
  • escape.rs - consume and consume_pat will need to be merged
  • needless_pass_by_value - should probably use borrow instead of matched_pat

@bors
Copy link
Contributor

bors commented Sep 30, 2019

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

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM, r=me if you don't want to change more due to #64874 (comment)

@matthewjasper
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Oct 3, 2019

📌 Commit b4ad612 has been approved by eddyb

@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 Oct 3, 2019
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Nice cleanup, @matthewjasper!

Centril added a commit to Centril/rust that referenced this pull request Oct 4, 2019
Simplify ExprUseVisitor

* Remove HIR const qualification
* Remove parts of ExprUseVisitor that aren't being used

r? @eddyb
bors added a commit that referenced this pull request Oct 4, 2019
Rollup of 5 pull requests

Successful merges:

 - #64749 (Fix most remaining Polonius test differences)
 - #64817 (Replace ClosureSubsts with SubstsRef)
 - #64874 (Simplify ExprUseVisitor)
 - #65026 (metadata: Some crate loading cleanup)
 - #65073 (Remove `borrowck_graphviz_postflow` from test)

Failed merges:

r? @ghost
@bors bors merged commit b4ad612 into rust-lang:master Oct 4, 2019
@matthewjasper matthewjasper deleted the simplify-euv branch October 4, 2019 12:03
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Oct 4, 2019
Episode 1 - The simple cases
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Oct 4, 2019
Episode 1 - The simple cases
bors added a commit to rust-lang/rust-clippy that referenced this pull request Oct 8, 2019
Rustup to rust-lang/rust#64874

TODO:
- [x] replace `rvalue_promotable_map` in [1]
- [ ] ~~fix [2] according to this comment rust-lang/rust#64874 (comment) this should be merged with `consume`, but I didn't figure out how to merge them, yet.~~
- [ ] ~~fix [3]; What to do with `LoanCause`?~~

[2]+[3] probably have to be resolved by a rewrite of the lint. #4628 (comment)

[1]
https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/methods/mod.rs#L1292-L1299

[2]
https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/escape.rs#L126

[3]
https://github.com/rust-lang/rust-clippy/blob/54bf4ffd626970e831bb80c037f804a3b3450835/clippy_lints/src/escape.rs#L166-L176

I could need some help with [1]. The purpose of this is to "don't lint for constant values". cc @matthewjasper

For now I see what I can do with [2].

changelog: Temporary break `boxed_local` lint.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants