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.
After that PR, I will work on include!, but I have questions :
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?
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 ?
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.
So, for DejaVuSans.ttf, the user will have to explicitly configure rust-analyzer to care about files with .ttf extension
{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?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:
@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 ?
Okay, seem like I find what's happening :
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:
@Edwin Cheng I think you souldn't create SyntaxNodePtrs into the eagarly expanded tokens at all
I didn't create it, but it is created by other place I didn't know ...I am tracing it now
@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) .
@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
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)
}
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
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?
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
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.
(oh, and it should handle derives (and maybe even cfg's ?) as well)
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.
Since https://github.com/rust-lang/cargo/pull/7622 is landed, seem like it is possible to implement env! builtin macro now, right ?
right!
And of course another roadblock is eager macro ..
I remember there is a discussion about eager macro and salsa..
which actually didn't happen. Still, It think the idea with just interning eager macro expansions would work
Okay !
We add EagerMaro(id) variant to MacroCallid, and we add #[salsa::inten] fn intern_eager_expansion(result: tt::Subtree) -> EagerMacroId
If I remember correctly, Currently we have three places to resolve ast::MacroCall MacroCallId :
Do we want to refactoring it to a single place first ? I did tried that but without success :(
Yeah, creating a single expander would be helpful!
Let me try again, but recently there is some awkward situation in HK (the damn virus) and maybe it would a little bit slower :(
:heart:
Be sure to keep yourself safe!
sure! thanks and you too!
#[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 named to to OrdinaryMacro instead ?