Skip to content

Commit 29960af

Browse files
VeykrilVeykril
authored
Merge pull request #19713 from Veykril/push-xkppskpmuzpo
refactor: Cleanup cfg check handling in expression store lowering
2 parents 9f5b884 + d11dbf6 commit 29960af

File tree

17 files changed

+234
-188
lines changed

17 files changed

+234
-188
lines changed

‎crates/hir-def/src/attr.rs

+39-16
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
//! A higher level attributes based on TokenTree, with also some shortcuts.
22
3-
use std::{borrow::Cow, hash::Hash, ops};
3+
use std::{borrow::Cow, convert::identity, hash::Hash, ops};
44

55
use base_db::Crate;
66
use cfg::{CfgExpr, CfgOptions};
77
use either::Either;
88
use hir_expand::{
99
HirFileId, InFile,
1010
attrs::{Attr, AttrId, RawAttrs, collect_attrs},
11+
span_map::SpanMapRef,
1112
};
1213
use intern::{Symbol, sym};
1314
use la_arena::{ArenaMap, Idx, RawIdx};
@@ -45,8 +46,27 @@ impl Attrs {
4546
(**self).iter().find(|attr| attr.id == id)
4647
}
4748

48-
pub(crate) fn filter(db: &dyn DefDatabase, krate: Crate, raw_attrs: RawAttrs) -> Attrs {
49-
Attrs(raw_attrs.filter(db, krate))
49+
pub(crate) fn expand_cfg_attr(
50+
db: &dyn DefDatabase,
51+
krate: Crate,
52+
raw_attrs: RawAttrs,
53+
) -> Attrs {
54+
Attrs(raw_attrs.expand_cfg_attr(db, krate))
55+
}
56+
57+
pub(crate) fn is_cfg_enabled_for(
58+
db: &dyn DefDatabase,
59+
owner: &dyn ast::HasAttrs,
60+
span_map: SpanMapRef<'_>,
61+
cfg_options: &CfgOptions,
62+
) -> Result<(), CfgExpr> {
63+
RawAttrs::attrs_iter_expanded::<false>(db, owner, span_map, cfg_options)
64+
.filter_map(|attr| attr.cfg())
65+
.find_map(|cfg| match cfg_options.check(&cfg).is_none_or(identity) {
66+
true => None,
67+
false => Some(cfg),
68+
})
69+
.map_or(Ok(()), Err)
5070
}
5171
}
5272

@@ -522,46 +542,49 @@ impl AttrsWithOwner {
522542
GenericParamId::ConstParamId(it) => {
523543
let src = it.parent().child_source(db);
524544
// FIXME: We should be never getting `None` here.
525-
match src.value.get(it.local_id()) {
526-
Some(val) => RawAttrs::from_attrs_owner(
545+
return Attrs(match src.value.get(it.local_id()) {
546+
Some(val) => RawAttrs::new_expanded(
527547
db,
528-
src.with_value(val),
548+
val,
529549
db.span_map(src.file_id).as_ref(),
550+
def.krate(db).cfg_options(db),
530551
),
531552
None => RawAttrs::EMPTY,
532-
}
553+
});
533554
}
534555
GenericParamId::TypeParamId(it) => {
535556
let src = it.parent().child_source(db);
536557
// FIXME: We should be never getting `None` here.
537-
match src.value.get(it.local_id()) {
538-
Some(val) => RawAttrs::from_attrs_owner(
558+
return Attrs(match src.value.get(it.local_id()) {
559+
Some(val) => RawAttrs::new_expanded(
539560
db,
540-
src.with_value(val),
561+
val,
541562
db.span_map(src.file_id).as_ref(),
563+
def.krate(db).cfg_options(db),
542564
),
543565
None => RawAttrs::EMPTY,
544-
}
566+
});
545567
}
546568
GenericParamId::LifetimeParamId(it) => {
547569
let src = it.parent.child_source(db);
548570
// FIXME: We should be never getting `None` here.
549-
match src.value.get(it.local_id) {
550-
Some(val) => RawAttrs::from_attrs_owner(
571+
return Attrs(match src.value.get(it.local_id) {
572+
Some(val) => RawAttrs::new_expanded(
551573
db,
552-
src.with_value(val),
574+
val,
553575
db.span_map(src.file_id).as_ref(),
576+
def.krate(db).cfg_options(db),
554577
),
555578
None => RawAttrs::EMPTY,
556-
}
579+
});
557580
}
558581
},
559582
AttrDefId::ExternBlockId(it) => attrs_from_item_tree_loc(db, it),
560583
AttrDefId::ExternCrateId(it) => attrs_from_item_tree_loc(db, it),
561584
AttrDefId::UseId(it) => attrs_from_item_tree_loc(db, it),
562585
};
563586

564-
let attrs = raw_attrs.filter(db, def.krate(db));
587+
let attrs = raw_attrs.expand_cfg_attr(db, def.krate(db));
565588
Attrs(attrs)
566589
}
567590

‎crates/hir-def/src/expr_store/expander.rs

+5-13
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
use std::mem;
44

55
use base_db::Crate;
6+
use cfg::CfgOptions;
67
use drop_bomb::DropBomb;
78
use hir_expand::{
89
ExpandError, ExpandErrorKind, ExpandResult, HirFileId, InFile, Lookup, MacroCallId,
9-
attrs::RawAttrs, eager::EagerCallBackFn, mod_path::ModPath, span_map::SpanMap,
10+
eager::EagerCallBackFn, mod_path::ModPath, span_map::SpanMap,
1011
};
1112
use span::{AstIdMap, Edition, SyntaxContext};
1213
use syntax::ast::HasAttrs;
@@ -64,22 +65,13 @@ impl Expander {
6465
}
6566
}
6667

67-
pub(super) fn attrs(
68-
&self,
69-
db: &dyn DefDatabase,
70-
krate: Crate,
71-
has_attrs: &dyn HasAttrs,
72-
) -> Attrs {
73-
Attrs::filter(db, krate, RawAttrs::new(db, has_attrs, self.span_map.as_ref()))
74-
}
75-
7668
pub(super) fn is_cfg_enabled(
7769
&self,
7870
db: &dyn DefDatabase,
79-
krate: Crate,
8071
has_attrs: &dyn HasAttrs,
81-
) -> bool {
82-
self.attrs(db, krate, has_attrs).is_cfg_enabled(krate.cfg_options(db))
72+
cfg_options: &CfgOptions,
73+
) -> Result<(), cfg::CfgExpr> {
74+
Attrs::is_cfg_enabled_for(db, has_attrs, self.span_map.as_ref(), cfg_options)
8375
}
8476

8577
pub(super) fn call_syntax_ctx(&self) -> SyntaxContext {

‎crates/hir-def/src/expr_store/lower.rs

+48-45
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ mod path;
77

88
use std::mem;
99

10+
use cfg::CfgOptions;
1011
use either::Either;
1112
use hir_expand::{
1213
HirFileId, InFile, Lookup, MacroDefId,
@@ -81,8 +82,6 @@ pub(super) fn lower_body(
8182
// even though they should be the same. Also, when the body comes from multiple expansions, their
8283
// hygiene is different.
8384

84-
let krate = module.krate();
85-
8685
let mut self_param = None;
8786
let mut source_map_self_param = None;
8887
let mut params = vec![];
@@ -100,9 +99,8 @@ pub(super) fn lower_body(
10099
// and skip the body.
101100
if skip_body {
102101
if let Some(param_list) = parameters {
103-
if let Some(self_param_syn) = param_list
104-
.self_param()
105-
.filter(|self_param| collector.expander.is_cfg_enabled(db, krate, self_param))
102+
if let Some(self_param_syn) =
103+
param_list.self_param().filter(|self_param| collector.check_cfg(self_param))
106104
{
107105
let is_mutable =
108106
self_param_syn.mut_token().is_some() && self_param_syn.amp_token().is_none();
@@ -119,10 +117,7 @@ pub(super) fn lower_body(
119117
source_map_self_param =
120118
Some(collector.expander.in_file(AstPtr::new(&self_param_syn)));
121119
}
122-
let count = param_list
123-
.params()
124-
.filter(|it| collector.expander.is_cfg_enabled(db, krate, it))
125-
.count();
120+
let count = param_list.params().filter(|it| collector.check_cfg(it)).count();
126121
params = (0..count).map(|_| collector.missing_pat()).collect();
127122
};
128123
let body_expr = collector.missing_expr();
@@ -138,9 +133,7 @@ pub(super) fn lower_body(
138133
}
139134

140135
if let Some(param_list) = parameters {
141-
if let Some(self_param_syn) =
142-
param_list.self_param().filter(|it| collector.expander.is_cfg_enabled(db, krate, it))
143-
{
136+
if let Some(self_param_syn) = param_list.self_param().filter(|it| collector.check_cfg(it)) {
144137
let is_mutable =
145138
self_param_syn.mut_token().is_some() && self_param_syn.amp_token().is_none();
146139
let hygiene = self_param_syn
@@ -157,7 +150,7 @@ pub(super) fn lower_body(
157150
}
158151

159152
for param in param_list.params() {
160-
if collector.expander.is_cfg_enabled(db, krate, &param) {
153+
if collector.check_cfg(&param) {
161154
let param_pat = collector.collect_pat_top(param.pat());
162155
params.push(param_pat);
163156
}
@@ -346,7 +339,7 @@ pub(crate) fn lower_function(
346339
collector.collect_impl_trait(&mut expr_collector, |collector, mut impl_trait_lower_fn| {
347340
if let Some(param_list) = fn_.value.param_list() {
348341
if let Some(param) = param_list.self_param() {
349-
let enabled = collector.expander.is_cfg_enabled(db, module.krate(), &param);
342+
let enabled = collector.check_cfg(&param);
350343
if enabled {
351344
has_self_param = true;
352345
params.push(match param.ty() {
@@ -381,7 +374,7 @@ pub(crate) fn lower_function(
381374
}
382375
let p = param_list
383376
.params()
384-
.filter(|param| collector.expander.is_cfg_enabled(db, module.krate(), param))
377+
.filter(|param| collector.check_cfg(param))
385378
.filter(|param| {
386379
let is_variadic = param.dotdotdot_token().is_some();
387380
has_variadic |= is_variadic;
@@ -441,6 +434,7 @@ pub(crate) fn lower_function(
441434

442435
pub struct ExprCollector<'db> {
443436
db: &'db dyn DefDatabase,
437+
cfg_options: &'db CfgOptions,
444438
expander: Expander,
445439
def_map: Arc<DefMap>,
446440
local_def_map: Arc<LocalDefMap>,
@@ -553,6 +547,7 @@ impl ExprCollector<'_> {
553547
let expander = Expander::new(db, current_file_id, &def_map);
554548
ExprCollector {
555549
db,
550+
cfg_options: module.krate().cfg_options(db),
556551
module,
557552
def_map,
558553
local_def_map,
@@ -1026,7 +1021,9 @@ impl ExprCollector<'_> {
10261021
/// Returns `None` if and only if the expression is `#[cfg]`d out.
10271022
fn maybe_collect_expr(&mut self, expr: ast::Expr) -> Option<ExprId> {
10281023
let syntax_ptr = AstPtr::new(&expr);
1029-
self.check_cfg(&expr)?;
1024+
if !self.check_cfg(&expr) {
1025+
return None;
1026+
}
10301027

10311028
// FIXME: Move some of these arms out into separate methods for clarity
10321029
Some(match expr {
@@ -1114,6 +1111,7 @@ impl ExprCollector<'_> {
11141111
ast::Expr::WhileExpr(e) => self.collect_while_loop(syntax_ptr, e),
11151112
ast::Expr::ForExpr(e) => self.collect_for_loop(syntax_ptr, e),
11161113
ast::Expr::CallExpr(e) => {
1114+
// FIXME: Remove this once we drop support for <1.86, https://github.com/rust-lang/rust/commit/ac9cb908ac4301dfc25e7a2edee574320022ae2c
11171115
let is_rustc_box = {
11181116
let attrs = e.attrs();
11191117
attrs.filter_map(|it| it.as_simple_atom()).any(|it| it == "rustc_box")
@@ -1156,13 +1154,17 @@ impl ExprCollector<'_> {
11561154
match_arm_list
11571155
.arms()
11581156
.filter_map(|arm| {
1159-
self.check_cfg(&arm).map(|()| MatchArm {
1160-
pat: self.collect_pat_top(arm.pat()),
1161-
expr: self.collect_expr_opt(arm.expr()),
1162-
guard: arm
1163-
.guard()
1164-
.map(|guard| self.collect_expr_opt(guard.condition())),
1165-
})
1157+
if self.check_cfg(&arm) {
1158+
Some(MatchArm {
1159+
pat: self.collect_pat_top(arm.pat()),
1160+
expr: self.collect_expr_opt(arm.expr()),
1161+
guard: arm
1162+
.guard()
1163+
.map(|guard| self.collect_expr_opt(guard.condition())),
1164+
})
1165+
} else {
1166+
None
1167+
}
11661168
})
11671169
.collect()
11681170
} else {
@@ -1230,7 +1232,9 @@ impl ExprCollector<'_> {
12301232
let fields = nfl
12311233
.fields()
12321234
.filter_map(|field| {
1233-
self.check_cfg(&field)?;
1235+
if !self.check_cfg(&field) {
1236+
return None;
1237+
}
12341238

12351239
let name = field.field_name()?.as_name();
12361240

@@ -1483,7 +1487,9 @@ impl ExprCollector<'_> {
14831487
}
14841488

14851489
fn maybe_collect_expr_as_pat(&mut self, expr: &ast::Expr) -> Option<PatId> {
1486-
self.check_cfg(expr)?;
1490+
if !self.check_cfg(expr) {
1491+
return None;
1492+
}
14871493
let syntax_ptr = AstPtr::new(expr);
14881494

14891495
let result = match expr {
@@ -1558,7 +1564,9 @@ impl ExprCollector<'_> {
15581564
let args = record_field_list
15591565
.fields()
15601566
.filter_map(|f| {
1561-
self.check_cfg(&f)?;
1567+
if !self.check_cfg(&f) {
1568+
return None;
1569+
}
15621570
let field_expr = f.expr()?;
15631571
let pat = self.collect_expr_as_pat(field_expr);
15641572
let name = f.field_name()?.as_name();
@@ -2044,7 +2052,7 @@ impl ExprCollector<'_> {
20442052
fn collect_stmt(&mut self, statements: &mut Vec<Statement>, s: ast::Stmt) {
20452053
match s {
20462054
ast::Stmt::LetStmt(stmt) => {
2047-
if self.check_cfg(&stmt).is_none() {
2055+
if !self.check_cfg(&stmt) {
20482056
return;
20492057
}
20502058
let pat = self.collect_pat_top(stmt.pat());
@@ -2059,7 +2067,7 @@ impl ExprCollector<'_> {
20592067
ast::Stmt::ExprStmt(stmt) => {
20602068
let expr = stmt.expr();
20612069
match &expr {
2062-
Some(expr) if self.check_cfg(expr).is_none() => return,
2070+
Some(expr) if !self.check_cfg(expr) => return,
20632071
_ => (),
20642072
}
20652073
let has_semi = stmt.semicolon_token().is_some();
@@ -2074,7 +2082,7 @@ impl ExprCollector<'_> {
20742082
}
20752083
}
20762084
ast::Stmt::Item(ast::Item::MacroDef(macro_)) => {
2077-
if self.check_cfg(&macro_).is_none() {
2085+
if !self.check_cfg(&macro_) {
20782086
return;
20792087
}
20802088
let Some(name) = macro_.name() else {
@@ -2086,7 +2094,7 @@ impl ExprCollector<'_> {
20862094
self.collect_macro_def(statements, macro_id);
20872095
}
20882096
ast::Stmt::Item(ast::Item::MacroRules(macro_)) => {
2089-
if self.check_cfg(&macro_).is_none() {
2097+
if !self.check_cfg(&macro_) {
20902098
return;
20912099
}
20922100
let Some(name) = macro_.name() else {
@@ -2360,7 +2368,9 @@ impl ExprCollector<'_> {
23602368
let args = record_pat_field_list
23612369
.fields()
23622370
.filter_map(|f| {
2363-
self.check_cfg(&f)?;
2371+
if !self.check_cfg(&f) {
2372+
return None;
2373+
}
23642374
let ast_pat = f.pat()?;
23652375
let pat = self.collect_pat(ast_pat, binding_list);
23662376
let name = f.field_name()?.as_name();
@@ -2536,25 +2546,18 @@ impl ExprCollector<'_> {
25362546

25372547
/// Returns `None` (and emits diagnostics) when `owner` if `#[cfg]`d out, and `Some(())` when
25382548
/// not.
2539-
fn check_cfg(&mut self, owner: &dyn ast::HasAttrs) -> Option<()> {
2540-
let attrs = self.expander.attrs(self.db, self.module.krate(), owner);
2541-
match attrs.cfg() {
2542-
Some(cfg) => {
2543-
let cfg_options = self.module.krate().cfg_options(self.db);
2544-
2545-
if cfg_options.check(&cfg) != Some(false) {
2546-
return Some(());
2547-
}
2548-
2549+
fn check_cfg(&mut self, owner: &dyn ast::HasAttrs) -> bool {
2550+
let enabled = self.expander.is_cfg_enabled(self.db, owner, self.cfg_options);
2551+
match enabled {
2552+
Ok(()) => true,
2553+
Err(cfg) => {
25492554
self.source_map.diagnostics.push(ExpressionStoreDiagnostics::InactiveCode {
25502555
node: self.expander.in_file(SyntaxNodePtr::new(owner.syntax())),
25512556
cfg,
2552-
opts: cfg_options.clone(),
2557+
opts: self.cfg_options.clone(),
25532558
});
2554-
2555-
None
2559+
false
25562560
}
2557-
None => Some(()),
25582561
}
25592562
}
25602563

0 commit comments

Comments
 (0)