Skip to content

Commit ec45b87

Browse files
petrochenkovpetrochenkov
committed
resolve: Block expansion of a derive container until all its derives are resolved
Also mark derive helpers as known as a part of the derive container's expansion instead of expansion of the derives themselves which may happen too late.
1 parent 9b91b9c commit ec45b87

15 files changed

+195
-153
lines changed

‎src/librustc_metadata/decoder.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -538,9 +538,7 @@ impl<'a, 'tcx> CrateMetadata {
538538
attributes.iter().cloned().map(Symbol::intern).collect::<Vec<_>>();
539539
(
540540
trait_name,
541-
SyntaxExtensionKind::Derive(Box::new(ProcMacroDerive {
542-
client, attrs: helper_attrs.clone()
543-
})),
541+
SyntaxExtensionKind::Derive(Box::new(ProcMacroDerive { client })),
544542
helper_attrs,
545543
)
546544
}

‎src/librustc_resolve/macros.rs

+20-21
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use rustc::{ty, lint, span_bug};
1313
use syntax::ast::{self, NodeId, Ident};
1414
use syntax::attr::StabilityLevel;
1515
use syntax::edition::Edition;
16-
use syntax::ext::base::{self, Indeterminate, SpecialDerives};
16+
use syntax::ext::base::{self, InvocationRes, Indeterminate, SpecialDerives};
1717
use syntax::ext::base::{MacroKind, SyntaxExtension};
1818
use syntax::ext::expand::{AstFragment, Invocation, InvocationKind};
1919
use syntax::ext::hygiene::{self, ExpnId, ExpnData, ExpnKind};
@@ -142,7 +142,7 @@ impl<'a> base::Resolver for Resolver<'a> {
142142

143143
fn resolve_macro_invocation(
144144
&mut self, invoc: &Invocation, eager_expansion_root: ExpnId, force: bool
145-
) -> Result<Option<Lrc<SyntaxExtension>>, Indeterminate> {
145+
) -> Result<InvocationRes, Indeterminate> {
146146
let invoc_id = invoc.expansion_data.id;
147147
let parent_scope = match self.invocation_parent_scopes.get(&invoc_id) {
148148
Some(parent_scope) => *parent_scope,
@@ -165,25 +165,24 @@ impl<'a> base::Resolver for Resolver<'a> {
165165
InvocationKind::Derive { ref path, .. } =>
166166
(path, MacroKind::Derive, &[][..], false),
167167
InvocationKind::DeriveContainer { ref derives, .. } => {
168-
// Block expansion of derives in the container until we know whether one of them
169-
// is a built-in `Copy`. Skip the resolution if there's only one derive - either
170-
// it's not a `Copy` and we don't need to do anything, or it's a `Copy` and it
171-
// will automatically knows about itself.
172-
let mut result = Ok(None);
173-
if derives.len() > 1 {
174-
for path in derives {
175-
match self.resolve_macro_path(path, Some(MacroKind::Derive),
176-
&parent_scope, true, force) {
177-
Ok((Some(ref ext), _)) if ext.is_derive_copy => {
178-
self.add_derives(invoc_id, SpecialDerives::COPY);
179-
return Ok(None);
180-
}
181-
Err(Determinacy::Undetermined) => result = Err(Indeterminate),
182-
_ => {}
183-
}
184-
}
168+
// Block expansion of the container until we resolve all derives in it.
169+
// This is required for two reasons:
170+
// - Derive helper attributes are in scope for the item to which the `#[derive]`
171+
// is applied, so they have to be produced by the container's expansion rather
172+
// than by individual derives.
173+
// - Derives in the container need to know whether one of them is a built-in `Copy`.
174+
// FIXME: Try to avoid repeated resolutions for derives here and in expansion.
175+
let mut exts = Vec::new();
176+
for path in derives {
177+
exts.push(match self.resolve_macro_path(
178+
path, Some(MacroKind::Derive), &parent_scope, true, force
179+
) {
180+
Ok((Some(ext), _)) => ext,
181+
Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive),
182+
Err(Determinacy::Undetermined) => return Err(Indeterminate),
183+
})
185184
}
186-
return result;
185+
return Ok(InvocationRes::DeriveContainer(exts));
187186
}
188187
};
189188

@@ -203,7 +202,7 @@ impl<'a> base::Resolver for Resolver<'a> {
203202
self.definitions.add_parent_module_of_macro_def(invoc_id, normal_module_def_id);
204203
}
205204

206-
Ok(Some(ext))
205+
Ok(InvocationRes::Single(ext))
207206
}
208207

209208
fn check_unused_macros(&self) {

‎src/libsyntax/ext/base.rs

+19-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use crate::ptr::P;
1111
use crate::symbol::{kw, sym, Ident, Symbol};
1212
use crate::{ThinVec, MACRO_ARGUMENTS};
1313
use crate::tokenstream::{self, TokenStream, TokenTree};
14+
use crate::visit::Visitor;
1415

1516
use errors::{DiagnosticBuilder, DiagnosticId};
1617
use smallvec::{smallvec, SmallVec};
@@ -72,6 +73,17 @@ impl Annotatable {
7273
}
7374
}
7475

76+
pub fn visit_with<'a, V: Visitor<'a>>(&'a self, visitor: &mut V) {
77+
match self {
78+
Annotatable::Item(item) => visitor.visit_item(item),
79+
Annotatable::TraitItem(trait_item) => visitor.visit_trait_item(trait_item),
80+
Annotatable::ImplItem(impl_item) => visitor.visit_impl_item(impl_item),
81+
Annotatable::ForeignItem(foreign_item) => visitor.visit_foreign_item(foreign_item),
82+
Annotatable::Stmt(stmt) => visitor.visit_stmt(stmt),
83+
Annotatable::Expr(expr) => visitor.visit_expr(expr),
84+
}
85+
}
86+
7587
pub fn expect_item(self) -> P<ast::Item> {
7688
match self {
7789
Annotatable::Item(i) => i,
@@ -637,6 +649,12 @@ impl SyntaxExtension {
637649

638650
pub type NamedSyntaxExtension = (Name, SyntaxExtension);
639651

652+
/// Result of resolving a macro invocation.
653+
pub enum InvocationRes {
654+
Single(Lrc<SyntaxExtension>),
655+
DeriveContainer(Vec<Lrc<SyntaxExtension>>),
656+
}
657+
640658
/// Error type that denotes indeterminacy.
641659
pub struct Indeterminate;
642660

@@ -664,7 +682,7 @@ pub trait Resolver {
664682

665683
fn resolve_macro_invocation(
666684
&mut self, invoc: &Invocation, eager_expansion_root: ExpnId, force: bool
667-
) -> Result<Option<Lrc<SyntaxExtension>>, Indeterminate>;
685+
) -> Result<InvocationRes, Indeterminate>;
668686

669687
fn check_unused_macros(&self);
670688

‎src/libsyntax/ext/expand.rs

+66-48
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use crate::attr::{self, HasAttrs};
44
use crate::source_map::respan;
55
use crate::config::StripUnconfigured;
66
use crate::ext::base::*;
7-
use crate::ext::proc_macro::collect_derives;
7+
use crate::ext::proc_macro::{collect_derives, MarkAttrs};
88
use crate::ext::hygiene::{ExpnId, SyntaxContext, ExpnData, ExpnKind};
99
use crate::ext::tt::macro_rules::annotate_err_with_kind;
1010
use crate::ext::placeholders::{placeholder, PlaceholderExpander};
@@ -307,10 +307,10 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
307307

308308
let eager_expansion_root =
309309
if self.monotonic { invoc.expansion_data.id } else { orig_expansion_data.id };
310-
let ext = match self.cx.resolver.resolve_macro_invocation(
310+
let res = match self.cx.resolver.resolve_macro_invocation(
311311
&invoc, eager_expansion_root, force
312312
) {
313-
Ok(ext) => ext,
313+
Ok(res) => res,
314314
Err(Indeterminate) => {
315315
undetermined_invocations.push(invoc);
316316
continue
@@ -322,54 +322,72 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
322322
self.cx.current_expansion = invoc.expansion_data.clone();
323323

324324
// FIXME(jseyfried): Refactor out the following logic
325-
let (expanded_fragment, new_invocations) = if let Some(ext) = ext {
326-
let fragment = self.expand_invoc(invoc, &ext.kind);
327-
self.collect_invocations(fragment, &[])
328-
} else if let InvocationKind::DeriveContainer { derives: traits, item } = invoc.kind {
329-
if !item.derive_allowed() {
330-
let attr = attr::find_by_name(item.attrs(), sym::derive)
331-
.expect("`derive` attribute should exist");
332-
let span = attr.span;
333-
let mut err = self.cx.mut_span_err(span,
334-
"`derive` may only be applied to \
335-
structs, enums and unions");
336-
if let ast::AttrStyle::Inner = attr.style {
337-
let trait_list = traits.iter()
338-
.map(|t| t.to_string()).collect::<Vec<_>>();
339-
let suggestion = format!("#[derive({})]", trait_list.join(", "));
340-
err.span_suggestion(
341-
span, "try an outer attribute", suggestion,
342-
// We don't 𝑘𝑛𝑜𝑤 that the following item is an ADT
343-
Applicability::MaybeIncorrect
344-
);
345-
}
346-
err.emit();
325+
let (expanded_fragment, new_invocations) = match res {
326+
InvocationRes::Single(ext) => {
327+
let fragment = self.expand_invoc(invoc, &ext.kind);
328+
self.collect_invocations(fragment, &[])
347329
}
330+
InvocationRes::DeriveContainer(exts) => {
331+
let (derives, item) = match invoc.kind {
332+
InvocationKind::DeriveContainer { derives, item } => (derives, item),
333+
_ => unreachable!(),
334+
};
335+
if !item.derive_allowed() {
336+
let attr = attr::find_by_name(item.attrs(), sym::derive)
337+
.expect("`derive` attribute should exist");
338+
let span = attr.span;
339+
let mut err = self.cx.mut_span_err(span,
340+
"`derive` may only be applied to structs, enums and unions");
341+
if let ast::AttrStyle::Inner = attr.style {
342+
let trait_list = derives.iter()
343+
.map(|t| t.to_string()).collect::<Vec<_>>();
344+
let suggestion = format!("#[derive({})]", trait_list.join(", "));
345+
err.span_suggestion(
346+
span, "try an outer attribute", suggestion,
347+
// We don't 𝑘𝑛𝑜𝑤 that the following item is an ADT
348+
Applicability::MaybeIncorrect
349+
);
350+
}
351+
err.emit();
352+
}
348353

349-
let mut item = self.fully_configure(item);
350-
item.visit_attrs(|attrs| attrs.retain(|a| a.path != sym::derive));
351-
let derive_placeholders =
352-
all_derive_placeholders.entry(invoc.expansion_data.id).or_default();
353-
354-
derive_placeholders.reserve(traits.len());
355-
invocations.reserve(traits.len());
356-
for path in traits {
357-
let expn_id = ExpnId::fresh(None);
358-
derive_placeholders.push(NodeId::placeholder_from_expn_id(expn_id));
359-
invocations.push(Invocation {
360-
kind: InvocationKind::Derive { path, item: item.clone() },
361-
fragment_kind: invoc.fragment_kind,
362-
expansion_data: ExpansionData {
363-
id: expn_id,
364-
..invoc.expansion_data.clone()
365-
},
366-
});
354+
let mut item = self.fully_configure(item);
355+
item.visit_attrs(|attrs| attrs.retain(|a| a.path != sym::derive));
356+
let mut helper_attrs = Vec::new();
357+
let mut has_copy = false;
358+
for ext in exts {
359+
helper_attrs.extend(&ext.helper_attrs);
360+
has_copy |= ext.is_derive_copy;
361+
}
362+
// Mark derive helpers inside this item as known and used.
363+
// FIXME: This is a hack, derive helpers should be integrated with regular name
364+
// resolution instead. For example, helpers introduced by a derive container
365+
// can be in scope for all code produced by that container's expansion.
366+
item.visit_with(&mut MarkAttrs(&helper_attrs));
367+
if has_copy {
368+
self.cx.resolver.add_derives(invoc.expansion_data.id, SpecialDerives::COPY);
369+
}
370+
371+
let derive_placeholders =
372+
all_derive_placeholders.entry(invoc.expansion_data.id).or_default();
373+
derive_placeholders.reserve(derives.len());
374+
invocations.reserve(derives.len());
375+
for path in derives {
376+
let expn_id = ExpnId::fresh(None);
377+
derive_placeholders.push(NodeId::placeholder_from_expn_id(expn_id));
378+
invocations.push(Invocation {
379+
kind: InvocationKind::Derive { path, item: item.clone() },
380+
fragment_kind: invoc.fragment_kind,
381+
expansion_data: ExpansionData {
382+
id: expn_id,
383+
..invoc.expansion_data.clone()
384+
},
385+
});
386+
}
387+
let fragment = invoc.fragment_kind
388+
.expect_from_annotatables(::std::iter::once(item));
389+
self.collect_invocations(fragment, derive_placeholders)
367390
}
368-
let fragment = invoc.fragment_kind
369-
.expect_from_annotatables(::std::iter::once(item));
370-
self.collect_invocations(fragment, derive_placeholders)
371-
} else {
372-
unreachable!()
373391
};
374392

375393
if expanded_fragments.len() < depth {

‎src/libsyntax/ext/proc_macro.rs

+1-5
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ pub struct ProcMacroDerive {
7878
pub client: proc_macro::bridge::client::Client<
7979
fn(proc_macro::TokenStream) -> proc_macro::TokenStream,
8080
>,
81-
pub attrs: Vec<ast::Name>,
8281
}
8382

8483
impl MultiItemModifier for ProcMacroDerive {
@@ -111,9 +110,6 @@ impl MultiItemModifier for ProcMacroDerive {
111110
}
112111
}
113112

114-
// Mark attributes as known, and used.
115-
MarkAttrs(&self.attrs).visit_item(&item);
116-
117113
let token = token::Interpolated(Lrc::new(token::NtItem(item)));
118114
let input = tokenstream::TokenTree::token(token, DUMMY_SP).into();
119115

@@ -164,7 +160,7 @@ impl MultiItemModifier for ProcMacroDerive {
164160
}
165161
}
166162

167-
struct MarkAttrs<'a>(&'a [ast::Name]);
163+
crate struct MarkAttrs<'a>(crate &'a [ast::Name]);
168164

169165
impl<'a> Visitor<'a> for MarkAttrs<'a> {
170166
fn visit_attribute(&mut self, attr: &Attribute) {

‎src/test/ui/derives/deriving-bounds.stderr

+12-12
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,3 @@
1-
error: cannot find derive macro `Send` in this scope
2-
--> $DIR/deriving-bounds.rs:1:10
3-
|
4-
LL | #[derive(Send)]
5-
| ^^^^
6-
|
7-
note: unsafe traits like `Send` should be implemented explicitly
8-
--> $DIR/deriving-bounds.rs:1:10
9-
|
10-
LL | #[derive(Send)]
11-
| ^^^^
12-
131
error: cannot find derive macro `Sync` in this scope
142
--> $DIR/deriving-bounds.rs:5:10
153
|
@@ -22,5 +10,17 @@ note: unsafe traits like `Sync` should be implemented explicitly
2210
LL | #[derive(Sync)]
2311
| ^^^^
2412

13+
error: cannot find derive macro `Send` in this scope
14+
--> $DIR/deriving-bounds.rs:1:10
15+
|
16+
LL | #[derive(Send)]
17+
| ^^^^
18+
|
19+
note: unsafe traits like `Send` should be implemented explicitly
20+
--> $DIR/deriving-bounds.rs:1:10
21+
|
22+
LL | #[derive(Send)]
23+
| ^^^^
24+
2525
error: aborting due to 2 previous errors
2626

‎src/test/ui/feature-gate/issue-43106-gating-of-derive-2.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: cannot find derive macro `x3300` in this scope
2-
--> $DIR/issue-43106-gating-of-derive-2.rs:4:14
2+
--> $DIR/issue-43106-gating-of-derive-2.rs:12:14
33
|
44
LL | #[derive(x3300)]
55
| ^^^^^
@@ -11,7 +11,7 @@ LL | #[derive(x3300)]
1111
| ^^^^^
1212

1313
error: cannot find derive macro `x3300` in this scope
14-
--> $DIR/issue-43106-gating-of-derive-2.rs:12:14
14+
--> $DIR/issue-43106-gating-of-derive-2.rs:4:14
1515
|
1616
LL | #[derive(x3300)]
1717
| ^^^^^

‎src/test/ui/feature-gate/issue-43106-gating-of-derive.rs

-3
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
// `#![derive]` raises errors when it occurs at contexts other than ADT
22
// definitions.
33

4-
#![derive(Debug)]
5-
//~^ ERROR `derive` may only be applied to structs, enums and unions
6-
74
#[derive(Debug)]
85
//~^ ERROR `derive` may only be applied to structs, enums and unions
96
mod derive {

‎src/test/ui/feature-gate/issue-43106-gating-of-derive.stderr

+5-11
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,32 @@
11
error: `derive` may only be applied to structs, enums and unions
22
--> $DIR/issue-43106-gating-of-derive.rs:4:1
33
|
4-
LL | #![derive(Debug)]
5-
| ^^^^^^^^^^^^^^^^^ help: try an outer attribute: `#[derive(Debug)]`
6-
7-
error: `derive` may only be applied to structs, enums and unions
8-
--> $DIR/issue-43106-gating-of-derive.rs:7:1
9-
|
104
LL | #[derive(Debug)]
115
| ^^^^^^^^^^^^^^^^
126

137
error: `derive` may only be applied to structs, enums and unions
14-
--> $DIR/issue-43106-gating-of-derive.rs:10:17
8+
--> $DIR/issue-43106-gating-of-derive.rs:7:17
159
|
1610
LL | mod inner { #![derive(Debug)] }
1711
| ^^^^^^^^^^^^^^^^^ help: try an outer attribute: `#[derive(Debug)]`
1812

1913
error: `derive` may only be applied to structs, enums and unions
20-
--> $DIR/issue-43106-gating-of-derive.rs:13:5
14+
--> $DIR/issue-43106-gating-of-derive.rs:10:5
2115
|
2216
LL | #[derive(Debug)]
2317
| ^^^^^^^^^^^^^^^^
2418

2519
error: `derive` may only be applied to structs, enums and unions
26-
--> $DIR/issue-43106-gating-of-derive.rs:26:5
20+
--> $DIR/issue-43106-gating-of-derive.rs:23:5
2721
|
2822
LL | #[derive(Debug)]
2923
| ^^^^^^^^^^^^^^^^
3024

3125
error: `derive` may only be applied to structs, enums and unions
32-
--> $DIR/issue-43106-gating-of-derive.rs:30:5
26+
--> $DIR/issue-43106-gating-of-derive.rs:27:5
3327
|
3428
LL | #[derive(Debug)]
3529
| ^^^^^^^^^^^^^^^^
3630

37-
error: aborting due to 6 previous errors
31+
error: aborting due to 5 previous errors
3832

0 commit comments

Comments
 (0)