Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmparser: "duplicate identifier for field" in subtype #1092

Closed
patrick96 opened this issue Jun 29, 2023 · 10 comments · Fixed by #1117
Closed

wasmparser: "duplicate identifier for field" in subtype #1092

patrick96 opened this issue Jun 29, 2023 · 10 comments · Fixed by #1117

Comments

@patrick96
Copy link

For the following module:

(module
    (type $A (struct (field $vt (mut i32))))
    (type $B (sub $A (struct (field $vt (mut i32)))))
)

I get the following output:

$ wasm-tools parse demo.wat                                              

error: duplicate identifier for field
     --> demo.wat:3:37
      |
    3 |     (type $B (sub $A (struct (field $vt (mut i32)))))
      |                                     ^

For subtypes, the same field in the subtype has to have the same name as in the supertype, otherwise it's impossible to do any kind of field manipulation unless you know the exact type of the object.

In fact, the spec for the gc proposal says:

For fields, names need only be unique within a single type.
-- document/core/text/conventions.rst


This happens both on main (0263c2f) and version 1.0.35.

@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2023

Thanks for filing an issue!

Just a heads up, our GC support is very much still a work in progress.

@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2023

cc @imikushin

@patrick96
Copy link
Author

Just a heads up, our GC support is very much still a work in progress.

Like most other tooling out there ;)

Appreciate your work on this

@imikushin
Copy link
Contributor

Added a simple fix for this to #1059 which happens to add subtype support to wasmparser :)

@imikushin
Copy link
Contributor

Okay, the fix above will work, but with one caveat: field namespace is global per module.

So, current limitation: fields with the same name can only be at the same position, even in totally different structs 😅

@patrick96
Copy link
Author

So, current limitation: fields with the same name can only be at the same position, even in totally different structs 😅

This works for my use-case since field names are distinct in unrelated structs. But I guess the spec won't allow for this limitation.

Also, with #1059 I am able to successfully assemble my webassembly text file (at least at the current state, my codegen is still missing a ton of things).

@imikushin
Copy link
Contributor

imikushin commented Jul 5, 2023

@patrick96 unfortunately, by @alexcrichton 's suggestion (#1059 (comment)), I had to remove the temporary dirty fix from #1059 . Good news is I'm going to send a proper fix after #1059 is merged.

@patrick96
Copy link
Author

That's fine, keep doing what you're doing :)

imikushin added a commit to imikushin/wasm-tools that referenced this issue Jul 10, 2023
Field namespaces are per struct now. Inherited fields' names must be exactly the same as in the super-type.

Fixes bytecodealliance#1092.
@imikushin
Copy link
Contributor

Submitted a fix ⬆️

imikushin added a commit to imikushin/wasm-tools that referenced this issue Jul 11, 2023
Field namespaces are per struct now. Inherited fields' names must be exactly the same as in the super-type.

Fixes bytecodealliance#1092.
alexcrichton pushed a commit that referenced this issue Jul 12, 2023
* Fix "duplicate identifier for field" for subtype fields

Field namespaces are per struct now. Inherited fields' names must be exactly the same as in the super-type.

Fixes #1092.

* Remove some unused imports

* Address review comments

* Address review comments

* Fix a broken test
@patrick96
Copy link
Author

Awesome! Thanks for all your efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants