Stream: t-compiler/rust-analyzer

Topic: Support builtin macros


Edwin Cheng (Nov 05 2019 at 04:56, on Zulip):

I am starting to work on adding builtin macros. (e.g: concat!) . My initial thought is to create each custom MacroDef for each builtin macros and registers these in the beginning of module collecting phase. Basically changing MacroDef as following:

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum MacroDefId {
    MacroDefSource(MacroDefSource),
    MacroDefBuiltin(...)
}

// `MacroDefSource` is the normal MacroDef
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct MacroDefSource {
    pub krate: CrateId,
    pub ast_id: AstId<ast::MacroCall>,
}

Another option is to add a new type ModuleDef::BultinMacro and handle it separately (Although I hadn't figured out the whole path for how to do it)

Do anyone have any idea?

Edwin Cheng (Nov 05 2019 at 16:04, on Zulip):

@matklad After studying how rustc implement concat! macro, I finally understand what you mean "include! and concat! seem to use "eager expansion" of some sorts.".

For example:

macro_rules! foo {
    () => {a}
}
fn main() {
    println!("{}", concat!(foo!(), bar!()));
}

When we try to expand concat!, we have to expand foo!() and bar!() first. And this order is different from what normal mbe is handled.

matklad (Nov 06 2019 at 09:53, on Zulip):

@Edwin Cheng yeah, I think we need to extend MacroDefId. I imagine, in the end it might want to look like

enum MacroDefId {
  DeclarativeMacro(DeclarativeMacro),
  ProcMacro(ProcMacro),
  BuiltinMacro(BuiltinMacro),
}

I think it makes sense to just hard-code bulitin macrod right now for simplicity (so, don't add them to ModuleDef), but, long term, I think nameres should be aware of such macros.

matklad (Nov 06 2019 at 09:55, on Zulip):

And yeah, the hardest part about build-in macros is that they are not necessary restricted to tt -> tt interface, and might do all sorts of weird call-backs into the compiler. I hope we can handle them clearly withing hir_expand framework, but we must brace ourselves for the worst just in case :)

Edwin Cheng (Nov 07 2019 at 13:14, on Zulip):

I just checked, seem like there are only 2 builtin macros which use eagar expansion. So my first MVP will be implement line! macro first. It seem like not depending on anything complicated.

Edwin Cheng (Nov 07 2019 at 13:14, on Zulip):

Another question:, I found that for every builtin macro, An attribute rustc_builtin_macro must be used. link. But I don't know it is a good idea depends on it to recognize the input path as bultin-macro because I don't know how stable it is.

On the other hand, it simplifies the whole builtin macro name resolution alot; We could just mark the macro in name resolution phase as builtin, and handle it in ModCollector.

matklad (Nov 07 2019 at 13:16, on Zulip):

@Edwin Cheng that attribute is a relatively new addition, but I think we can rely on it

matklad (Nov 07 2019 at 13:16, on Zulip):

Even if it breaks later, we can just fix the code

Edwin Cheng (Nov 13 2019 at 14:11, on Zulip):

I figure out how to do eager macro expansion in current infrastructure finally and implemented concat!. However I can’t test it in whole def collection phase since there’s no way to expand it before include! is implemented. 🙄

matklad (Nov 13 2019 at 14:13, on Zulip):

hm..... Should we have dedicated macro expansion tests for built-in macros?

Edwin Cheng (Nov 13 2019 at 15:30, on Zulip):

Yes, sure we should. However, the actual problem here is: To test the macro name resolution in items scope, we have to start with a macro which expand to Macro::Items, concat itself only expands to expr. Of course I can write a test for it without triggering eager expansion but it is not ideal.

matklad (Nov 13 2019 at 15:32, on Zulip):

Hm, and I guess, we need name-resolution to write non-trivial test cases?

Like, it seems like it should be possible to test macro expansion without nameres, in theory, but in practice you need name resolution for interesting test cases

Edwin Cheng (Nov 15 2019 at 01:55, on Zulip):

Like, it seems like it should be possible to test macro expansion without nameres, in theory, but in practice you need name resolution for interesting test cases

Yes, and FYI : in #2244, I added some trivial tests without name resolutions.

Edwin Cheng (Nov 15 2019 at 02:01, on Zulip):

After that PR, I will work on include!, but I have some questions :

Edwin Cheng (Nov 15 2019 at 02:02, on Zulip):
  1. If I understand correctly, currently the file loading is "push" model: It insert files at the beginning of project loading. However, how do I handle the following case (which exists in my other project):
 let font = Vec::from(include_bytes!("DejaVuSans.ttf") as &[u8]);

I don't think we push all files in SourceRoot, right?

Edwin Cheng (Nov 15 2019 at 02:17, on Zulip):
  1. Imagine to support something like this (https://github.com/intellij-rust/intellij-rust/pull/4542):
    The HirFileId of the expanded include! macro is {MacroFile(MacroCallId(lib.rs:include!(concat(..)))} . Note that we don't have the information of hello.rs in the chain of HirFileId.

Any idea how to handle it correctly ?

matklad (Nov 15 2019 at 06:04, on Zulip):

Yeah, the model is push-based. Specifically, rust-analyzer need to know that a file exists before reading it. I've spend about a week trying to transition to a fully-pull based model, but figured out that, ideally, we want a hybrid model.

The set of files is push based, while the contents is pulled. The main reason for this is that, for file watching to be reliable, you need to scan for all files anyway. In particular, if you subscribe to a glob using watchman, it will immediately send all file paths to you as modified.

matklad (Nov 15 2019 at 06:05, on Zulip):

So, for DejaVuSans.ttf, the user will have to explicitly configure rust-analyzer to care about files with .ttf extension

matklad (Nov 15 2019 at 06:09, on Zulip):
  1. My gut feeling that maybe we shouldn't create {MacroFile(MacroCallId(lib.rs:include!(concat(..)))}? Like, when we made CallID with include!, we already know that it is special. So perhaps we can expand it at that point and use FilleId?
Edwin Cheng (Nov 15 2019 at 07:44, on Zulip):

My gut feeling that maybe we shouldn't create {MacroFile(MacroCallId(lib.rs:include!(concat(..)))}? Like, when we made CallID with include!, we already know that it is special. So perhaps we can expand it at that point and use FilleId?

It is an interesting solution, let me try to implement it first and see if there is any problem. :+1:

Edwin Cheng (Nov 16 2019 at 05:02, on Zulip):

@matklad I'm trying to implement the new eager expansion method, I have a question: As the new expanded syntax node is not belonged to any HirFileId, This line of code basically must fail. How do I solve this ?

Edwin Cheng (Nov 16 2019 at 06:14, on Zulip):

(Deleted)

matklad (Nov 16 2019 at 09:10, on Zulip):

@Edwin Cheng I think you souldn't create SyntaxNodePtrs into the eagarly expanded tokens at all

Edwin Cheng (Nov 16 2019 at 09:11, on Zulip):

I didn't create it, but it is created by other place I didn't know ...I am tracing it now

Edwin Cheng (Nov 28 2019 at 06:37, on Zulip):

@maklad I am thinking how to handle builtin macro and eager macro in a general way, let me show some code first:

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct MacroCallLoc {
    pub(crate) def: MacroDefId,
    pub(crate) ast_id: AstId<ast::MacroCall>,
    pub(crate) ctx: Option<MacroCallSiteCtxId>,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum MacroCallSiteCtx {
    CurrentFile(FileId),
    Eager(Arc<tt::Subtree>),
    ...
}

pub struct MacroCallSiteCtxId(salsa::InternId);

My reasoning of this change is, a lot of builtin macros depends on some kind of call site context, such as File Position, Name resolution, Error reporting data etc. If we think of macro expansion as pure token manipulation only, this kind of context should be provided outside of macro_expand. (For example, we COULD compute file position by using MacroCallId::ast_id inside macro_expand) .

matklad (Nov 28 2019 at 08:39, on Zulip):

@Edwin Cheng I would avoid add Context to as a query-parameter: it seems like it can be huge, and would be a bad fit for storage

matklad (Nov 28 2019 at 08:41, on Zulip):

INstead, I would do something like this:

trait AstDatabase {
    #[salsa::inter]
    fn intern_eager_expansion(tt: tt::Subtree) -> EagerExpansionId
}

enum MacroCallId {
    Lazy(our current id here)
    Eager(EagerExpansionId)
}
matklad (Nov 28 2019 at 08:42, on Zulip):

That way, when you are processing some code and face an eager macro, you can expand it to token tree using whatever context you have on the stack, and then, to produce appropriate HirFIleId for source maps and such, you can intern the result of expanion into an id

Edwin Cheng (Nov 28 2019 at 08:57, on Zulip):

Do you think we should move out the builtin macro expansion out of macro_expand such that:

// In ra_hir_expand/src/db.rs
impl TokenExpander {
    pub fn expand(
        &self,
        db: &dyn AstDatabase,
        id: MacroCallId,
        tt: &tt::Subtree,
    ) -> Result<tt::Subtree, mbe::ExpandError> {
        match id {
            Eager(e) => db.lookup_eager(id),  // return it directly
            Lazy(rules) => rules.expand(tt),
        }
    }
}

And we expand the completed result of builtin macros as eager expansion at the call site ? For example, the eager id of concat!("a", "b") is pointing to "ab" instead of ("a","b") directly?

matklad (Nov 28 2019 at 08:59, on Zulip):

For example, the eager id of concat!("a", "b") is pointing to "ab" instead of ("a","b") directly?

I think this should work this way, but I can't explain why this would be better, so it might be actually worse. But I do feel that it's a better approach

matklad (Nov 28 2019 at 09:03, on Zulip):

Do you think we should move out the builtin macro expansion out of macro_expand such that:

Heh, maybe! I know that we should provide a nice single entry-point for macro expansion (like we do for Resolver). Currently we do macro expansion in def collector and body::lower, which is not great, and when we support macros in types we will have to expand macros from various places in hir_ty. It's pretty bad that at the moment every macro expansion is basically "do it yourself". There's a nascent expander for expressions, but it is really not well thought out.

We need to add a proper expander, that works in all contexts, deals with hygiene properly, and handles both eager and lazy macros. But I didn't have a chance yet to think how it's API should looks like. It probably should live in the hir_expand crate and get name resolution info via callback.

matklad (Nov 28 2019 at 09:04, on Zulip):

(oh, and it should handle derives (and maybe even cfg's ?) as well)

Edwin Cheng (Nov 28 2019 at 09:16, on Zulip):

Yes, and one of key point I didn't think of before I implement to_macro_file_kind, we could know the final Node Kind a MacroCallId will expand without parent context. So in theory the whole macro_expand works could be self-contained.

Edwin Cheng (Jan 30 2020 at 17:32, on Zulip):

Since https://github.com/rust-lang/cargo/pull/7622 is landed, seem like it is possible to implement env! builtin macro now, right ?

matklad (Jan 30 2020 at 17:33, on Zulip):

right!

Edwin Cheng (Jan 30 2020 at 17:33, on Zulip):

And of course another roadblock is eager macro ..

Edwin Cheng (Jan 30 2020 at 17:34, on Zulip):

I remember there is a discussion about eager macro and salsa..

matklad (Jan 30 2020 at 17:35, on Zulip):

which actually didn't happen. Still, It think the idea with just interning eager macro expansions would work

Edwin Cheng (Jan 30 2020 at 17:36, on Zulip):

Okay !

matklad (Jan 30 2020 at 17:36, on Zulip):

We add EagerMaro(id) variant to MacroCallid, and we add #[salsa::inten] fn intern_eager_expansion(result: tt::Subtree) -> EagerMacroId

Edwin Cheng (Jan 30 2020 at 17:41, on Zulip):

If I remember correctly, Currently we have three places to resolve ast::MacroCall MacroCallId :

  1. Item collection phase
  2. Lowering body
  3. SourceAnalyzer

Do we want to refactoring it to a single place first ? I did tried that but without success :(

matklad (Jan 30 2020 at 17:43, on Zulip):

Yeah, creating a single expander would be helpful!

Edwin Cheng (Jan 30 2020 at 17:49, on Zulip):

Let me try again, but recently there is some awkward situation in HK (the damn virus) and maybe it would a little bit slower :(

matklad (Jan 30 2020 at 17:50, on Zulip):

:heart:

Be sure to keep yourself safe!

Edwin Cheng (Jan 30 2020 at 17:50, on Zulip):

sure! thanks :heart: !

Edwin Cheng (Mar 01 2020 at 11:34, on Zulip):
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum MacroCallId {
    LazyMacro(LazyMacroId),
    EagerMacro(EagerMacroId),
}

I am not satisfied the name of LazyMacro , Would it be better if I name it to OrdinaryMacro instead ?

Last update: Jul 29 2021 at 08:15UTC