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

Extend cfg_if! support to cfg_match! #6522

Merged
merged 3 commits into from
Apr 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/modules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,25 @@ impl<'ast, 'psess, 'c> ModResolver<'ast, 'psess> {
Ok(())
}

fn visit_cfg_match(&mut self, item: Cow<'ast, ast::Item>) -> Result<(), ModuleResolutionError> {
let mut visitor = visitor::CfgMatchVisitor::new(self.psess);
visitor.visit_item(&item);
for module_item in visitor.mods() {
if let ast::ItemKind::Mod(_, _, ref sub_mod_kind) = module_item.item.kind {
self.visit_sub_mod(
&module_item.item,
Module::new(
module_item.item.span,
Some(Cow::Owned(sub_mod_kind.clone())),
Cow::Owned(ThinVec::new()),
Cow::Owned(ast::AttrVec::new()),
),
)?;
}
}
Ok(())
}

/// Visit modules defined inside macro calls.
fn visit_mod_outside_ast(
&mut self,
Expand All @@ -178,6 +197,11 @@ impl<'ast, 'psess, 'c> ModResolver<'ast, 'psess> {
continue;
}

if is_cfg_match(&item) {
self.visit_cfg_match(Cow::Owned(item.into_inner()))?;
continue;
}

if let ast::ItemKind::Mod(_, _, ref sub_mod_kind) = item.kind {
let span = item.span;
self.visit_sub_mod(
Expand All @@ -204,6 +228,10 @@ impl<'ast, 'psess, 'c> ModResolver<'ast, 'psess> {
self.visit_cfg_if(Cow::Borrowed(item))?;
}

if is_cfg_match(item) {
self.visit_cfg_match(Cow::Borrowed(item))?;
}

if let ast::ItemKind::Mod(_, _, ref sub_mod_kind) = item.kind {
let span = item.span;
self.visit_sub_mod(
Expand Down Expand Up @@ -575,3 +603,17 @@ fn is_cfg_if(item: &ast::Item) -> bool {
_ => false,
}
}

fn is_cfg_match(item: &ast::Item) -> bool {
match item.kind {
ast::ItemKind::MacCall(ref mac) => {
if let Some(last_segment) = mac.path.segments.last() {
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 want to draw attention to this: cfg_match! is checking that the last path segment is cfg_match! (i.e. tries to process any::path::cfg_match!), whereas cfg_if! is checking that the first path segment is cfg_if! (i.e. tries to process cfg_if::any::path!). I think my behavior is more correct, and the worst case scenario is that some more files get discovered when a custom cfg_match! that looks similar enough to core::cfg_match! is used, but it bears pointing out.

If factoring this behavior out to be the same between the two is desirable, I can do that, and would choose to check the macro-ident, not the path front. (The current cfg_if! processing will skip ::cfg_if::cfg_if!, since the path starts with kw::PathRoot.)

Copy link
Contributor Author

@CAD97 CAD97 Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and wrote the patch to unify the predicate: a129548

The visitor could be further refactored into a single KnownMacroVisitor which processes both cfg_if! and cfg_match!, but that starts to feel a bit excessive...

I haven't included it in this PR since it's behavior changing for the existing cfg_if! support, to keep this PR scoped to just the cfg_match! support.

Copy link
Contributor Author

@CAD97 CAD97 Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and I ended up doing that cleanup as well: 4ae9dff

But I'm actually done doing extended cleanup for this now 😅

To be fully explicit about it: the two linked commits in this and the prior comment are made available under the same license terms as if I had directly contributed them by placing them in a PR. Feel free to pull them (or ask me to PR them) if you believe them to be an improvement.

if last_segment.ident.name == Symbol::intern("cfg_match") {
return true;
}
}
false
}
_ => false,
}
}
60 changes: 60 additions & 0 deletions src/modules/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use tracing::debug;

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::session::ParseSess;

pub(crate) struct ModItem {
Expand Down Expand Up @@ -71,6 +72,65 @@ impl<'a, 'ast: 'a> CfgIfVisitor<'a> {
}
}

/// Traverse `cfg_match!` macro and fetch modules.
pub(crate) struct CfgMatchVisitor<'a> {
psess: &'a ParseSess,
mods: Vec<ModItem>,
}

impl<'a> CfgMatchVisitor<'a> {
pub(crate) fn new(psess: &'a ParseSess) -> CfgMatchVisitor<'a> {
CfgMatchVisitor {
mods: vec![],
psess,
}
}

pub(crate) fn mods(self) -> Vec<ModItem> {
self.mods
}
}

impl<'a, 'ast: 'a> Visitor<'ast> for CfgMatchVisitor<'a> {
fn visit_mac_call(&mut self, mac: &'ast ast::MacCall) {
match self.visit_mac_inner(mac) {
Ok(()) => (),
Err(e) => debug!("{}", e),
}
}
}

impl<'a, 'ast: 'a> CfgMatchVisitor<'a> {
fn visit_mac_inner(&mut self, mac: &'ast ast::MacCall) -> Result<(), &'static str> {
// Support both:
// ```
// std::cfg_match! {..}
// core::cfg_match! {..}
// ```
// And:
// ```
// use std::cfg_match;
// cfg_match! {..}
// ```
match mac.path.segments.last() {
Some(last_segment) => {
if last_segment.ident.name != Symbol::intern("cfg_match") {
return Err("Expected cfg_match");
}
}
None => {
return Err("Expected cfg_match");
}
};

let items = parse_cfg_match(self.psess, mac)?;
self.mods
.append(&mut items.into_iter().map(|item| ModItem { item }).collect());

Ok(())
}
}

/// Extracts `path = "foo.rs"` from attributes.
#[derive(Default)]
pub(crate) struct PathVisitor {
Expand Down
80 changes: 80 additions & 0 deletions src/parse/macros/cfg_match.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
use std::panic::{AssertUnwindSafe, catch_unwind};

use rustc_ast::ast;
use rustc_ast::token::{Delimiter, TokenKind};
use rustc_parse::exp;
use rustc_parse::parser::ForceCollect;

use crate::parse::macros::build_stream_parser;
use crate::parse::session::ParseSess;

pub(crate) fn parse_cfg_match<'a>(
psess: &'a ParseSess,
mac: &'a ast::MacCall,
) -> Result<Vec<ast::Item>, &'static str> {
match catch_unwind(AssertUnwindSafe(|| parse_cfg_match_inner(psess, mac))) {
Ok(Ok(items)) => Ok(items),
Ok(err @ Err(_)) => err,
Err(..) => Err("failed to parse cfg_match!"),
}
}

fn parse_cfg_match_inner<'a>(
psess: &'a ParseSess,
mac: &'a ast::MacCall,
) -> Result<Vec<ast::Item>, &'static str> {
let ts = mac.args.tokens.clone();
let mut parser = build_stream_parser(psess.inner(), ts);

