Skip to content

Conversation

@Mossaka
Copy link
Contributor

@Mossaka Mossaka commented Mar 3, 2023

As proposed by #169 , this PR adds an include keyword to the WIT.md. It also sorts the keyword list alphabetically.

Copy link
Collaborator

@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.

Could you expand a bit about the semantic meaning of the include statement? For example when with is required, what with does, when include might fail, etc.

@Mossaka
Copy link
Contributor Author

Mossaka commented Mar 6, 2023

Could you expand a bit about the semantic meaning of the include statement? For example when with is required, what with does, when include might fail, etc.

Good suggestion. I have thought about that but then I saw the section I've modified is for Lexical Structure. Is there a dedicated section for semantic structure of WIT? If not, I can expand the the semantic meaning in the part where I modified. @alexcrichton

@alexcrichton
Copy link
Collaborator

The top of the document is probably the best place for that right now, probably around the area that worlds are explained.

@Mossaka Mossaka requested a review from alexcrichton March 7, 2023 00:36
Copy link
Collaborator

@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, but I might recommend trying to run some examples through wit-parser to sort out self/pkg paths since right now they're relatively imprecise and can lead to some ambiguities in the examples I think. I realize that include isn't implemented yet but I think most other pieces should be able to be fleshed out to make sure they're valid.

As I think about this as well though the "de-duplication" section I think will actually need to be required for the first implementation. It's not valid to import the same interface twice under two names, so if the same interface is imported into two worlds which are union'd then it's required that a deduplication happens rather than emitting two separate imports (e.g. resolving with with isn't valid)

@Mossaka
Copy link
Contributor Author

Mossaka commented Mar 8, 2023

so if the same interface is imported into two worlds which are union'd then it's required that a deduplication happens rather than emitting two separate imports (e.g. resolving with with isn't valid)

That's a really good point. Transient deps are always hard haha. I will update the doc to include de-dep semantics.

@danbugs
Copy link

danbugs commented Mar 9, 2023

~one way to make this less verbose than:

world union-my-world {
    include self.my-world-1
    include self.my-world-2
}

... could be to change the world word itself – something like universe might be fitting:

universe union-my-world {
     self.my-world-1,
     self.my-world-2
}

Of course, this would mostly just be syntactical sugar – the union of worlds should still equate to a world.

@Mossaka Mossaka requested a review from alexcrichton March 9, 2023 23:42
@Mossaka
Copy link
Contributor Author

Mossaka commented May 4, 2023

@alexcrichton thanks for reviewing. Could you please take another look?

Copy link
Collaborator

@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.

Looks good to me, thanks!

Mossaka added 5 commits June 13, 2023 17:44
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
@Mossaka
Copy link
Contributor Author

Mossaka commented Jun 14, 2023

I will rebase once #198 is merged in.

Mossaka added 2 commits June 22, 2023 17:12
This commit updates the WIT syntax in all of the examples illustrating
the use of the include syntax.

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

Mossaka commented Jun 23, 2023

Okay, I think this PR is ready to review @guybedford

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

It would be great to see this moving forward, thanks for the update.

Mossaka and others added 2 commits June 22, 2023 17:54
Co-authored-by: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@danbugs danbugs left a comment

Choose a reason for hiding this comment

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

Awesome work, Joe!

Mossaka and others added 3 commits June 23, 2023 10:36
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, and great work. Regarding the most recent change, which says that you can't rename an ID, unfortunately we can't just rule it out syntactically, because include x with { a as b } may or may not be allowed depending on x. Here's a suggested rewording, but feel free to reject and do it your own way.

Mossaka and others added 2 commits June 23, 2023 13:14
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
@Mossaka
Copy link
Contributor Author

Mossaka commented Jun 23, 2023

Thank you, @lukewagner !! I have committed your suggestions.

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Thanks Joe, overall lgtm, thanks for spearheading this! I'm thinking I'll merge early next week if there aren't any more comments from anyone.

@Mossaka
Copy link
Contributor Author

Mossaka commented Jun 23, 2023

Thanks everyone for reviewing this!! This meant a lot to me as it marks the first contribution I've contributed to a Wasm proposal. Cheers 🙌 🥂

@lukewagner lukewagner merged commit 3a1d715 into WebAssembly:main Jun 26, 2023
@Mossaka Mossaka deleted the union branch June 26, 2023 23:42
@cardoso cardoso mentioned this pull request Sep 11, 2023
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.

5 participants