Stream: t-compiler/rust-analyzer

Topic: Attribute stripping for proc macros


Jonas Schievink [he/him] (May 03 2021 at 15:24, on Zulip):

In https://github.com/rust-analyzer/rust-analyzer/issues/8434, I describe that we have to remove some attributes from custom derive input before calling the proc macro with it. The same applies to attribute macros, for example:

#[inline]
#[proc_macro]
#[cold]
fn f() {}

Then proc_macro's input consists of #[inline] #[cold] fn f() {}, and the attribute macro was removed. This is important because otherwise many proc macros will result in infinite expansion if it's left in place.

I tried to implement this in https://github.com/rust-analyzer/rust-analyzer/pull/8443 but later realized that hir_def needs a better way to tell hir_expand which attributes to remove. Specifically, hir_def's Attr abstraction turns doc comments into attributes, but hir_expand doesn't know that. The solution I'd propose here is:

Another alternative would be to do attribute stripping entirely in hir_def, but I feel like that shouldn't be its responsibility.

Any opinions on this?

Edwin Cheng (May 03 2021 at 15:29, on Zulip):

IIRC, seem like move parts of attr.rs should be simpler option here

matklad (May 03 2021 at 16:36, on Zulip):

Hm, why does hir_def handling of doc comments affect macros?

I think these might be two different things? Macros want to covert /// to #[] token trees during lowering. Hir wants to convert /// to attributues during analysis.

I don't think hir necessary needs to go via #[], it can convert commets directly to hir::Attrs

Jonas Schievink [he/him] (May 03 2021 at 16:40, on Zulip):

It makes it hard to find the right AST node to remove, because now "attribute index 4" also counts doc comments

Jonas Schievink [he/him] (May 03 2021 at 16:41, on Zulip):

If we want to replicate that logic in hir_expand, copying the relevant parts of attr.rs there seems like the right solution

matklad (May 03 2021 at 17:10, on Zulip):

Hm, can we pass the right index to hir_expand?

matklad (May 03 2021 at 17:10, on Zulip):

or rather, can we avoid indicis alltogether, and pass whole SyntaxNodes?

matklad (May 03 2021 at 17:11, on Zulip):

So you'd have something like to_tt(node: ast::Struct, attrs_to_skip: HashSet<ast::Attr>) -> tt::TokenTree ?

Jonas Schievink [he/him] (May 03 2021 at 17:25, on Zulip):

that would work, but hir_expand computes the macro input, so we'd have to restructure how the queries work

matklad (May 03 2021 at 17:32, on Zulip):

ouch, right. So we'd need to embedd that into MacroCallId, and that needs ids...

matklad (May 03 2021 at 17:36, on Zulip):

I guess, my gut feeling is that it makes sense to keep Attrs in the def, and adjust expand's interface such that it is natural for the expand (so, use ids to refer to syntactic attributes, rather than hir_def::Attr attributes), but I am also not that much familiar with the code here.

Jonas Schievink [he/him] (May 03 2021 at 17:40, on Zulip):

I suppose we could do that, yeah. Perhaps by making AttrId store whether it's a doc comment or an attribute or something...

matklad (May 03 2021 at 17:53, on Zulip):

I guess we can avoid AttrIds altogether for this purpose? Like, can we move AttrId to hir_def, and use just ast_attr_position: usize in expand?

matklad (May 03 2021 at 17:53, on Zulip):

We don't need to use the same type for two

Jonas Schievink [he/him] (May 10 2021 at 13:48, on Zulip):

One thing I'm not sure about here is how to derive the AST attribute index from AttrId in the first place (since we are already dealing with semantic Attrs when we need to know the attribute to remove). Doing that also goes back-and-forth between the representations a bit much for my taste.

Jonas Schievink [he/him] (May 10 2021 at 14:18, on Zulip):

I think the easiest way to do this is to encode AttrId as (kind: DocCommentOrRealAttr, syntactic_index: u32), that makes it trivial

Jonas Schievink [he/him] (May 10 2021 at 14:19, on Zulip):

Only it probably doesn't work right with out-of-line modules, but I'm pretty sure hir_def normally doesn't respect inner attributes in any case

matklad (May 10 2021 at 14:30, on Zulip):

:thinking: my general rule of thumb is that back&forth is generally worth it to keep layering sane (rust-analyzer has a lot of that, lsp_types -> ide::* -> hir::* -> hir_def::*, I think it pays off).

On the other hand, its not like we worry about layering a lot at this layer , so if you feel like that's too much layering, feel free to poke a hole in lasagna !

Jonas Schievink [he/him] (May 10 2021 at 14:33, on Zulip):

that layering has benefits though, I don't really see them for AttrId conversion

Jonas Schievink [he/him] (May 20 2021 at 20:10, on Zulip):

this turned out not to work so well because inner attributes require special handling that's done in attr.rs

Jonas Schievink [he/him] (May 21 2021 at 00:52, on Zulip):

should we maybe move a version of hir_def::attr::collect_attrs into libsyntax? seems like an adequate place to answer "which syntactical attributes are on this item?"

matklad (May 21 2021 at 07:54, on Zulip):

uhu

matklad (May 21 2021 at 07:54, on Zulip):

I think AttrsOwner API should be extended, so it has methonds like


matklad (May 21 2021 at 07:55, on Zulip):
fn inner_attrs_and_doc_comments()
fn outer_attrs_and_doc_comments()
fn attrs_and_doc_coments()
Last update: Jul 29 2021 at 21:30UTC