if parser.token == TokenKind::OpenDelim(Delimiter::Brace) {
return Err("Expression position cfg_match! not yet supported");
}

let mut items = vec![];

while parser.token.kind != TokenKind::Eof {
if !parser.eat_keyword(exp!(Underscore)) {
parser.parse_attr_item(ForceCollect::No).map_err(|e| {
e.cancel();
"Failed to parse attr item"
})?;
}

if !parser.eat(exp!(FatArrow)) {
return Err("Expected a fat arrow");
}

if !parser.eat(exp!(OpenBrace)) {
return Err("Expected an opening brace");
}

while parser.token != TokenKind::CloseDelim(Delimiter::Brace)
&& parser.token.kind != TokenKind::Eof
{
let item = match parser.parse_item(ForceCollect::No) {
Ok(Some(item_ptr)) => item_ptr.into_inner(),
Ok(None) => continue,
Err(err) => {
err.cancel();
parser.psess.dcx().reset_err_count();
return Err(
"Expected item inside cfg_match block, but failed to parse it as an item",
);
}
};
if let ast::ItemKind::Mod(..) = item.kind {
items.push(item);
}
}

if !parser.eat(exp!(CloseBrace)) {
return Err("Expected a closing brace");
}

if parser.eat(exp!(Eof)) {
break;
}
}

Ok(items)
}
1 change: 1 addition & 0 deletions src/parse/macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::rewrite::RewriteContext;

