Skip to content

fix: Fix some cfg censoring bugs #17006

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 3, 2024
Merged

fix: Fix some cfg censoring bugs #17006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 11 additions & 3 deletions crates/hir-def/src/macro_expansion_tests/builtin_derive_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,10 @@ struct Foo {
field1: i32,
#[cfg(never)]
field2: (),
#[cfg(feature = "never")]
field3: (),
#[cfg(not(feature = "never"))]
field4: (),
}
#[derive(Default)]
enum Bar {
Expand All @@ -618,12 +622,16 @@ enum Bar {
Bar,
}
"#,
expect![[r#"
expect![[r##"
#[derive(Default)]
struct Foo {
field1: i32,
#[cfg(never)]
field2: (),
#[cfg(feature = "never")]
field3: (),
#[cfg(not(feature = "never"))]
field4: (),
}
#[derive(Default)]
enum Bar {
Expand All @@ -635,14 +643,14 @@ enum Bar {
impl < > $crate::default::Default for Foo< > where {
fn default() -> Self {
Foo {
field1: $crate::default::Default::default(),
field1: $crate::default::Default::default(), field4: $crate::default::Default::default(),
}
}
}
impl < > $crate::default::Default for Bar< > where {
fn default() -> Self {
Bar::Bar
}
}"#]],
}"##]],
);
}
119 changes: 68 additions & 51 deletions crates/hir-expand/src/cfg_process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,59 @@
//! Processes out #[cfg] and #[cfg_attr] attributes from the input for the derive macro
use std::iter::Peekable;

use base_db::CrateId;
use cfg::{CfgAtom, CfgExpr};
use rustc_hash::FxHashSet;
use syntax::{
ast::{self, Attr, HasAttrs, Meta, VariantList},
AstNode, NodeOrToken, SyntaxElement, SyntaxNode, T,
AstNode, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, T,
};
use tracing::{debug, warn};
use tt::SmolStr;

use crate::{db::ExpandDatabase, proc_macro::ProcMacroKind, MacroCallLoc, MacroDefKind};

fn check_cfg_attr(attr: &Attr, loc: &MacroCallLoc, db: &dyn ExpandDatabase) -> Option<bool> {
fn check_cfg(db: &dyn ExpandDatabase, attr: &Attr, krate: CrateId) -> Option<bool> {
if !attr.simple_name().as_deref().map(|v| v == "cfg")? {
return None;
}
debug!("Evaluating cfg {}", attr);
let cfg = parse_from_attr_meta(attr.meta()?)?;
debug!("Checking cfg {:?}", cfg);
let enabled = db.crate_graph()[loc.krate].cfg_options.check(&cfg) != Some(false);
let enabled = db.crate_graph()[krate].cfg_options.check(&cfg) != Some(false);
Some(enabled)
}

fn check_cfg_attr_attr(attr: &Attr, loc: &MacroCallLoc, db: &dyn ExpandDatabase) -> Option<bool> {
fn check_cfg_attr(db: &dyn ExpandDatabase, attr: &Attr, krate: CrateId) -> Option<bool> {
if !attr.simple_name().as_deref().map(|v| v == "cfg_attr")? {
return None;
}
debug!("Evaluating cfg_attr {}", attr);
let cfg_expr = parse_from_attr_meta(attr.meta()?)?;
debug!("Checking cfg_attr {:?}", cfg_expr);
let enabled = db.crate_graph()[loc.krate].cfg_options.check(&cfg_expr) != Some(false);
let enabled = db.crate_graph()[krate].cfg_options.check(&cfg_expr) != Some(false);
Some(enabled)
}

fn process_has_attrs_with_possible_comma<I: HasAttrs>(
items: impl Iterator<Item = I>,
loc: &MacroCallLoc,
db: &dyn ExpandDatabase,
items: impl Iterator<Item = I>,
krate: CrateId,
remove: &mut FxHashSet<SyntaxElement>,
) -> Option<()> {
for item in items {
let field_attrs = item.attrs();
'attrs: for attr in field_attrs {
if check_cfg_attr(&attr, loc, db).map(|enabled| !enabled).unwrap_or_default() {
debug!("censoring type {:?}", item.syntax());
remove.insert(item.syntax().clone().into());
// We need to remove the , as well
remove_possible_comma(&item, remove);
break 'attrs;
if let Some(enabled) = check_cfg(db, &attr, krate) {
if enabled {
debug!("censoring {:?}", attr.syntax());
Copy link
Contributor

Choose a reason for hiding this comment

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

on an aside, i wonder if it's possible to write an SRR that rewrites debug!("censoring {:?}", attr.syntax()); to debug!(?attr.syntax(), "censoring").

remove.insert(attr.syntax().clone().into());
} else {
debug!("censoring {:?}", item.syntax());
remove.insert(item.syntax().clone().into());
// We need to remove the , as well
remove_possible_comma(&item, remove);
break 'attrs;
}
}

if let Some(enabled) = check_cfg_attr_attr(&attr, loc, db) {
if let Some(enabled) = check_cfg_attr(db, &attr, krate) {
if enabled {
debug!("Removing cfg_attr tokens {:?}", attr);
let meta = attr.meta()?;
Expand All @@ -60,13 +62,13 @@ fn process_has_attrs_with_possible_comma<I: HasAttrs>(
} else {
debug!("censoring type cfg_attr {:?}", item.syntax());
remove.insert(attr.syntax().clone().into());
continue;
}
}
}
}
Some(())
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
enum CfgExprStage {
/// Stripping the CFGExpr part of the attribute
Expand All @@ -78,6 +80,7 @@ enum CfgExprStage {
// Related Issue: https://github.com/rust-lang/rust-analyzer/issues/10110
EverythingElse,
}

/// This function creates its own set of tokens to remove. To help prevent malformed syntax as input.
fn remove_tokens_within_cfg_attr(meta: Meta) -> Option<FxHashSet<SyntaxElement>> {
let mut remove: FxHashSet<SyntaxElement> = FxHashSet::default();
Expand Down Expand Up @@ -131,23 +134,28 @@ fn remove_possible_comma(item: &impl AstNode, res: &mut FxHashSet<SyntaxElement>
}
}
fn process_enum(
variants: VariantList,
loc: &MacroCallLoc,
db: &dyn ExpandDatabase,
variants: VariantList,
krate: CrateId,
remove: &mut FxHashSet<SyntaxElement>,
) -> Option<()> {
'variant: for variant in variants.variants() {
for attr in variant.attrs() {
if check_cfg_attr(&attr, loc, db).map(|enabled| !enabled).unwrap_or_default() {
// Rustc does not strip the attribute if it is enabled. So we will leave it
debug!("censoring type {:?}", variant.syntax());
remove.insert(variant.syntax().clone().into());
// We need to remove the , as well
remove_possible_comma(&variant, remove);
continue 'variant;
};
if let Some(enabled) = check_cfg(db, &attr, krate) {
if enabled {
debug!("censoring {:?}", attr.syntax());
remove.insert(attr.syntax().clone().into());
} else {
// Rustc does not strip the attribute if it is enabled. So we will leave it
debug!("censoring type {:?}", variant.syntax());
remove.insert(variant.syntax().clone().into());
// We need to remove the , as well
remove_possible_comma(&variant, remove);
continue 'variant;
}
}

if let Some(enabled) = check_cfg_attr_attr(&attr, loc, db) {
if let Some(enabled) = check_cfg_attr(db, &attr, krate) {
if enabled {
debug!("Removing cfg_attr tokens {:?}", attr);
let meta = attr.meta()?;
Expand All @@ -156,17 +164,16 @@ fn process_enum(
} else {
debug!("censoring type cfg_attr {:?}", variant.syntax());
remove.insert(attr.syntax().clone().into());
continue;
}
}
}
if let Some(fields) = variant.field_list() {
match fields {
ast::FieldList::RecordFieldList(fields) => {
process_has_attrs_with_possible_comma(fields.fields(), loc, db, remove)?;
process_has_attrs_with_possible_comma(db, fields.fields(), krate, remove)?;
}
ast::FieldList::TupleFieldList(fields) => {
process_has_attrs_with_possible_comma(fields.fields(), loc, db, remove)?;
process_has_attrs_with_possible_comma(db, fields.fields(), krate, remove)?;
}
}
}
Expand All @@ -175,9 +182,9 @@ fn process_enum(
}

pub(crate) fn process_cfg_attrs(
db: &dyn ExpandDatabase,
node: &SyntaxNode,
loc: &MacroCallLoc,
db: &dyn ExpandDatabase,
) -> Option<FxHashSet<SyntaxElement>> {
// FIXME: #[cfg_eval] is not implemented. But it is not stable yet
let is_derive = match loc.def.kind {
Expand All @@ -193,36 +200,35 @@ pub(crate) fn process_cfg_attrs(

let item = ast::Item::cast(node.clone())?;
for attr in item.attrs() {
if let Some(enabled) = check_cfg_attr_attr(&attr, loc, db) {
if let Some(enabled) = check_cfg_attr(db, &attr, loc.krate) {
if enabled {
debug!("Removing cfg_attr tokens {:?}", attr);
let meta = attr.meta()?;
let removes_from_cfg_attr = remove_tokens_within_cfg_attr(meta)?;
remove.extend(removes_from_cfg_attr);
} else {
debug!("censoring type cfg_attr {:?}", item.syntax());
debug!("Removing type cfg_attr {:?}", item.syntax());
remove.insert(attr.syntax().clone().into());
continue;
}
}
}
match item {
ast::Item::Struct(it) => match it.field_list()? {
ast::FieldList::RecordFieldList(fields) => {
process_has_attrs_with_possible_comma(fields.fields(), loc, db, &mut remove)?;
process_has_attrs_with_possible_comma(db, fields.fields(), loc.krate, &mut remove)?;
}
ast::FieldList::TupleFieldList(fields) => {
process_has_attrs_with_possible_comma(fields.fields(), loc, db, &mut remove)?;
process_has_attrs_with_possible_comma(db, fields.fields(), loc.krate, &mut remove)?;
}
},
ast::Item::Enum(it) => {
process_enum(it.variant_list()?, loc, db, &mut remove)?;
process_enum(db, it.variant_list()?, loc.krate, &mut remove)?;
}
ast::Item::Union(it) => {
process_has_attrs_with_possible_comma(
it.record_field_list()?.fields(),
loc,
db,
it.record_field_list()?.fields(),
loc.krate,
&mut remove,
)?;
}
Expand All @@ -234,10 +240,22 @@ pub(crate) fn process_cfg_attrs(
/// Parses a `cfg` attribute from the meta
fn parse_from_attr_meta(meta: Meta) -> Option<CfgExpr> {
let tt = meta.token_tree()?;
let mut iter = tt.token_trees_and_tokens().skip(1).peekable();
let mut iter = tt
.token_trees_and_tokens()
.filter(is_not_whitespace)
.skip(1)
.take_while(is_not_closing_paren)
.peekable();
next_cfg_expr_from_syntax(&mut iter)
}

fn is_not_closing_paren(element: &NodeOrToken<ast::TokenTree, syntax::SyntaxToken>) -> bool {
!matches!(element, NodeOrToken::Token(token) if (token.kind() == syntax::T![')']))
}
fn is_not_whitespace(element: &NodeOrToken<ast::TokenTree, syntax::SyntaxToken>) -> bool {
!matches!(element, NodeOrToken::Token(token) if (token.kind() == SyntaxKind::WHITESPACE))
}

fn next_cfg_expr_from_syntax<I>(iter: &mut Peekable<I>) -> Option<CfgExpr>
where
I: Iterator<Item = NodeOrToken<ast::TokenTree, syntax::SyntaxToken>>,
Expand All @@ -256,14 +274,13 @@ where
let Some(NodeOrToken::Node(tree)) = iter.next() else {
return Some(CfgExpr::Invalid);
};
let mut tree_iter = tree.token_trees_and_tokens().skip(1).peekable();
while tree_iter
.peek()
.filter(
|element| matches!(element, NodeOrToken::Token(token) if (token.kind() != syntax::T![')'])),
)
.is_some()
{
let mut tree_iter = tree
.token_trees_and_tokens()
.filter(is_not_whitespace)
.skip(1)
.take_while(is_not_closing_paren)
.peekable();
while tree_iter.peek().is_some() {
let pred = next_cfg_expr_from_syntax(&mut tree_iter);
if let Some(pred) = pred {
preds.push(pred);
Expand Down
4 changes: 2 additions & 2 deletions crates/hir-expand/src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ pub fn expand_speculative(
};

let censor_cfg =
cfg_process::process_cfg_attrs(speculative_args, &loc, db).unwrap_or_default();
cfg_process::process_cfg_attrs(db, speculative_args, &loc).unwrap_or_default();
let mut fixups = fixup::fixup_syntax(span_map, speculative_args, span);
fixups.append.retain(|it, _| match it {
syntax::NodeOrToken::Token(_) => true,
Expand Down Expand Up @@ -462,7 +462,7 @@ fn macro_arg(db: &dyn ExpandDatabase, id: MacroCallId) -> MacroArgResult {

let (mut tt, undo_info) = {
let syntax = item_node.syntax();
let censor_cfg = cfg_process::process_cfg_attrs(syntax, &loc, db).unwrap_or_default();
let censor_cfg = cfg_process::process_cfg_attrs(db, syntax, &loc).unwrap_or_default();
let mut fixups = fixup::fixup_syntax(map.as_ref(), syntax, span);
fixups.append.retain(|it, _| match it {
syntax::NodeOrToken::Token(_) => true,
Expand Down
5 changes: 4 additions & 1 deletion crates/ide-completion/src/completions/postfix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use ide_db::{
ty_filter::TryEnum,
SnippetCap,
};
use stdx::never;
use syntax::{
ast::{self, make, AstNode, AstToken},
SyntaxKind::{BLOCK_EXPR, EXPR_STMT, FOR_EXPR, IF_EXPR, LOOP_EXPR, STMT_LIST, WHILE_EXPR},
Expand Down Expand Up @@ -319,7 +320,9 @@ fn build_postfix_snippet_builder<'ctx>(
) -> Option<impl Fn(&str, &str, &str) -> Builder + 'ctx> {
let receiver_range = ctx.sema.original_range_opt(receiver.syntax())?.range;
if ctx.source_range().end() < receiver_range.start() {
// This shouldn't happen, yet it does. I assume this might be due to an incorrect token mapping.
// This shouldn't happen, yet it does. I assume this might be due to an incorrect token
// mapping.
never!();
return None;
}
let delete_range = TextRange::new(receiver_range.start(), ctx.source_range().end());
Expand Down