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

feat(wit-parser): add parser for include #1054

Merged
merged 15 commits into from
Jun 13, 2023

Conversation

Mossaka
Copy link
Member

@Mossaka Mossaka commented May 29, 2023

This PR supercedes #956 as it rebases to the newest wit package format. @alexcrichton PTAL thanks!

Notice that I don't find there is any use cases to the include ... with { ... } syntax anymore and thus changes it to include only.

Mossaka added 3 commits May 29, 2023 10:47
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think it would be good though to add some more tests for failure cases such as:

  • Use include on a foreign interface
  • Use include on a foreign world that doesn't exist
  • Have a top-level use for a foreign world
  • Have a top-level use for a foreign interface and then use the same identifier in an include

Things like that. One recommendation I'd have is to run a coverage report of the UI tests and ensure that all code added here is covered for all new error messages.

crates/wit-parser/src/ast/resolve.rs Outdated Show resolved Hide resolved
crates/wit-parser/src/ast/resolve.rs Outdated Show resolved Hide resolved
crates/wit-parser/src/lib.rs Outdated Show resolved Hide resolved
crates/wit-parser/src/resolve.rs Outdated Show resolved Hide resolved
crates/wit-parser/src/resolve.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member

Oh additionally if you're up for it it'd be great to add support for this to wit-smith to get this fuzzed.

Mossaka added 5 commits May 31, 2023 09:53
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
@Mossaka
Copy link
Member Author

Mossaka commented May 31, 2023

run a coverage report of the UI tests and ensure that all code added here is covered for all new error messages.

Do you have a command that can share with me on doing this?

Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
@alexcrichton
Copy link
Member

I always forget the flags myself so I read over this and follow it

Mossaka added 4 commits May 31, 2023 15:02
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
@Mossaka
Copy link
Member Author

Mossaka commented Jun 1, 2023

@alexcrichton I've resolved all the comments except the wit-smith and code coverage. I've figured out how to generate a code coverage report yesterday and will be looking into how much been covered already in these newly added code blocks.

I need some help on fuzz testing. I did some experiemntal implementation on wit-smith but I didn't really understand how to do fuzz tests through it. Could you please help me with it?

Other than that, PTAL!

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, and thanks again! Some minor comments below, but otherwise I think this is good to go. It's ok to cover fuzzing in a future PR.

crates/wit-parser/src/ast/resolve.rs Outdated Show resolved Hide resolved
fn resolve_include(&mut self, owner: TypeOwner, i: &ast::Include<'a>) -> Result<()> {
let (item, name, span) = self.resolve_ast_item_path(&i.from)?;
let include_from = self.extract_world_from_item(&item, &name, span)?;
self.foreign_world_spans.push(span);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not quite sure, but what is this for?

Copy link
Member Author

@Mossaka Mossaka Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This span is used at wit-parser::resolve::resolve_include Error handling

crates/wit-parser/src/resolve.rs Outdated Show resolved Hide resolved
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
@alexcrichton
Copy link
Member

Ok I'm going to go ahead and merge this and anything else can be done in follow-ups as necessary. Thanks again @Mossaka!

@alexcrichton alexcrichton merged commit 98d809f into bytecodealliance:main Jun 13, 2023
@Mossaka Mossaka deleted the union-wit-parser-2 branch June 13, 2023 16:22
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal. bytecodealliance#1045 bytecodealliance#1059
* Support for resources throughout most tooling. bytecodealliance#1053 bytecodealliance#1068 bytecodealliance#1070 bytecodealliance#1082
  bytecodealliance#1079 bytecodealliance#1083 bytecodealliance#1084 bytecodealliance#1105 bytecodealliance#1113 bytecodealliance#1116
* Support for `include` in WIT files. bytecodealliance#1054 bytecodealliance#1085 bytecodealliance#1088
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously. bytecodealliance#1081 bytecodealliance#1107
* Support for registry metadata in `wasm-tools metadata`. bytecodealliance#1060
* A new subcommand `wasm-tools component targets`. bytecodealliance#1089
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package. bytecodealliance#1108
* Miscellaneous bug fixes and improvements. bytecodealliance#1061 bytecodealliance#1065 bytecodealliance#1074 bytecodealliance#1073 bytecodealliance#1078 bytecodealliance#1077
  bytecodealliance#1072 bytecodealliance#1086 bytecodealliance#1091 bytecodealliance#1094 bytecodealliance#1114 bytecodealliance#1106
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal.
  [bytecodealliance#1045](bytecodealliance#1045)
  [bytecodealliance#1059](bytecodealliance#1059)
* Support for resources throughout most tooling.
  [bytecodealliance#1053](bytecodealliance#1053)
  [bytecodealliance#1068](bytecodealliance#1068)
  [bytecodealliance#1070](bytecodealliance#1070)
  [bytecodealliance#1082](bytecodealliance#1082)
  [bytecodealliance#1079](bytecodealliance#1079)
  [bytecodealliance#1083](bytecodealliance#1083)
  [bytecodealliance#1084](bytecodealliance#1084)
  [bytecodealliance#1105](bytecodealliance#1105)
  [bytecodealliance#1113](bytecodealliance#1113)
  [bytecodealliance#1116](bytecodealliance#1116)
* Support for `include` in WIT files.
  [bytecodealliance#1054](bytecodealliance#1054)
  [bytecodealliance#1085](bytecodealliance#1085)
  [bytecodealliance#1088](bytecodealliance#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [bytecodealliance#1081](bytecodealliance#1081)
  [bytecodealliance#1107](bytecodealliance#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [bytecodealliance#1060](bytecodealliance#1060)
* A new subcommand `wasm-tools component targets`.
  [bytecodealliance#1089](bytecodealliance#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [bytecodealliance#1108](bytecodealliance#1108)
* Miscellaneous bug fixes and improvements.
  [bytecodealliance#1061](bytecodealliance#1061)
  [bytecodealliance#1065](bytecodealliance#1065)
  [bytecodealliance#1074](bytecodealliance#1074)
  [bytecodealliance#1073](bytecodealliance#1073)
  [bytecodealliance#1078](bytecodealliance#1078)
  [bytecodealliance#1077](bytecodealliance#1077)
  [bytecodealliance#1072](bytecodealliance#1072)
  [bytecodealliance#1086](bytecodealliance#1086)
  [bytecodealliance#1091](bytecodealliance#1091)
  [bytecodealliance#1094](bytecodealliance#1094)
  [bytecodealliance#1114](bytecodealliance#1114)
  [bytecodealliance#1106](bytecodealliance#1106)
alexcrichton added a commit that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal.
  [#1045](#1045)
  [#1059](#1059)
* Support for resources throughout most tooling.
  [#1053](#1053)
  [#1068](#1068)
  [#1070](#1070)
  [#1082](#1082)
  [#1079](#1079)
  [#1083](#1083)
  [#1084](#1084)
  [#1105](#1105)
  [#1113](#1113)
  [#1116](#1116)
* Support for `include` in WIT files.
  [#1054](#1054)
  [#1085](#1085)
  [#1088](#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [#1081](#1081)
  [#1107](#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [#1060](#1060)
* A new subcommand `wasm-tools component targets`.
  [#1089](#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [#1108](#1108)
* Miscellaneous bug fixes and improvements.
  [#1061](#1061)
  [#1065](#1065)
  [#1074](#1074)
  [#1073](#1073)
  [#1078](#1078)
  [#1077](#1077)
  [#1072](#1072)
  [#1086](#1086)
  [#1091](#1091)
  [#1094](#1094)
  [#1114](#1114)
  [#1106](#1106)
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 this pull request may close these issues.

None yet

2 participants