librustc_middle: change query functions to accept impl Into<$K> type #70956

marmeladema · 2020-04-09T14:34:06Z

This should help to increase the usage of LocalDefId instead of DefId.

Some now useless calls to `.to_def_id()` have been removed as example of usefulness of the change.

rust-highfive · 2020-04-09T14:34:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

mark-i-m · 2020-04-09T14:39:27Z

cc @eddyb @ecstatic-morse

eddyb · 2020-04-09T14:40:12Z

r? @Zoxc cc @rust-lang/compiler

Not sure Into is the right trait (we might want to use our own trait to control it better), but I've wanted this before. We should also check if this affects building the compiler (time/size).

eddyb · 2020-04-09T14:41:26Z

src/librustc_span/def_id.rs

+impl From<LocalDefId> for DefId {
+    fn from(local_def_id: LocalDefId) -> DefId {
+        local_def_id.to_def_id()
+    }
+}


Another reason to not use From/Into is that we kind of want people to write .to_def_id() instead of .into()... or maybe we don't.

I am new to all of this and my opinion probably does not matter much but using Into is useful because we don't have to import a new trait everywhere.
Also, if i understand correctly, that new trait would only be implemented for LocalDefId/DefId. What about other key types? Using Into makes it more general, which may or may not be good.

useful because we don't have to import a new trait everywhere.

I was imagining an IntoQueryKey trait which would be private/sealed so that we couldn't import it anywhere, i.e. these aren't conversions we'd let people do by calling .into_query_key() (which has all the same issues .into() does).

I don't think to_def_id() adds anything over into(), since anecdotally most variables of type LocalDefId are named def_id anyways. We could always start with a custom trait and later migrate to Into if you still have concerns. We would keep an inherent method on LocalDefId for converting to a DefId so additional trait imports shouldn't be a factor.

Zoxc · 2020-04-09T15:35:48Z

src/librustc_middle/ty/query/plumbing.rs

-            pub fn $name(self, key: $K) {
-                ensure_query::<queries::$name<'_>, _>(self.tcx, key)
+            pub fn $name(self, key: impl Into<$K>) {
+                ensure_query::<queries::$name<'_>, _>(self.tcx, key.into())


I'd make some new methods that call the existing ones in this file to avoid making these generic. We want to make sure that ensure_query and get_query is only instantiated in rustc_middle.

Just that i understand; the proposal here is to add new methods only for methods where the key is a DefId and give them a name similar to the original one but with a prefix (for example). We would end up with methods like local_generics_of for LocalDefId instead of generics_of for DefId?

If so, all callers that have a LocalDefId would have to use the new methods, and then there is not much benefit compared to calling .to_def_id() everywhere. All code will have to be changed anyway, either to call the new methods or to call .to_def_id().

I believe they want the caller of ensure_query to remain monomorphic and for you to introduce a generic wrapper around it that calls .into() on the query param before passing it to the monomorphic version.

ecstatic-morse · 2020-04-09T17:41:05Z

I opened #70961, which implements the approach that I had in mind for this. Notably, only queries that accept a DefId become generic. My hope is that this makes using Into instead of a hypothetical IntoQueryParam more feasible. I also made sure that the caller of ensure_query was monomorphic to address Zoxc's concern.

If the additional macro complexity required for my PR is not acceptable, we should go with some version of this one.

eddyb · 2020-04-09T17:47:20Z

My hope is that this makes using Into instead of a hypothetical IntoQueryParam more feasible

Into doesn't stop people from writing .into() everywhere though, in cases where we might want the conversion to be explicit.

ecstatic-morse · 2020-04-09T17:55:43Z

Agreed, I just don't think that def_id.into() is less understandable than def_id.to_def_id(). See my reply above. I get types on hover though, so maybe I'm biased.

eddyb · 2020-04-09T18:21:51Z

@ecstatic-morse Heh, for me it's not about understanding but rather refactorability.
You can search for .to_def_id() now, you can't easily search for .into() applied on a LocalDefId.

ecstatic-morse · 2020-04-09T18:32:47Z

That's fair. Let's go with a custom trait for now and we can re-evaluate later. I'm hoping that the series of PRs addressing #70853 are the last time we will need to do such a refactoring, but I could be wrong.

nikomatsakis · 2020-04-10T17:28:35Z

r? @eddyb

…e-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.

…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.

marmeladema · 2020-04-12T14:28:12Z

Lets close this in favor of #70961

Take `impl Into<DefId>` for query methods on `TyCtxt` Alternative implementation of rust-lang#70956. cc rust-lang#70853.

marmeladema added 2 commits April 9, 2020 15:15

librustc_middle: change query functions to accept impl Into<$K> type

e3bf24f

This should help to increase the usage of LocalDefId instead of DefId.

librustc_span: implement From<LocalDefId> for DefId

3fe4327

Some now useless calls to `.to_def_id()` have been removed as example of usefulness of the change.

rust-highfive assigned petrochenkov Apr 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2020

rust-highfive assigned Zoxc and unassigned petrochenkov Apr 9, 2020

eddyb reviewed Apr 9, 2020

View reviewed changes

Zoxc reviewed Apr 9, 2020

View reviewed changes

ecstatic-morse mentioned this pull request Apr 9, 2020

Take impl Into<DefId> for query methods on TyCtxt #70961

Merged

marmeladema mentioned this pull request Apr 10, 2020

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

Merged

nikomatsakis unassigned Zoxc Apr 10, 2020

rust-highfive assigned eddyb Apr 10, 2020

marmeladema closed this Apr 12, 2020

marmeladema deleted the queries/impl-into-def-id branch April 12, 2020 14:28

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2020

Auto merge of rust-lang#70961 - ecstatic-morse:into-def-id, r=eddyb

5179ebe

Take `impl Into<DefId>` for query methods on `TyCtxt` Alternative implementation of rust-lang#70956. cc rust-lang#70853.

jyn514 mentioned this pull request Jun 7, 2020

impl From<LocalDefId> for DefId #73076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librustc_middle: change query functions to accept impl Into<$K> type #70956

librustc_middle: change query functions to accept impl Into<$K> type #70956

marmeladema commented Apr 9, 2020

rust-highfive commented Apr 9, 2020

mark-i-m commented Apr 9, 2020

eddyb commented Apr 9, 2020

eddyb Apr 9, 2020

marmeladema Apr 9, 2020

eddyb Apr 9, 2020

ecstatic-morse Apr 9, 2020 •

edited

Loading

Zoxc Apr 9, 2020

marmeladema Apr 9, 2020

ecstatic-morse Apr 9, 2020 •

edited

Loading

ecstatic-morse commented Apr 9, 2020 •

edited

Loading

eddyb commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020 •

edited

Loading

eddyb commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020

nikomatsakis commented Apr 10, 2020

marmeladema commented Apr 12, 2020

librustc_middle: change query functions to accept impl Into<$K> type #70956

librustc_middle: change query functions to accept impl Into<$K> type #70956

Conversation

marmeladema commented Apr 9, 2020

rust-highfive commented Apr 9, 2020

mark-i-m commented Apr 9, 2020

eddyb commented Apr 9, 2020

eddyb Apr 9, 2020

Choose a reason for hiding this comment

marmeladema Apr 9, 2020

Choose a reason for hiding this comment

eddyb Apr 9, 2020

Choose a reason for hiding this comment

ecstatic-morse Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Zoxc Apr 9, 2020

Choose a reason for hiding this comment

marmeladema Apr 9, 2020

Choose a reason for hiding this comment

ecstatic-morse Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Apr 9, 2020 • edited Loading

eddyb commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020 • edited Loading

eddyb commented Apr 9, 2020

ecstatic-morse commented Apr 9, 2020

nikomatsakis commented Apr 10, 2020

marmeladema commented Apr 12, 2020

ecstatic-morse Apr 9, 2020 •

edited

Loading

ecstatic-morse Apr 9, 2020 •

edited

Loading

ecstatic-morse commented Apr 9, 2020 •

edited

Loading

ecstatic-morse commented Apr 9, 2020 •

edited

Loading