rustfmt: Discover modules via cfg_select!#158372
Conversation
|
cc @rust-lang/rustfmt |
rustbot
added
S-waiting-on-review
|
r? @ytmimi rustbot has assigned @ytmimi. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot label +beta-nominated
|
rustbot
added
the
beta-nominated
| test => { | ||
| mod format_me_please_1; | ||
| } | ||
| target_family = "unix" => { | ||
| mod format_me_please_2; | ||
| } | ||
| cfg(target_pointer_width = "32") => { |
There was a problem hiding this comment.
Thought: doesn't this change stable formatting? I guess it's technically a bug fix so maybe this is... acceptable re. formatting stability guarantees?
Hm, I guess technically we could consider it... "stable but unformatted"
There was a problem hiding this comment.
stable but unformatted
I believe that was the precedent set by let-else.
There was a problem hiding this comment.
Any changes to cfg_select! bodies from #154202 are probably going to be way less substantial than having entire unformatted modules trees, no?
If that PR looked closer to ready then I'd agree we should wait. It As-is though, anyone migrating from cfg_if is going to start accumulating code that's out of formatting sync (possibly without noticing), and that seems like a hole worth plugging.
There was a problem hiding this comment.
Regarding the changes in the test files: only renaming cfg_match! to cfg_select! is required for the tests to work. The following other changes just update the test to be more modern and to compile successfully (which is not tested):
- removing the nightly feature
- removing
std::before the Marco - migrating away from the removed
cfg()syntax
The last point specifically still works for module file discovery, even though it does not compile. The parsing machinery can probably drop support for that but I did not remove it to keep the PR minimal.
|
Overall I think this is a good change, and although it would technically be breaking since we'd end up formatting modules listed inside the |
There was a problem hiding this comment.
I think there are still a few references to the old cfg_match! that we should change. Once that's done and we figure out how we'll disclose this breaking change you can r=me.
I think it's fine to get this out before cfg_select! formatting lands since I agree that the impact of entire modules being left unformatted is larger than what will change once we start formatting cfg_select! calls.
| use crate::attr::MetaVisitor; | ||
| use crate::parse::macros::cfg_if::parse_cfg_if; | ||
| use crate::parse::macros::cfg_match::parse_cfg_match; | ||
| use crate::parse::macros::cfg_select::parse_cfg_match; |
There was a problem hiding this comment.
Shouldn't we also update the name from parse_cfg_match -> parse_cfg_select?
There was a problem hiding this comment.
Thanks for catching this! Done. 👍
|
|
||
| /// Traverse `cfg_match!` macro and fetch modules. | ||
| /// Traverse `cfg_select!` macro and fetch modules. | ||
| pub(crate) struct CfgMatchVisitor<'a> { |
There was a problem hiding this comment.
Shouldn't we also rename CfgMatchVisitor to CfgSelectVisitor?
There was a problem hiding this comment.
Thanks for catching this! Done. 👍
mkroening
force-pushed
the
rustfmt-cfg_select-modules
branch
from
3ba2282 to
a71b047
Compare
This comment has been minimized.
This comment has been minimized.
mkroening
force-pushed
the
rustfmt-cfg_select-modules
branch
from
a71b047 to
4e98db3
Compare
mkroening
force-pushed
the
rustfmt-cfg_select-modules
branch
from
4e98db3 to
facc787
Compare
8 participants
This PR renames all occurrences of
cfg_match!in rustfmt tocfg_select!. This makes the module file detection logic from rust-lang/rustfmt#6522 kick in forcfg_select!instead ofcfg_match!, which no longer exists.This PR performs no other adjustments to the logic to be as small as possible.
I am opening this PR in this repo since that is also the target for the more comprehensive #154202, which covers more than merely detecting other files through
cfg_select!.Closes #158371.
CC: @ytmimi, @CAD97