Stream: t-compiler/rust-analyzer

Topic: macro expansion at type alias position


cynecx (Apr 05 2021 at 20:52, on Zulip):

Hey, I am slowly reading/digging more into hir_def to figure out an approach to support macro expansion at type alias positions:

trait Foo {
    type Ty;
}

impl<T> Foo for T {
    type Ty = U32!();
}

type TayTo = U32!();

fn testy() {
    let a: <() as Mom>::M; // currently: {unknown}
    let b: TayTo; // currently: {unknown}
}

I think the first step might be to modify TypeAlias' ast (therefor rust.ungrammar) to also include an optional MacroCall. I was then looking into the ItemTree lowering code, which should "lower" the MacroCall besides storing the type. At this point I am quite unsure, whether we would want to expand the MacroCall eagerly here (and the implications of doing so), but this wouldn't fit into the current "model", whereas just like current MacroCalls are expanded later in nameres::Collector.

I was wondering what approach/direction would be the "best" here.

Jonas Schievink [he/him] (Apr 05 2021 at 21:06, on Zulip):

They cannot be expanded by the item tree, since that's crate-independent

Florian Diebold (Apr 05 2021 at 21:15, on Zulip):

I think here would be a good place to start? it seems to me the syntax tree already supports macro calls in types, we just have to lower them there

cynecx (Apr 05 2021 at 21:21, on Zulip):

Thanks, I'll dig more into it!

cynecx (Apr 09 2021 at 17:31, on Zulip):

@Florian Diebold Is it actually possible to expand the macrocall at that point? (since that TypeRef will be constructed at ItemTree lowering)

Jonas Schievink [he/him] (Apr 09 2021 at 17:33, on Zulip):

no, it needs to be expanded later

Florian Diebold (Apr 09 2021 at 17:35, on Zulip):

hmm true

cynecx (Apr 09 2021 at 17:35, on Zulip):

is there some part at this stage that also does this (delayed expansion)?

Jonas Schievink [he/him] (Apr 09 2021 at 17:39, on Zulip):

you can either add a Macro variant to reflect this, or split TypeRef into pre- and post-expansion

Jonas Schievink [he/him] (Apr 09 2021 at 17:39, on Zulip):

the expansion would happen in the struct_data (etc.) queries, as well as during body lowering (for types in let statements and expressions)

cynecx (Apr 09 2021 at 17:42, on Zulip):

I actually thought about your first suggestion, however I wasn't quite sure how exactly that would work. But I think I might give this a go first! Thanks.

cynecx (Apr 09 2021 at 20:26, on Zulip):

I am getting the following macro expansion with:

