[WIP] Expand `#[derive(..)]`s in the InvocationCollector by keeperofdakeys · Pull Request #39391 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits18 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Expand derive attributes in the InvocationCollector

r? @jseyfried

This allows builtin derives to be registered and resolved, just like other derive types.

@jseyfried This code could is a bit rough, and it currently has a bug that means it only works with legacy custom derives, and builtin derives. I'm hoping that a second set of eyes will help me spot the error I'm making.

Cause of bug:

22:21 < jseyfried> The problem is that no proc macro derives have been imported yet when we are doing invocation collection

pub fn is_derive_attr(attr: &ast::Attribute) -> bool {
let res = attr.name() == Symbol::intern("derive");
res
}

Choose a reason for hiding this comment

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

nit: This can just be attr.name() == "derive" (I would inline and remove the function)

}

pub fn get_derive_attr(cx: &mut ExtCtxt, attrs: &mut Vecast::Attribute,
is_derive_type: fn(&mut ExtCtxt, &NestedMetaItem) -> bool)

Choose a reason for hiding this comment

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

I think it would be a little cleaner if this were an enum (three variants) and is_derive_type(...) were a match.

Annotatable::TraitItem(P(fully_configure!(self, item, noop_fold_trait_item)));
return self.collect_attr(attr, item, ExpansionKind::TraitItems).make_trait_items()
} else {
self.cx.span_err(attr.span, "`derive` can't be only be applied to items");

Choose a reason for hiding this comment

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

Could we move this logic into collect_attr?
e.g.

if attr.name() == "derive" && (kind == ExpansionKind::TraitItems || ExpansionKind::ImplItems) { ... }

Choose a reason for hiding this comment

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

What would you return fromcollect_attr for the error case? The callers assume that collect_attr will always return a valid Expansion.

Choose a reason for hiding this comment

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

It could just return the original item (via kind.expect_from_annotatables(iter::once(item))).

@@ -54,7 +54,7 @@ impl MultiItemModifier for CustomDerive {
Annotatable::Item(item) => item,
Annotatable::ImplItem(_) \|
Annotatable::TraitItem(_) => {
ecx.span_err(span, "custom derive attributes may only be \
ecx.span_err(span, "proc_macro_derive attributes may only be \

Choose a reason for hiding this comment

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

"proc_macro_derive attribute" makes me think of #[proc_macro_derive] itself rather than #[derive(Trait)].
Maybe "proc-macro derives may only be ..."?

}

pub fn verify_derive_attrs(cx: &mut ExtCtxt, attrs: &mut Vecast::Attribute) {
for i in 0..attrs.len() {

Choose a reason for hiding this comment

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

nit: for attr in attrs { ... } is more idiomatic

get_proc_macro_derive(self.cx, &mut attrs)
}).or_else(\|
add_structural_marker(self.cx, &mut attrs);
add_copy_clone_marker(self.cx, &mut attrs);

Choose a reason for hiding this comment

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

I think we could merge these two function into a single function add_derived_markers that computes a Vec<Name> (or Vec<MetaItem>) of all the derived trait names and then just uses e.g. vec.contains(&Symbol::intern("Copy")) || vec.contains(&Symbol::intern("Clone"))

Choose a reason for hiding this comment

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

One annoying thing is that currently we need some span to add for the new attribute. If we make a Vec<Name>, we'll either need to grab the span of the first attribute, or if possible make a dummy span.

Choose a reason for hiding this comment

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

I think either would be fine (provided the span is allow_internal_unstable).

We're kind of breaking the semantics of the InvocationCollector with this change, which causes serious breakage. We're trying to resolve derives during collection, when the InvocationCollector usually resolves separately itself. With Foo as a proc_macro_derive, this works: #[derive(Copy, Foo), this doesn't: #[derive(Foo)] (it's never expanded at all) - as you suggested, this is probably because the first gives #[macro_use] time to be expanded.

The fix for this seems to be moving the resolution of types of derives into the invocation expansion loop. However I'm not sure how to do the ordering of derives if this is done. Maybe it's enough to resolve custom_derives in collect_attr (which become InvocKind::Attr anyway), and leave proc_macro_derive vs builtin to the resolution in the invocation loop. Though I'm not sure how detection of undefined derives works here.

This means that proc_macro_derives that haven't been imported yet don't get ignored. This is safe because legacy_derives and builtin derives are instantly resolvable.

return None;
}

let traits = attr.meta_item_list().unwrap();

Choose a reason for hiding this comment

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

This looks like it might panic on #[derive].

return traits.get(0);
}

pub fn verify_derive_attrs(cx: &mut ExtCtxt, attrs: &mut Vecast::Attribute) {

Choose a reason for hiding this comment

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

nit: I think attrs: &[ast::Attribute] is sufficient here

}

impl DeriveType {
pub fn is_derive_type(&self, cx: &mut ExtCtxt, titem: &NestedMetaItem) -> bool {

Choose a reason for hiding this comment

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

Could we instead have a method DeriveType::classify(name, cx) here? The function would check legacy first (returning DeriveType::Legacy if we find derive_* in builtin_macros), then check builtins (returning DeriveType::Builtin), then finally return DeriveType::ProcMacro.

e.g. is_legacy_derive(cx, titem) would become DeriveType::classify(titem.name(), cx) == DeriveType::Legacy.

Even better, we could move this logic into cx.resolver.find_attr_invoc() or a new method of ext::base::Resolver.


if !attrs.iter().any(\|a
titems.iter().any(\|t
let structural_match = Symbol::intern("structural_match");

Choose a reason for hiding this comment

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

nit: indent the if body one more space


if !attrs.iter().any(\|a
titems.iter().any(\|t
let structural_match = Symbol::intern("structural_match");

Choose a reason for hiding this comment

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

(same here)

let tname = titem.name().unwrap();
let name = Symbol::intern(&format!("derive_{}", tname));
let path = ast::Path::from_ident(titem.span, ast::Ident::with_empty_ctxt(name));
cx.resolver.resolve_macro(cx.current_expansion.mark, &path, false).is_ok()

Choose a reason for hiding this comment

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

Ideally we would just resolve in resolver.builtin_macros, either by moving this logic into resolve or adding a method to check builtin macros directly to ext::base::Resolver.

pub fn is_builtin_derive(cx: &mut ExtCtxt, titem: &NestedMetaItem) -> bool {
let tname = titem.name().unwrap();
let derive_mode = ast::Path::from_ident(titem.span, ast::Ident::with_empty_ctxt(tname));
cx.resolver.resolve_macro(cx.current_expansion.mark, &derive_mode, false).map(\|ext

Choose a reason for hiding this comment

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

(same here, re resolver.builtin_macros)

@@ -856,7 +957,8 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

let (item, attr) = self.classify_item(item);
if let Some(attr) = attr {
let item = Annotatable::ImplItem(P(fully_configure!(self, item, noop_fold_impl_item)));
let item =
Annotatable::ImplItem(P(fully_configure!(self, item, noop_fold_impl_item)));

Choose a reason for hiding this comment

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

nit: stray change

I implemented the change that you suggested, and all the serious test failures are now gone. It's down to trivial test failures, namely:

Using derive as a macro like this derive!() now gives the error macro undefined: 'derive!', when before it gave 'derive' can only be used in attributes. This is a natural consequence of removing derive as a resolvable macro. Should there be a dummy macro now, since you can't use derive as an attribute macro anyway?

An invalid derive attribute (like #[derive(Zero)]) now gives the error macro undefined: 'Zero!', where as before it gave '#[derive]' for custom traits is not stable enough for use. It is deprecated and will be removed in v1.15 (see issue #29644). Obviously an improvement, but could this be improved even more?

@keeperofdakeys

Should there be a dummy macro now, since you can't use derive as an attribute macro anyway?

I don't think it matters that much -- macro undefined: 'derive!' is fine.

but could [invalid derive attribute diagnostics] be improved even more?

Yeah, by changing from macro undefined: 'Zero!' to macro undefined: 'Zero' (removing !). Even better -- something like undefined custom derive `Zero` .

I've pushed some fixes for the latest test failures, there will probably be some more though.

What is the new error message for the situation described in #39326 ?

I was thinking something like `Derive macro 'Foo' does not exist`. It isn't in the PR yet as things are still being refactored. In a future PR, I was thinking of doing suggestions for (at least) builtin derives.

Choose a reason for hiding this comment

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

r=me with tests passing (all comments optional)

tname
} else {
continue;
};

Choose a reason for hiding this comment

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

nit: using a match is more idiomatic here

let span = attrs[0].span;

if !attrs.iter().any(\|a
titems.iter().any(\|t

Choose a reason for hiding this comment

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

nit: this line should be un-indented one space.

}

if !attrs.iter().any(\|a
titems.iter().any(\|t

Choose a reason for hiding this comment

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

(same here)

-> Optionast::Attribute {
verify_derive_attrs(cx, attrs);
get_derive_attr(cx, attrs, DeriveType::Legacy).and_then(\|a
let titem = derive_attr_trait(cx, &a);

Choose a reason for hiding this comment

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

nit: too much indent

return Some(cx.attribute(mitem.span, mitem));
}
}
None

Choose a reason for hiding this comment

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

Using an else to avoid the above return Some is more idiomatic

"custom_derive",
titem.span,
feature_gate::GateIssue::Language,
feature_gate::EXPLAIN_CUSTOM_DERIVE

Choose a reason for hiding this comment

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

the above four lines should be un-indented a space

let name = Symbol::intern(&format!("derive_{}", tname));
if !cx.resolver.is_whitelisted_legacy_custom_derive(name) {
cx.span_warn(titem.span,
feature_gate::EXPLAIN_DEPR_CUSTOM_DERIVE);

Choose a reason for hiding this comment

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

(wrong indent)

Travis seems unhappy with this PR, but happy with others. So try creating a new PR for all this.

[WIP] Expand #[derive(..)]s in the InvocationCollector by keeperofdakeys · Pull Request #39391 · rust-lang/rust (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[WIP] Expand `#[derive(..)]`s in the InvocationCollector by keeperofdakeys · Pull Request #39391 · rust-lang/rust (original) (raw)