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

rustc_middle: return LocalDefId where possible in hir::map module #70986

Merged

Conversation

marmeladema
Copy link
Contributor

This changes the return type of the following functions to return a LocalDefId instead of a DefId:

  • opt_local_def_id_from_node_id
  • opt_local_def_id
  • body_owner_def_id
  • local_def_id_from_node_id
  • get_parent_id

This is another step in the right direction for #70853

This pull request will be followed by another (substantial one) which changes the return type of local_def_id function but this change being more invasive, we might want to wait for #70956 or #70961 (or some other form it) to land first.

@marmeladema
Copy link
Contributor Author

cc @ecstatic-morse @eddyb

@@ -62,7 +62,7 @@ fn mir_keys(tcx: TyCtxt<'_>, krate: CrateNum) -> &DefIdSet {
let mut set = DefIdSet::default();

// All body-owners have MIR associated with them.
set.extend(tcx.body_owners());
set.extend(tcx.body_owners().map(LocalDefId::to_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 query should return a &'tcx FxHashSet<LocalDefId> instead of &'tcx DefIdSet (which AFAIK is just FxHashSet<DefId> - IMO we should stop using the aliases, they only exist because NodeMap was the first iteration of FxHashMap, ages ago, and we never moved on from that).

Copy link
Member

Choose a reason for hiding this comment

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

(this doesn't have to be done in this PR, especially if you plan to tweak queries in a different PR)

@eddyb
Copy link
Member

eddyb commented Apr 10, 2020

@bors r+ rollup=never (for perf)

@bors
Copy link
Contributor

bors commented Apr 10, 2020

📌 Commit b6b0057 has been approved by eddyb

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 10, 2020
@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

Let's check in advance if it has any perf impact; @bors try @rust-timer queue

@bors
Copy link
Contributor

bors commented Apr 10, 2020

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@eddyb
Copy link
Member

eddyb commented Apr 10, 2020

@Centril I don't see why it could cause a regression, but it may result in improvements, maybe I should be clearer on that.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

@bors p=1

@bors
Copy link
Contributor

bors commented Apr 10, 2020

⌛ Testing commit b6b0057 with merge f363450...

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 10, 2020
…dle-local-def-id, r=eddyb

rustc_middle: return `LocalDefId` where possible in hir::map module

This changes the return type of the following functions to return a `LocalDefId` instead of a `DefId`:
* opt_local_def_id_from_node_id
* opt_local_def_id
* body_owner_def_id
* local_def_id_from_node_id
* get_parent_id

This is another step in the right direction for rust-lang#70853

This pull request will be followed by another (substantial one) which changes the return type of `local_def_id` function but this change being more invasive, we might want to wait for rust-lang#70956 or rust-lang#70961 (or some other form it) to land first.
@bors
Copy link
Contributor

bors commented Apr 11, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2020
@bors bors merged commit f363450 into rust-lang:master Apr 11, 2020
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 11, 2020
@eddyb
Copy link
Member

eddyb commented Apr 12, 2020

perf results look pretty neutral, within noise margins.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 12, 2020
Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang#70986
rustup rust-lang#69745
Rustup to rust-lang#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes rust-lang#70993
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2020
submodules: update clippy from d342cee to af5940b

Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang#70986
rustup rust-lang#69745
Rustup to rust-lang#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes rust-lang#70993

r? @Dylan-DPC
@marmeladema marmeladema deleted the issue70853/librustc_middle-local-def-id branch April 18, 2020 12:42
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
Allow UUID style formatting for `inconsistent_digit_grouping` lint
rustup rust-lang/rust#70986
rustup rust-lang/rust#69745
Rustup to rust-lang/rust#70913
compare with the second largest instead of the smallest variant
Revert "Downgrade new_ret_no_self to pedantic"
Check for clone-on-copy in argument positions
Check fn header along with decl when suggesting to implement trait
Downgrade implicit_hasher to pedantic
Move cognitive_complexity to nursery
Run fmt and update test
Use int assoc consts in MANUAL_SATURATING_ARITHMETIC
Use int assoc consts in checked_conversions lint
Use primitive type assoc consts in more tests
Use integer assoc consts in more lint example code
Don't import primitive type modules
Use assoc const NAN for zero_div_zero lint
Fix float cmp to use assoc fxx::EPSILON
Fix NAN comparison lint to use assoc NAN
Refine lint message.
Lint on opt.as_ref().map(|x| &**x).
Include OpAssign in suspicious_op_assign_impl
result_map_or_into_option: fix syntax error in example
result_map_or_into: fix dogfood_clippy error => {h,l}int
CONTRIBUTING.md: fix broken triage link
result_map_or_into_option: fix `cargo dev fmt --check` errors
result_map_or_into_option: move arg checks into tuple assignment
result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test
result_map_or_into_option: destructure lint tuple or return early
result_map_or_into_option: add good and bad examples
result_map_or_into_option: explicitly note absence of known problems
Downgrade new_ret_no_self to pedantic
Downgrade unreadable_literal to pedantic
Update CONTRIBUTING.md
Rename rustc -> rustc_middle in doc links
result_map_or_into_option: add lint to catch manually adpating Result -> Option
Move matches test in matches module
Run update_lints
Make lint modules private
Don't filter lints in code generation functions
Build lint lists once and the reuse them to update files
Get rid of Lint::is_internal method
Clean up update_lints
Downgrade inefficient_to_string to pedantic
Downgrade trivially_copy_pass_by_ref to pedantic
Downgrade let_unit_value to pedantic
````
Fixes #70993
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.

6 participants