Remove (useless) argument of entry_fn query by marmeladema · Pull Request #71648 · rust-lang/rust

marmeladema · 2020-04-28T16:37:30Z

This should be fine because argument was asserted to always be
LOCAL_CRATE. Moreover the query returns LocalDefId which makes
it clear that this query is about the current local crate.

This should be fine because argument was asserted to always be LOCAL_CRATE. Moreover the query returns `LocalDefId` which makes it clear that this query is about the current local crate.

rust-highfive · 2020-04-28T16:37:34Z

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

marmeladema · 2020-04-28T16:37:47Z

r? @eddyb

eddyb · 2020-04-28T16:41:02Z

src/librustc_middle/ty/query/keys.rs

    fn default_span(&self, tcx: TyCtxt<'_>) -> Span;
 }

+impl Key for () {


Wait, this is new? I thought there was precedent for this. I guess we can still go ahead with this.

I checked, no query have () as argument/key type yet, i think.

eddyb · 2020-04-28T16:41:47Z

cc @rust-lang/compiler We could probably do this for all the queries that are local-crate analyses like "typeck everything" and "borrowck everything" etc., what do you think?

nikomatsakis · 2020-04-28T16:47:16Z

@eddyb I have no objection. We originally included the crate because we wanted to be moving away from the idea of having a "local crate" that is being compiled -- but I think that's easy to change later. Also, there will always have to be some notion of "local crate", since some things -- like coherence rules -- depend on what the "current crate" is, although I imagine those things will always be associated with some def-id.

eddyb · 2020-04-28T16:50:18Z

I think it's simpler than that, the query system started out with DefId and CrateNum keys, and the cratenum from either of those was used to look up the providers. We only added CrateNum-less key types like Ty after that.

ecstatic-morse · 2020-04-28T16:56:38Z

src/librustc_codegen_ssa/back/symbol_export.rs

        .collect();

-    if tcx.entry_fn(LOCAL_CRATE).is_some() {
+    if tcx.entry_fn(()).is_some() {


Can we give a name (LocalCrate) to the unit struct that implements query::Key? Otherwise, it's not immediately clear to me what this line is doing.

That seems like a good idea to me. @eddyb do you agree?

That's one way. But this is basically a nulary query.
If we tweaked the macros a bit we could write this instead:

if tcx.entry_fn().is_some() {

If we go that route, we should rename the entry_fn query to entry_fn_local_crate. Otherwise the same readability issues apply.

I wonder if adding an improved doc comment would suffice? It seems reasonable clear to me that the entry_fn computes, well, the entry function -- I guess the thing that is sort of non-obvious is the overall model where the root crate has an entry function, and it seems like that would be best explained in docs?

In particular, entry_fn_for_local_crate (if we're going to use a long name, I'd prefer to make it read well...) seems to imply that other crates can have entry functions, which isn't really true?

Anyway, a minor thing, but I guess it may become a more wide-spread convention either way.

oli-obk · 2020-04-29T10:40:49Z

src/librustc_middle/query/mod.rs

+        /// Identifies the entry-point (e.g., the `main` function) for the current
        /// crate, returning `None` if there is no entry point (such as for library crates).
-        query entry_fn(_: CrateNum) -> Option<(LocalDefId, EntryFnType)> {
+        query entry_fn(_: ()) -> Option<(LocalDefId, EntryFnType)> {


Won't this query only get executed once in the entire dependency tree? So if a lib crate calls the query, you get None and then all other crates just yield that None?

bors · 2020-05-03T03:10:45Z

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

Elinvynia · 2020-05-27T18:07:50Z

Ping from Triage: @marmeladema Hello, any updates on this PR?

marmeladema · 2020-05-27T18:17:31Z

@Elinvynia it appears there is not really a consensus about what the prototype of such query should be. Possibilities are:

Keep it as proposed in this PR with explicit unit struct as argument tcx.entry_fn(())
Use an empty struct and implement query::Key trait: tcx.entry_fn(LocalCrate)
Modify the query macros to be able to define queries without argument: tcx.entry_fn()

crlf0710 · 2020-07-10T09:10:38Z

@marmeladema Ping from triage! This has merge conflicts now, and it seems the design discussion has stalled. Could you fix the merge conflicts so i can switch this back to S-waiting-on-review state?

By the way, I think you could also open a topic on t-compiler in Zulip to push resolving the API design issue forward.

oli-obk · 2020-07-10T10:08:31Z

Modify the query macros to be able to define queries without argument: tcx.entry_fn()

I think this is the final goal. I'd be fine with merging the current PR with a LocalCrate ZST for the query argument and then doing the removal in a separate PR if that's preferrable to you. Otherwise, let's go full nullary query and allow queries to not have arguments at all right away in this PR.

oli-obk · 2020-07-10T10:09:26Z

r? @oli-obk

Dylan-DPC-zz · 2020-08-03T01:11:28Z

@marmeladema any updates on this?

marmeladema · 2020-08-08T01:00:25Z

I will try to modify the macro in order to be able to define queries without argument 👍

JohnCSimon · 2020-08-25T05:43:53Z

note from triage
@marmeladema - this now has merge conflicts and hasn't seen any commits in a while. Thank you.

crlf0710 · 2020-09-11T16:26:30Z

@marmeladema Ping from triage! I'm going to close this due to inactivity. Feel free to reopen or open a new PR when you get to continue working on this. Thanks!

Remove (useless) argument of entry_fn query

1e50901

This should be fine because argument was asserted to always be LOCAL_CRATE. Moreover the query returns `LocalDefId` which makes it clear that this query is about the current local crate.

rust-highfive assigned matthewjasper Apr 28, 2020

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

rust-highfive assigned eddyb and unassigned matthewjasper Apr 28, 2020

eddyb reviewed Apr 28, 2020

View reviewed changes

ecstatic-morse reviewed Apr 28, 2020

View reviewed changes

oli-obk reviewed Apr 29, 2020

View reviewed changes

Elinvynia added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2020

Elinvynia added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2020

Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2020

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2020

crlf0710 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 10, 2020

crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 10, 2020

rust-highfive assigned oli-obk and unassigned eddyb Jul 10, 2020

Elinvynia added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2020

crlf0710 closed this Sep 11, 2020

marmeladema mentioned this pull request May 11, 2021

Remove CrateNum parameter for queries that only work on local crate #85178

Merged

Uh oh!

Conversation

marmeladema commented Apr 28, 2020

Uh oh!

rust-highfive commented Apr 28, 2020

Uh oh!

marmeladema commented Apr 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Apr 28, 2020

Uh oh!

eddyb commented Apr 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 3, 2020

Uh oh!

Elinvynia commented May 27, 2020

Uh oh!

marmeladema commented May 27, 2020

Uh oh!

crlf0710 commented Jul 10, 2020

Uh oh!

oli-obk commented Jul 10, 2020

Uh oh!

oli-obk commented Jul 10, 2020

Uh oh!

Dylan-DPC-zz commented Aug 3, 2020

Uh oh!

marmeladema commented Aug 8, 2020

Uh oh!

JohnCSimon commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crlf0710 commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

eddyb commented Apr 28, 2020 •

edited

Loading

JohnCSimon commented Aug 25, 2020 •

edited

Loading