Add expansion infrastructure for derive macros #2479

flodiebold · 2019-12-05T14:28:48Z

I thought I'd experiment a bit with attribute macro/derive expansion, and here's what I've got so far. It has dummy implementations of the Copy / Clone derives, to show that the approach works; it doesn't add any attribute macro support, but I think that fits into the architecture.

Basically, during raw item collection, we look at the attributes and generate macro calls for them if necessary. Currently I only do this for derives, and just add the derive macro calls as separate calls next to the item. I think for derives, it's important that they don't obscure the actual item, since they can't actually change it (e.g. sending the item token tree through macro expansion unnecessarily might make completion within it more complicated).

Attribute macros would have to be recognized at that stage and replace the item (i.e., the raw item collector will just emit an attribute macro call, and not the item). I think when we implement this, we should try to recognize known inert attributes, so that we don't do macro expansion unnecessarily; anything that isn't known needs to be treated as a possible attribute macro call (since the raw item collector can't resolve the macro yet).

There's basically no name resolution for attribute macros implemented, I just hardcoded the built-in derives. In the future, the built-ins should work within the normal name resolution infrastructure; the problem there is that the builtin stubs in std use macros 2.0, which we don't support yet (and adding support is outside the scope of this).

One aspect that I don't really have a solution for, but I don't know how important it is, is removing the attribute itself from its input. I'm pretty sure rustc leaves out the attribute macro from the input, but to do that, we'd have to create a completely new syntax node. I guess we could do it when / after converting to a token tree.

matklad

Overall, this looks good to me, but I think attribute -> macro call converssion should happen after raw items step: I believe in general one needs name resolution to figure out if an attribute is a macro or not, and we can't have that in RawItems.

it might also be a good idea to at least extract the correct name in the dummy impl, just to make sure that we correctly thread the right syntax tree.

The question of stripping attributes is interesting... In general, I believe we would have to really create new syntax nodes and basically treat attributed items as

my_macro! {
    fn item_goes_here() {}
}

completion would then have to just deal with it, by merging syntax trees at the call-site and def-site.

crates/ra_hir_def/src/attr.rs

matklad · 2019-12-05T14:48:48Z

crates/ra_hir_def/src/nameres/raw.rs

+    pub(super) path: Path,
+    /// Whether this is a derive invocation. If so, `path` is just the trait path.
+    pub(super) derive: bool,
+}


Do we really need to have this at the raw_items layer? Raw items already have attributes, so it seems like we could and should deal with it at the DefCollector layer? I think that becomes required when we add proper name resolution to the mix.

I think we need to treat any unknown attribute as a macro invocation anyway. I'll try whether it's simpler to do it in the def collector, though.

we need to treat any unknown attribute as a macro invocation anyway

Hmm, we might be able to figure out that some attributes are inert using name resolution though, so that's a good point 🤔

Yeah, that works, actually...

matklad · 2019-12-05T14:57:45Z

crates/ra_hir_expand/src/ast_id_map.rs

@@ -53,7 +53,7 @@ impl AstIdMap {
    pub(crate) fn from_source(node: &SyntaxNode) -> AstIdMap {
        assert!(node.parent().is_none());
        let mut res = AstIdMap { arena: Arena::default() };
-        // By walking the tree in bread-first order we make sure that parents


crates/ra_hir_expand/src/lib.rs

matklad · 2019-12-05T15:10:50Z

crates/ra_hir_expand/src/lib.rs

+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum MacroCallKind {
+    FnLike(AstId<ast::MacroCall>),
+    Attr(AstId<ast::ModuleItem>),


This probably should include some kind of index, to tell which attribute on the item this is.

Hm, or maybe not? Attributes are expanded in order, so this can always mean first attribute

I think it'd always be the first (non-inert one). If you have multiple macro attributes on an item, the first one gets applied, and then the second macro call happens inside the expansion 'file', right?

This is "mostly right" I think modulo, ahem, some backwards compatibility cases: rust-lang/rust#66529 (comment)

Yeah, derive helper attributes are very weird anyway. The derive helper attribute is inert though, so it's still the first non-inert one ;)

flodiebold · 2019-12-05T15:39:17Z

it might also be a good idea to at least extract the correct name in the dummy impl, just to make sure that we correctly thread the right syntax tree.

Oh yeah, I was planning on implementing them a bit more properly if you think the approach makes sense 🙂

matklad · 2019-12-05T16:27:38Z

crates/ra_hir_def/src/nameres/collector.rs

+                self.def_collector.unexpanded_attribute_macros.push((self.module_id, ast_id, path));
+            }
+        }
+    }


