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 local_def_id and as_local_hir_id #71215

Merged

Conversation

marmeladema
Copy link
Contributor

@marmeladema marmeladema commented Apr 16, 2020

See #70853

@marmeladema marmeladema changed the title Issue70853/librustc middle local def id 2 Simplify local_def_id and as_local_hir_id Apr 16, 2020
@marmeladema
Copy link
Contributor Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned varkor Apr 16, 2020
@marmeladema marmeladema marked this pull request as ready for review April 16, 2020 20:23
Comment on lines +345 to +346
pub fn as_local_hir_id(&self, def_id: LocalDefId) -> hir::HirId {
self.local_def_id_to_hir_id(def_id)
Copy link
Member

Choose a reason for hiding this comment

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

This is hilarious now. Not sure we should in this PR but at some point maybe we can change everything to def_id_to_hir_id? def_id_to is just 1 longer than as_local but clearer IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Would local_def_id_to_hir_id be even clearer?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure tbh. It's a bit long but it might be worth it.

src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
@JohnTitor JohnTitor added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 16, 2020
@marmeladema marmeladema force-pushed the issue70853/librustc_middle-local-def-id-2 branch from 77734a9 to 2988d3d Compare April 16, 2020 22:45
@marmeladema marmeladema force-pushed the issue70853/librustc_middle-local-def-id-2 branch from 2988d3d to da9c867 Compare April 16, 2020 22:59
@eddyb
Copy link
Member

eddyb commented Apr 16, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 16, 2020

⌛ Trying commit da9c867 with merge 4b8b1c55d91c00df6ad5b5b174c4dbfae7bab373...

@bors
Copy link
Contributor

bors commented Apr 17, 2020

☀️ Try build successful - checks-azure
Build commit: 4b8b1c55d91c00df6ad5b5b174c4dbfae7bab373 (4b8b1c55d91c00df6ad5b5b174c4dbfae7bab373)

@rust-timer
Copy link
Collaborator

Queued 4b8b1c55d91c00df6ad5b5b174c4dbfae7bab373 with parent 7f3df57, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 4b8b1c55d91c00df6ad5b5b174c4dbfae7bab373, comparison URL.

@eddyb
Copy link
Member

eddyb commented Apr 17, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Apr 17, 2020

📌 Commit da9c867 has been approved by eddyb

@marmeladema marmeladema force-pushed the issue70853/librustc_middle-local-def-id-2 branch from 4bc4665 to b9ba521 Compare April 23, 2020 22:42
@marmeladema
Copy link
Contributor Author

@ecstatic-morse @Dylan-DPC @eddyb rebased over master and fix the new librustdoc error

@Dylan-DPC-zz
Copy link

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit b9ba521 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 Apr 23, 2020
@bors
Copy link
Contributor

bors commented Apr 24, 2020

⌛ Testing commit b9ba521 with merge 94317f50b93fe0774aa16e4f696ee62b7202f4bb...

@Dylan-DPC-zz
Copy link

@bors retry (Yielding to rollup)

@bors
Copy link
Contributor

bors commented Apr 24, 2020

⌛ Testing commit b9ba521 with merge 5a59527...

@bors
Copy link
Contributor

bors commented Apr 24, 2020

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 5a59527 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 24, 2020
@bors bors merged commit 5a59527 into rust-lang:master Apr 24, 2020
@marmeladema
Copy link
Contributor Author

Aaaaaaah finally! Thank you so much @ecstatic-morse @eddyb and @Dylan-DPC !

This change might break some tools because hir method as_local_def_id has changed signature. It does not return an Option anymore. Recommended practice is to call as_local() before on the DefId to get a LocalDefId is possible.
cc @RalfJung @matthiaskrgr

@marmeladema marmeladema deleted the issue70853/librustc_middle-local-def-id-2 branch April 24, 2020 07:28
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 24, 2020
flip1995 pushed a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 24, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 24, 2020
rustup rust-lang/rust#71215

There's currently an crash in `ui/new_without_default.rs` that I need to figure out how to avoid.

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 25, 2020
rustup rust-lang/rust#71215

There's currently an crash in `ui/new_without_default.rs` that I need to figure out how to avoid.

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 25, 2020
rustup rust-lang/rust#71215

There's currently an crash in `ui/new_without_default.rs` that I need to figure out how to avoid.

changelog: none
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 26, 2020
Changes:
````
rustup to rust-lang#70043
map_clone: avoid suggesting `copied()` for &mut
fix redundant_pattern_matching lint
Add tests for rust-lang#1654
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
Update issue_2356.stderr reference file
Update while_let_on_iterator tests
Fix while_let_on_iterator suggestion and make it MachineApplicable
Add lifetime test case for `new_ret_no_self`
rustup rust-lang#71215
Downgrade match_bool to pedantic
Run fetch before testing if master contains beta
The beta branch update should not require a force push
Add a note to the beta sections of release.md
Remove apt-get upgrade again
Always use the deploy script and templates of the master branch
README: fix lit count line
clippy_dev: make it fatal when the regex for updating lint count does not match
`predecessors_for` will be removed soon
Rustup "Remove `BodyAndCache`"
Only run (late) internal lints, when they are warn/deny/forbid
Only run cargo lints, when they are warn/deny/forbid
span_lint_and_note now takes an Option<Span> for the note_span instead of just a span
Make lint also capture blocks and closures, adjust language to mention other mutex types
don't test the code in the lint docs
Switch to matching against full paths instead of just the last element of the path
Lint for holding locks across await points
Also mention `--fix` for nightly users
fix crash on issue-69020-assoc-const-arith-overflow.rs
Address review comments
remark fixes
Update CHANGELOG.md for Rust 1.43 and 1.44
update stderr file
util/fetch_prs_between.sh: Add Markdown formatted Link
factor ifs into function, add differing mutex test
Update the changelog update documentation
Apply suggestions from PR review
update span_lint_and_help call to six args
test for mutex eq, add another test case
use if chain
cargo dev fmt
fix map import to rustc_middle
dev update_lints
fix internal clippy warnings
change visitor name to OppVisitor
use Visitor api to find Mutex::lock calls
add note about update-all-refs script, revert redundant pat to master
move closures to seperate fns, remove known problems
use span_lint_and_help, cargo dev fmt
creating suggestion
progress work on suggestion for auto fix
Implement unsafe_derive_deserialize lint
Update empty_enum.stderr
Formatting and naming
Formatting and naming
Cleanup: `node_id` -> `hir_id`
Fix issue rust-lang#2907.
Don't trigger toplevel_ref_arg for `for` loops
Cleanup: future_not_send: use `return_ty` method
Remove badge FIXME from Cargo.toml
Change note_span argument for span_lint_and_note.
Add an Option<Span> argument to span_lint_and_help.
Fixes internal lint warning in code base.
Implement collapsible_span_lint_calls lint.
````

Fixes rust-lang#71453
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 27, 2020
…=eddyb

Convert more queries to use `LocalDefId`

This PR is based on commits in rust-lang#71215 and should partially solve rust-lang#70853
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2020
…ddyb

Convert more queries to use `LocalDefId`

This PR is based on commits in rust-lang#71215 and should partially solve rust-lang#70853
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
rustup to rust-lang/rust#70043
map_clone: avoid suggesting `copied()` for &mut
fix redundant_pattern_matching lint
Add tests for rust-lang#1654
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
Update issue_2356.stderr reference file
Update while_let_on_iterator tests
Fix while_let_on_iterator suggestion and make it MachineApplicable
Add lifetime test case for `new_ret_no_self`
rustup rust-lang/rust#71215
Downgrade match_bool to pedantic
Run fetch before testing if master contains beta
The beta branch update should not require a force push
Add a note to the beta sections of release.md
Remove apt-get upgrade again
Always use the deploy script and templates of the master branch
README: fix lit count line
clippy_dev: make it fatal when the regex for updating lint count does not match
`predecessors_for` will be removed soon
Rustup "Remove `BodyAndCache`"
Only run (late) internal lints, when they are warn/deny/forbid
Only run cargo lints, when they are warn/deny/forbid
span_lint_and_note now takes an Option<Span> for the note_span instead of just a span
Make lint also capture blocks and closures, adjust language to mention other mutex types
don't test the code in the lint docs
Switch to matching against full paths instead of just the last element of the path
Lint for holding locks across await points
Also mention `--fix` for nightly users
fix crash on issue-69020-assoc-const-arith-overflow.rs
Address review comments
remark fixes
Update CHANGELOG.md for Rust 1.43 and 1.44
update stderr file
util/fetch_prs_between.sh: Add Markdown formatted Link
factor ifs into function, add differing mutex test
Update the changelog update documentation
Apply suggestions from PR review
update span_lint_and_help call to six args
test for mutex eq, add another test case
use if chain
cargo dev fmt
fix map import to rustc_middle
dev update_lints
fix internal clippy warnings
change visitor name to OppVisitor
use Visitor api to find Mutex::lock calls
add note about update-all-refs script, revert redundant pat to master
move closures to seperate fns, remove known problems
use span_lint_and_help, cargo dev fmt
creating suggestion
progress work on suggestion for auto fix
Implement unsafe_derive_deserialize lint
Update empty_enum.stderr
Formatting and naming
Formatting and naming
Cleanup: `node_id` -> `hir_id`
Fix issue rust-lang#2907.
Don't trigger toplevel_ref_arg for `for` loops
Cleanup: future_not_send: use `return_ty` method
Remove badge FIXME from Cargo.toml
Change note_span argument for span_lint_and_note.
Add an Option<Span> argument to span_lint_and_help.
Fixes internal lint warning in code base.
Implement collapsible_span_lint_calls lint.
````

Fixes #71453
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants