-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Convert more queries to use LocalDefId
#71292
Conversation
r? @eddyb |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit c17dd894ac6861fe689baf6ba16499de4de788dd with merge a70b0e7f3482873f39970306a89e352ead61b4f5... |
☀️ Try build successful - checks-azure |
Queued a70b0e7f3482873f39970306a89e352ead61b4f5 with parent 339a938, future comparison URL. |
c17dd89
to
f1a43bf
Compare
☔ The latest upstream changes (presumably #71231) made this pull request unmergeable. Please resolve the merge conflicts. |
f1a43bf
to
559236a
Compare
☔ The latest upstream changes (presumably #71044) made this pull request unmergeable. Please resolve the merge conflicts. |
559236a
to
a7e411e
Compare
☔ The latest upstream changes (presumably #71467) made this pull request unmergeable. Please resolve the merge conflicts. |
a7e411e
to
67cebf2
Compare
67cebf2
to
90639e2
Compare
@bors r+ |
📌 Commit 90639e2 has been approved by |
…=eddyb Convert more queries to use `LocalDefId` This PR is based on commits in rust-lang#71215 and should partially solve rust-lang#70853
90639e2
to
1349272
Compare
@Dylan-DPC @eddyb i just rebased over master and fixed the nits. |
@bors r=eddyb |
📌 Commit 1349272 has been approved by |
☀️ Test successful - checks-azure |
Tested on commit rust-lang/rust@fb5615a. Direct link to PR: <rust-lang/rust#71292> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
@RalfJung
Sorry for the trouble |
@marmeladema thanks for the notification! |
@@ -878,7 +886,7 @@ rustc_queries! { | |||
|
|||
/// Identifies the entry-point (e.g., the `main` function) for a given | |||
/// crate, returning `None` if there is no entry point (such as for library crates). | |||
query entry_fn(_: CrateNum) -> Option<(DefId, EntryFnType)> { | |||
query entry_fn(_: CrateNum) -> Option<(LocalDefId, EntryFnType)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by this. I thought LocalDefId
means "in the local crate", but entry_fn
takes a CrateNum
parameter and thus might return something in a different crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the query is cached, if it is called from another crate, it'll just fetch the cached version. This kind of trickery is used a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nevermind, that's wrong apparently, at least I'm reading @eddyb's comment below to meaning these are actually no-argument crates
@@ -48,7 +48,7 @@ impl<'a, 'tcx> ItemLikeVisitor<'tcx> for EntryContext<'a, 'tcx> { | |||
} | |||
} | |||
|
|||
fn entry_fn(tcx: TyCtxt<'_>, cnum: CrateNum) -> Option<(DefId, EntryFnType)> { | |||
fn entry_fn(tcx: TyCtxt<'_>, cnum: CrateNum) -> Option<(LocalDefId, EntryFnType)> { | |||
assert_eq!(cnum, LOCAL_CRATE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol, okay this explains why the return type can be LocalDefId
. Maybe the parameter should be removed, if there is only one possible value anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes ultimately we should remove the the param, but i haven't tried yet. Allegedly ,the query macros do not handle "no key at all" in a query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to look at it. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allegedly ,the query macros do not handle "no key at all" in a query
As a work-around, the key could be of type ()
. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung query keys of type ()
are new, at the start the system only supported DefId
and CrateNum
, so queries that always take LOCAL_CRATE
are an artifact of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also make the query system variadic by auto-tupling behind the scenes and hiding the tupling from the tcx
methods and the providers :D.
rustup rust-lang/rust#71292 cc rust-lang/rust#71608 --- changelog: none
rustup rust-lang/rust#71292 cc rust-lang/rust#71608 --- changelog: none
This PR is based on commits in #71215 and should partially solve #70853