Stream: t-compiler/wg-rls-2.0

Topic: Support builtin macros


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 questions :

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

I don't think we push that 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):

Okay, seem like I find what's happening :

https://github.com/rust-analyzer/rust-analyzer/blob/d9d99369b2765eaef7f49cd519990769191c3381/crates/ra_hir/src/from_source.rs#L81

impl FromSource for MacroDef {
    type Ast = ast::MacroCall;
    fn from_source(db: &(impl DefDatabase + AstDatabase), src: Source<Self::Ast>) -> Option<Self> {
        let kind = MacroDefKind::Declarative;

        let module_src = ModuleSource::from_child_node(db, src.as_ref().map(|it| it.syntax()));
        let module = Module::from_definition(db, Source::new(src.file_id, module_src))?;
        let krate = module.krate().crate_id();

        let ast_id = AstId::new(src.file_id, db.ast_id_map(src.file_id).ast_id(&src.ast));

        let id: MacroDefId = MacroDefId { krate, ast_id, kind };
        Some(MacroDef { id })
    }
}

Um... :thinking:

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 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.

Last update: Dec 12 2019 at 00:50UTC