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

Convert more queries to use LocalDefId #71292

Merged
merged 17 commits into from
Apr 28, 2020

Conversation

marmeladema
Copy link
Contributor

This PR is based on commits in #71215 and should partially solve #70853

@marmeladema
Copy link
Contributor Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned davidtwco Apr 18, 2020
@eddyb
Copy link
Member

eddyb commented Apr 18, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 18, 2020

⌛ Trying commit c17dd894ac6861fe689baf6ba16499de4de788dd with merge a70b0e7f3482873f39970306a89e352ead61b4f5...

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2020
@bors
Copy link
Contributor

bors commented Apr 18, 2020

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

@rust-timer
Copy link
Collaborator

Queued a70b0e7f3482873f39970306a89e352ead61b4f5 with parent 339a938, future comparison URL.

@bors
Copy link
Contributor

bors commented Apr 19, 2020

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

@bors
Copy link
Contributor

bors commented Apr 23, 2020

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

@bors
Copy link
Contributor

bors commented Apr 23, 2020

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

@marmeladema
Copy link
Contributor Author

@eddyb #71215 has landed! Do you think you will have some time to review this one?

@eddyb
Copy link
Member

eddyb commented Apr 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Apr 27, 2020

📌 Commit 90639e2 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 27, 2020
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
@marmeladema
Copy link
Contributor Author

@Dylan-DPC @eddyb i just rebased over master and fixed the nits.

@Dylan-DPC-zz
Copy link

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 27, 2020

📌 Commit 1349272 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 27, 2020
@bors
Copy link
Contributor

bors commented Apr 28, 2020

⌛ Testing commit 1349272 with merge fb5615a...

@bors
Copy link
Contributor

bors commented Apr 28, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2020
@bors bors merged commit fb5615a into rust-lang:master Apr 28, 2020
@marmeladema marmeladema deleted the queries-local-def-id branch April 28, 2020 08:18
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #71292!

Tested on commit fb5615a.
Direct link to PR: #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).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 28, 2020
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).
@marmeladema
Copy link
Contributor Author

@RalfJung
I think I broke miri again:

error[E0308]: mismatched types
  --> src\tools\miri\src\bin\miri.rs:42:61
   |
42 |             if let Some(return_code) = miri::eval_main(tcx, entry_def_id, config) {
   |                                                             ^^^^^^^^^^^^ expected struct `rustc_hir::def_id::DefId`, found struct `rustc_hir::def_id::LocalDefId`

error: aborting due to previous error

Sorry for the trouble

@RalfJung
Copy link
Member

@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)> {
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Contributor

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);
Copy link
Member

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?

Copy link
Contributor Author

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

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'll try to look at it. 👍

Copy link
Member

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 (). ;)

Copy link
Member

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.

Copy link
Member

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.

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Apr 28, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 28, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 28, 2020
ThibsG pushed a commit to ThibsG/rust-clippy that referenced this pull request May 2, 2020
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.

10 participants