pub(crate) mod asm;
pub(crate) mod cfg_if;
pub(crate) mod cfg_match;
pub(crate) mod lazy_static;

fn build_stream_parser<'a>(psess: &'a ParseSess, tokens: TokenStream) -> Parser<'a> {
Expand Down
41 changes: 41 additions & 0 deletions src/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const FILE_SKIP_LIST: &[&str] = &[
"issue-3253/foo.rs",
"issue-3253/bar.rs",
"issue-3253/paths",
// This directory is directly tested by format_files_find_new_files_via_cfg_match
"cfg_match",
// These files and directory are a part of modules defined inside `cfg_attr(..)`.
"cfg_mod/dir",
"cfg_mod/bar.rs",
Expand Down Expand Up @@ -468,6 +470,45 @@ fn format_files_find_new_files_via_cfg_if() {
});
}

#[test]
fn format_files_find_new_files_via_cfg_match() {
init_log();
run_test_with(&TestSetting::default(), || {
// We load these two files into the same session to test cfg_match!
// transparent mod discovery, and to ensure that it does not suffer
// from a similar issue as cfg_if! support did with issue-4656.
let files = vec![
Path::new("tests/source/cfg_match/lib2.rs"),
Path::new("tests/source/cfg_match/lib.rs"),
];

let config = Config::default();
let mut session = Session::<io::Stdout>::new(config, None);

let mut write_result = HashMap::new();
for file in files {
assert!(file.exists());
let result = session.format(Input::File(file.into())).unwrap();
assert!(!session.has_formatting_errors());
assert!(!result.has_warnings());
let mut source_file = SourceFile::new();
mem::swap(&mut session.source_file, &mut source_file);

for (filename, text) in source_file {
if let FileName::Real(ref filename) = filename {
write_result.insert(filename.to_owned(), text);
}
}
}
assert_eq!(
6,
write_result.len(),
"Should have uncovered an extra file (format_me_please_x.rs) via lib.rs"
);
assert!(handle_result(write_result, None).is_ok());
});
}

#[test]
fn stdin_formatting_smoke_test() {
init_log();
Expand Down
2 changes: 2 additions & 0 deletions tests/source/cfg_match/format_me_please_1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

pub fn format_me_please_1( ) { }
2 changes: 2 additions & 0 deletions tests/source/cfg_match/format_me_please_2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

pub fn format_me_please_2( ) { }
2 changes: 2 additions & 0 deletions tests/source/cfg_match/format_me_please_3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

pub fn format_me_please_3( ) { }
2 changes: 2 additions & 0 deletions tests/source/cfg_match/format_me_please_4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

pub fn format_me_please_4( ) { }
16 changes: 16 additions & 0 deletions tests/source/cfg_match/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#![feature(cfg_match)]

std::cfg_match! {
test => {
mod format_me_please_1;
}
target_family = "unix" => {
mod format_me_please_2;
}
cfg(target_pointer_width = "32") => {
mod format_me_please_3;
}
_ => {
mod format_me_please_4;
}
}
3 changes: 3 additions & 0 deletions tests/source/cfg_match/lib2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
its_a_macro! {
// Contents
}
1 change: 1 addition & 0 deletions tests/target/cfg_match/format_me_please_1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn format_me_please_1() {}
1 change: 1 addition & 0 deletions tests/target/cfg_match/format_me_please_2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn format_me_please_2() {}
1 change: 1 addition & 0 deletions tests/target/cfg_match/format_me_please_3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn format_me_please_3() {}
1 change: 1 addition & 0 deletions tests/target/cfg_match/format_me_please_4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn format_me_please_4() {}
16 changes: 16 additions & 0 deletions tests/target/cfg_match/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#![feature(cfg_match)]

std::cfg_match! {
test => {
mod format_me_please_1;
}
target_family = "unix" => {
mod format_me_please_2;
}
cfg(target_pointer_width = "32") => {
mod format_me_please_3;
}
_ => {
mod format_me_please_4;
}
}
3 changes: 3 additions & 0 deletions tests/target/cfg_match/lib2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
its_a_macro! {
// Contents
}
Loading