let mut expander = Expander::new(db, file_id, module_id);
match dbg!(expander.enter_expand::<ast::Type>(db, call)) { // doesn't work because see below

on

macro_rules! U32 {
    () => { u32 };
}

trait Foo {
    type Ty;
}

impl<T> Foo for T {
    type Ty = U32!();
}

:

[crates/hir_def/src/body.rs:136] db.parse_or_expand(file_id) = Some(
    MACRO_ITEMS@0..3
      MACRO_CALL@0..3
        PATH@0..3
          PATH_SEGMENT@0..3
            NAME_REF@0..3
              IDENT@0..3 "u32"
    ,
)
[crates/hir_def/src/data.rs:139] expander.enter_expand::<ast::Type>(db, call) = Ok(
    ExpandResult {
        value: None,
        err: None,
    },
)

I didn't expect that the expansion would have a MACRO_CALL or MACRO_ITEMS. Am I doing something wrong?

cynecx (Apr 09 2021 at 21:19, on Zulip):

Ah hir_expand::db::to_fragment_kind doesn't seem to cover this.

cynecx (Apr 09 2021 at 21:22, on Zulip):

Nice, extending the match with MACRO_TYPE => FragmentKind::Type, fixes the macro expansion.

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

Now that I think about this, it might be easier to start with macro expansion in pattern position – that only has to happen inside item bodies, which happens in a single place in rust-analyzer

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

I might look into that

cynecx (Apr 10 2021 at 19:34, on Zulip):

I actually do have a working implementation at type alias position. I am currently working on cleaning up stuff. My hopes were that fixing this would get https://github.com/rust-analyzer/rust-analyzer/issues/4774 to work, however it seems that there is some issue with the complex trait bounds somewhere.

cynecx (Apr 10 2021 at 19:38, on Zulip):

However I might wanna open a WIP PR, I am absolutely not sure about the general ideal ive taken.

cynecx (Apr 10 2021 at 19:44, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/8462

Jonas Schievink [he/him] (Apr 10 2021 at 21:36, on Zulip):

opened https://github.com/rust-analyzer/rust-analyzer/pull/8463 for macros in pattern position

cynecx (Apr 10 2021 at 23:29, on Zulip):

I am currently looking into "expanding" a TypeRef, which now has Macro variant. My first attempt would be to walk the TypeRef and mutably mem::replace all nestedMacro variants. I am not sure if this is a good idea since we might mem::replace an interned TypeRef, which I think might cause issue because, well interning?

Jonas Schievink [he/him] (Apr 10 2021 at 23:31, on Zulip):

Interned TypeRefs are immutable, so you can only get immutable refs to the interior

Jonas Schievink [he/him] (Apr 10 2021 at 23:32, on Zulip):

the expansion will have to allocate and intern a new typeref

cynecx (Apr 10 2021 at 23:32, on Zulip):

Uff, that's kind of cumbersome xD.

cynecx (Apr 10 2021 at 23:34, on Zulip):

okay it's 1am. lets go

Jonas Schievink [he/him] (Apr 10 2021 at 23:36, on Zulip):

yeah, it's basically a type fold but without having type folding infra

cynecx (Apr 11 2021 at 15:56, on Zulip):

I am currently using the following helper:

use std::panic::{self, UnwindSafe};

#[inline(always)]
fn replace_with<T: UnwindSafe, F: FnOnce(T) -> T + UnwindSafe>(dest: &mut T, f: F) {
    unsafe {
        let dest_ptr = dest as *mut T;
        let old = dest_ptr.read();
        let new = match panic::catch_unwind(|| f(old)) {
            Ok(val) => val,
            Err(err) => {
                eprintln!("{:?}", err);
                std::process::abort();
            }
        };
        dest_ptr.write(new);
    }
}

Is that sound? (I am using that helper in my hacky type folding code). If so, would it be okay if i could add that to stdx?

cynecx (Apr 11 2021 at 15:57, on Zulip):

The catch with that helper is that an inner panic causes an abort. But I'd say that's okay?

Daniel Mcnab (Apr 11 2021 at 16:28, on Zulip):

It's probably better to use https://docs.rs/replace_with/0.1.7/replace_with/ directly?

Lukas Wirth (Apr 11 2021 at 16:32, on Zulip):

It seems weird to me that you need this in general

cynecx (Apr 11 2021 at 17:15, on Zulip):

Well, I am currently trying to transform/map each vec entry (Vec<TypeRef>) without allocating another vec . Afaik my options are replace_with or two mem::replaces with a dummy (TypeRef::Error?).

cynecx (Apr 11 2021 at 17:15, on Zulip):

Or is this premature optimization, idk.

Lukas Wirth (Apr 11 2021 at 17:17, on Zulip):

whats preventing you from borrowing from the vec and writing to the reference directly? I'm not sure I understand why this function here would be required

Florian Diebold (Apr 11 2021 at 17:18, on Zulip):

I would prefer replacing with TypeRef::Error, but I'm not sure you need to avoid the allocation? won't you need a copy in the end anyway?

cynecx (Apr 11 2021 at 17:19, on Zulip):

I was following the following pattern:

fn fold_type_ref(type_ref: TypeRef, f: &mut impl FnMut(&TypeRef) -> Option<TypeRef>) -> TypeRef {...}
fn fold_path(path: Path, f: &mut impl FnMut(&TypeRef) -> Option<TypeRef>) -> Path
cynecx (Apr 11 2021 at 17:20, on Zulip):

My attempt before was based on mutable borrows however that didn't work well for some reason.

Florian Diebold (Apr 11 2021 at 17:22, on Zulip):

the item tree will contain unexpanded TypeRefs, right? so you can't expand in place anyway. I'd probably make it &TypeRef -> TypeRef?

Florian Diebold (Apr 11 2021 at 17:23, on Zulip):

btw @Jonas Schievink [he/him] I just realized that TypeRef still uses boxes instead of Interned, did you try interning the insides of typerefs as well?

cynecx (Apr 11 2021 at 17:24, on Zulip):

It's kind of hard to explain. Here is my current attempt: https://gist.github.com/cynecx/3f406413e7c57e233d43a32abbbfcee8

Florian Diebold (Apr 11 2021 at 17:26, on Zulip):

I think I would first try to write the expansion code specifically, without building a whole fold infrastructure, and make it clone as it goes

Florian Diebold (Apr 11 2021 at 17:27, on Zulip):

I actually doubt that we will have many more uses for such an infrastructure, because mostly that happens with lowered Tys

Florian Diebold (Apr 11 2021 at 17:29, on Zulip):

maybe we could even do the expansion during lowering? :thinking: I hesitate to make type lowering more complicated, but it might fit in there

Florian Diebold (Apr 11 2021 at 17:30, on Zulip):

(I'm talking about this)

cynecx (Apr 11 2021 at 17:30, on Zulip):

What kind of lowering are you talking about?

cynecx (Apr 11 2021 at 17:30, on Zulip):

oh didnt see your recent message yet

Florian Diebold (Apr 11 2021 at 17:31, on Zulip):

that's going from TypeRef to Ty, it could probably macroexpand as it goes

cynecx (Apr 11 2021 at 17:31, on Zulip):

Oh

Florian Diebold (Apr 11 2021 at 17:31, on Zulip):

I'm not sure it's a good idea though

cynecx (Apr 11 2021 at 17:32, on Zulip):

Didn't @Jonas Schievink [he/him] actually mentioned this on GH?:

He wrote:

Yeah, I currently think the best way to do this is to add another variant to the existing TypeRef. It should then be replaced by the expanded type during lowering, or left as-is if the macro fails to resolve/expand.

I was wondering what lowering he was talking about.

Florian Diebold (Apr 11 2021 at 17:33, on Zulip):

maybe :thinking:

cynecx (Apr 11 2021 at 17:34, on Zulip):

That would be even easier actually, without type folding.

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

I was actually thinking about TypeRef -> TypeRef lowering in data.rs, adt.rs, and body lowering, but doing it when lowering to Ty might be easier, yeah

Jonas Schievink [he/him] (Apr 11 2021 at 17:44, on Zulip):

Florian Diebold said:

btw Jonas Schievink [he/him] I just realized that TypeRef still uses boxes instead of Interned, did you try interning the insides of typerefs as well?

No, but that's probably something we should do

cynecx (Apr 11 2021 at 17:44, on Zulip):

I was actually thinking about TypeRef -> TypeRef lowering in data.rs, adt.rs, and body lowering,

That's partially how my local branch works right now. Does it have any implications when TypeRef expansion is moved to the TypeRef -> Ty lowering code? Besides being easier to implement? (Also that would include inserting TypeRef expansion calls everytime a TypeRef is referenced in data.rs, adt.rs, ...)

Florian Diebold (Apr 11 2021 at 18:06, on Zulip):

it slightly changes what's recomputed in which cases when code changes, ... but probably not enough to matter :thinking:

Last update: Jul 26 2021 at 13:30UTC