Rename -Zquery-dep-graph to -Zretain-dep-graph#152836
Rename -Zquery-dep-graph to -Zretain-dep-graph#152836Zalathar wants to merge 1 commit intorust-lang:mainfrom
-Zquery-dep-graph to -Zretain-dep-graph#152836Conversation
|
Some changes occurred in compiler/rustc_attr_parsing |
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| if !if_this_changed.is_empty() || !then_this_would_need.is_empty() { | ||
| assert!( | ||
| tcx.sess.opts.unstable_opts.query_dep_graph, | ||
| "cannot use the `#[{}]` or `#[{}]` annotations \ | ||
| without supplying `-Z query-dep-graph`", | ||
| sym::rustc_if_this_changed, | ||
| sym::rustc_then_this_would_need | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This assertion can't be hit, because the enclosing function will have already returned early if the flag isn't set.
That, combined with the fact that the attribute parser for these attributes requires the flag anyway, makes it seem pretty safe to get rid of this assertion.
| #[diag("found CGU-reuse attribute but `-Zretain-dep-graph` was not specified")] | ||
| pub(crate) struct MissingRetainDepGraph { |
There was a problem hiding this comment.
This error could potentially be unified with AttributeRequiresRetainDepGraph by checking for the flag in the parser for the relevant attributes, but I decided not to pursue that extra work in this PR.
| #[diag("attribute requires `-Zretain-dep-graph` to be enabled")] | ||
| pub(crate) struct AttributeRequiresRetainDepGraph { |
There was a problem hiding this comment.
This error is only ever used by attributes that require -Zretain-dep-graph, so I took the liberty of making it more specific, to avoid having to repeat the "-Zretain-dep-graph" string at multiple call sites.
|
I get that this might be an improvement in the naming, but I'm not sure if it's worth the churn. The flag is being used by people in local scripts and tools, and this will break them. Of course, the flag is unstable, so we can do that, but doing that purely for coming up with a slightly better name, especially since we don't really ever plan to stabilize this flag (?), is IMO just not worth it. |
|
I'm surprised to hear that anyone is using this out-of-tree. I can find very little indication anywhere that it even exists. If churn is a concern, I can narrow this to just the internal renaming. That was my main motivation, and I only renamed the flag because I saw no reason not to do so at the same time. |
|
rustc-perf uses it, I have some not-yet-published Cargo tooling for analyzing incremental builds that uses this, and I'm sure that various rustc contributors have this embedded in their local bash scripts 😆 Of course this wouldn't be a massive disruption to the nightly ecosystem or anything like that, but still. That being said, I went through https://github.com/search?type=code&q=%22-Zquery-dep-graph%22 and except for rustc forks I did not find much else form what I was mentioning here, so the fallout is probably rather low. |
|
Hmm, if people are mostly using it because it's required by (There would still be some churn, so in that scenario I wouldn't necessarily want to rename I think I'll replace this PR with one that only does the internal renaming, since that's the main thing I care about at the moment. @rustbot author |
…hanBrouwer Rename `DepGraphQuery` to `RetainedDepGraph` This is a revised subset of rust-lang#152836 that only performs an internal renaming, and does not touch the `-Zquery-dep-graph` flag. The new name and comments for `RetainedDepGraph` should hopefully do a better job of communicating that it is not used in normal compiler operation, even in incremental mode.
Rollup merge of #152844 - Zalathar:retain-dep-graph, r=JonathanBrouwer Rename `DepGraphQuery` to `RetainedDepGraph` This is a revised subset of #152836 that only performs an internal renaming, and does not touch the `-Zquery-dep-graph` flag. The new name and comments for `RetainedDepGraph` should hopefully do a better job of communicating that it is not used in normal compiler operation, even in incremental mode.
This obscure debugging/testing-only flag and its associated module and type have very confusing names.
It's unclear whether “query” is intended as a noun (referring to queries in the query system) or as a verb (to ask for information). And the names of the module/type (
rustc_middle::dep_graph::query::DepGraphQuery) suggest a greater level of importance than this code actually has.It's hard to come up with a clearly-good name for this flag, but I think
-Zretain-dep-graphis a solid improvement over the status quo.