Yeah, this looks better!

…to token tree Otherwise parsing them again doesn't work.

flodiebold · 2019-12-05T18:35:53Z

@matklad ok, implemented the derives more 'properly' now. I had to adapt the syntax node -> token tree conversion a bit because the parser can't deal with the invisible delimiters, not completely sure about that.

Since as long as we're not implementing the bodies, they all work the same way.

flodiebold · 2019-12-05T19:00:07Z

Actually I just added all the other built-in derives, since the stubs look the same for all of them...

matklad · 2019-12-05T20:00:03Z

😍

bors merge

💯

2479: Add expansion infrastructure for derive macros r=matklad a=flodiebold I thought I'd experiment a bit with attribute macro/derive expansion, and here's what I've got so far. It has dummy implementations of the Copy / Clone derives, to show that the approach works; it doesn't add any attribute macro support, but I think that fits into the architecture. Basically, during raw item collection, we look at the attributes and generate macro calls for them if necessary. Currently I only do this for derives, and just add the derive macro calls as separate calls next to the item. I think for derives, it's important that they don't obscure the actual item, since they can't actually change it (e.g. sending the item token tree through macro expansion unnecessarily might make completion within it more complicated). Attribute macros would have to be recognized at that stage and replace the item (i.e., the raw item collector will just emit an attribute macro call, and not the item). I think when we implement this, we should try to recognize known inert attributes, so that we don't do macro expansion unnecessarily; anything that isn't known needs to be treated as a possible attribute macro call (since the raw item collector can't resolve the macro yet). There's basically no name resolution for attribute macros implemented, I just hardcoded the built-in derives. In the future, the built-ins should work within the normal name resolution infrastructure; the problem there is that the builtin stubs in `std` use macros 2.0, which we don't support yet (and adding support is outside the scope of this). One aspect that I don't really have a solution for, but I don't know how important it is, is removing the attribute itself from its input. I'm pretty sure rustc leaves out the attribute macro from the input, but to do that, we'd have to create a completely new syntax node. I guess we could do it when / after converting to a token tree. Co-authored-by: Florian Diebold <flodiebold@gmail.com>

bors · 2019-12-05T20:08:39Z

Build succeeded

TypeScript
Rust

flodiebold requested a review from matklad December 5, 2019 14:28

matklad reviewed Dec 5, 2019

View reviewed changes

Add expansion infrastructure for derive macros

18f6a99

flodiebold force-pushed the derive branch from 12146d5 to 18f6a99 Compare December 5, 2019 16:24

matklad reviewed Dec 5, 2019

View reviewed changes

flodiebold added 2 commits December 5, 2019 19:29

Don't wrap most syntax trees in invisible delimiters when converting …

ab4ecca

…to token tree Otherwise parsing them again doesn't work.

Implement derive(Copy, Clone) properly (well, kind of)

db8a00b

flodiebold marked this pull request as ready for review December 5, 2019 18:30

Implement all the other built-in derives

1069704

Since as long as we're not implementing the bodies, they all work the same way.

bors bot merged commit 1069704 into rust-lang:master Dec 5, 2019

flodiebold deleted the derive branch December 5, 2019 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expansion infrastructure for derive macros #2479

Add expansion infrastructure for derive macros #2479

flodiebold commented Dec 5, 2019

matklad left a comment

matklad Dec 5, 2019

flodiebold Dec 5, 2019

flodiebold Dec 5, 2019

flodiebold Dec 5, 2019

matklad Dec 5, 2019

matklad Dec 5, 2019

matklad Dec 5, 2019

flodiebold Dec 5, 2019

matklad Dec 5, 2019

flodiebold Dec 5, 2019

flodiebold commented Dec 5, 2019

matklad Dec 5, 2019

flodiebold commented Dec 5, 2019

flodiebold commented Dec 5, 2019

matklad commented Dec 5, 2019

bors bot commented Dec 5, 2019

Add expansion infrastructure for derive macros #2479

Add expansion infrastructure for derive macros #2479

Conversation

flodiebold commented Dec 5, 2019

matklad left a 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

flodiebold commented Dec 5, 2019

Choose a reason for hiding this comment

flodiebold commented Dec 5, 2019

flodiebold commented Dec 5, 2019

matklad commented Dec 5, 2019

bors bot commented Dec 5, 2019

Build succeeded