wasm-bindgen-webidl doesn't seem to support typedefs

[blm768]

It appears that wasm_bindgen_webidl::compile_ast removes all typedefs via remove_undefined_imports, which prevents typedef bindings from being emitted. This occurs even when working with typedefs that directly reference defined types, such as:

typedef long MyInt;

[fitzgen]

It should only remove aliases of undefined types.

For example, your snippet should work OK:

typedef long MyInt;

But this example should not:

typedef SomeUnknownThing MyThing;

Are you seeing cases that should work but aren't? If so, can you construct a failing test in crates/webidl/tests/all/*? Thanks!

[blm768]

I've put a test in my typedef-test branch. I could just be doing something wrong, but it looks like it should compile.

[fitzgen]

Doh, I totally did it backwards and we are always removing TypeAliases.

Check out ImportedTypes for ast::TypeAlias in crates/backend/src/defined.rs. It is calling f on the dest (aka the alias name) rather than on the src type.

@blm768 fixing this bug should be as easy as s/f(&self.dest)/f(&self.src)/ right there. Would you like to make that change and submit a PR that includes your test case? It would be super appreciated!

[blm768]

I figured it might be that simple. Of course, that wouldn't handle the case of "chained" typedefs, but those should be considerably rarer. Anyway, I should be able to work on that tonight.

[fitzgen]

Double d'oh. We should have:

impl ImportedTypes for ast::TypeAlias {
    fn imported_types<F>(&self, f: &mut F)
    where
        F: FnMut(&Ident, ImportedTypeKind),
    {
        f(&self.dest, ImportedTypeKind::Definition);
        self.src.imported_types(f);
    }
}

[fitzgen]

We also might need to make remove_undefined_imports a fixed point algorithm to properly handle types referencing aliases that reference types that end up getting removed...

// This will end up getting removed.
typedef UnknownType Whatever;

// But this won't because `Whatever` is considered defined..
typedef Whatever UhOh;

[fitzgen]

rust-bindgen has to do a few different fixed point algorithms, and they all implement this trait: https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/ir/analysis/mod.rs#L71-L127

[blm768]

Actually, on a second review, it looks like the WebIDL spec doesn't permit typedefs to reference each other, so we may be safe there. But TypeAlias::src is a syn::Type, not an Ident, so it looks like some interface refactoring may be necessary.

[ohanar]

Actually, I just noticed the following line:

This new name is not exposed by language bindings; it is purely used as a shorthand for referencing the type in the IDL.

So we should probably not be including these typedefs at all, but replacing all their appearances with their underlying type.

[fitzgen]

But TypeAlias::src is a syn::Type, not an Ident, so it looks like some interface refactoring may be necessary.

That's ok, we implement ImportedTypes for syn::Type: https://github.com/rustwasm/wasm-bindgen/blob/master/crates/backend/src/defined.rs#L123

[blm768]

That's ok, we implement ImportedTypes for syn::Type

Oh; I missed that one. That would take care of it.

While I was thinking about the problem, though, it occurred to me that instead of storing a syn::Type in TypeAlias::src, we could have a TypeKind enum that roughly mirrors the one in the webidl crate but is more specifically tailored to wasm-bindgen's features. It seems like that would allow us to eventually generate bindings for other languages if that became a desired feature. Plus turning syn::Type instances into what basically amounts to a plain String feels a little unwieldy to me at this stage of the logic, especially if we want to support generics. Does that seem like something that could be worth looking into?

[blm768]

Another reason to consider using a custom type is that syn seems to be really picky about its execution environment. I keep running into errors like these after trying to go through syn::Type's implementation of ImportedTypes:

Running `/mnt/shared/BLM/Dev/wasm-bindgen/target/debug/build/test15-84ab614921d1d065/build-script-build`
thread 'main' panicked at 'procedural macro API is used outside of a procedural macro', libproc_macro/lib.rs:1489:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[alexcrichton]

@blm768 oh no need to worry about that panic, it's expected for now. Otherwise having a custom type seems reasonable to me! I wouldn't bank too much on generating bindings for other languages, but if it makes the code cleaner for now feel free!

[blm768]

…if it makes the code cleaner for now feel free!

Well, after playing with it a bit, I'm not totally sure about cleaner anymore. I thought we'd be able to get away without having to go from syn::Type to proc_macro2::Ident, but we basically end up needing to do that in the macro crate, so I can't avoid the need for that operation.
On the one hand, having the custom type still feels more "type-safe", but it's also expanding the code size. Only about 150 more lines so far, but I'm not even quite done with the backend and webidl crates, and there are still the others to deal with. I still like the idea, but maybe it's best to wait on making a change that size until there's a more pressing need, such as implementation of #111.
In any case, I'll keep the branch around in case it becomes useful. The "quick fix" seems like it should just be waiting on those panics getting fixed.

[alexcrichton]

Sure thing, either way's fine by me!

[afdw]

This issue is probably suppressed by #623 (at least the main issue, one that is in the title).

[alexcrichton]

Indeed, thanks @afdw!