Impl imports #1146

macovedj · 2023-07-26T20:13:56Z

This PR can serve as an implementation of what is proposed here.

I noticed that currently in bytecodealliance#1146 the CI there is green but some files are unformatted which got me wondering why that was the case. Turns out that `cargo fmt` won't find files that are referenced through macro-expanded macros, meaning that most of `wast` isn't actually checked for formatting on CI. This commit fixes that.

alexcrichton

Thanks! I think it would be good to add a suite of tests for this too. Adding files to tests/local/component-model/*.wast should be good enough. It'll be good to exercise the text parsing, binary encoding, binary validation, and text printing for all of this.

crates/wast/src/lib.rs

crates/wast/src/component/import.rs

crates/wasm-encoder/src/component/imports.rs

crates/wasmparser/src/validator/names.rs

crates/wast/src/component/import.rs

alexcrichton · 2023-08-21T21:07:41Z

Oh one other comment I'd recommend is to configure your text editor to rustfmt-on-save since there's some un-formatted files here which will get flagged on CI once #1174 merges

I noticed that currently in #1146 the CI there is green but some files are unformatted which got me wondering why that was the case. Turns out that `cargo fmt` won't find files that are referenced through macro-expanded macros, meaning that most of `wast` isn't actually checked for formatting on CI. This commit fixes that.

…truct

crates/wasm-encoder/src/component/imports.rs

alexcrichton · 2023-09-20T15:01:18Z

crates/wasm-encoder/src/component/imports.rs

-pub trait AsComponentExternName {
-    /// Converts this receiver into a `ComponentExternName`.
-    fn as_component_extern_name(&self) -> ComponentExternName<'_>;
+pub trait AsComponentImportName {


This made sense beforehand since the kebab/interface distinction was easy to differentiate between. Now though it's slightly more complicated where a single-string as an import could either be Kebab, Interface, or Unlocked. While / or : means it's definitely not Kebab I don't believe there's any means, in isolation, to tell the difference between Unlocked and Interface. For example a:b/c could be either I believe.

In that sense I think this trait should be removed and consumers relying on it are going to have to use the more verbose form to select which import form they intended to use.

crates/wasmparser/src/readers/component/imports.rs

crates/wast/src/component/import.rs

alexcrichton · 2023-09-20T16:14:14Z

crates/wasmparser/src/validator/names.rs

+    pub fn from_export(name: ComponentExportName<'_>, offset: usize) -> Result<KebabName> {
+        let validate_kebab = |s: &str| {
+            if KebabStr::new(s).is_none() {
+                bail!(offset, "`{s}` is not in kebab case")
+            } else {
+                Ok(())
+            }
+        };
+        let find = |s: &str, c: char| match s.find(c) {
+            Some(i) => Ok(i),
+            None => bail!(offset, "failed to find `{c}` character"),
+        };
+
+        let parsed = match name {
+            ComponentExportName::Kebab(s) => {
+                if let Some(s) = s.strip_prefix(CONSTRUCTOR) {
+                    validate_kebab(s)?;
+                    ParsedKebabName::Constructor
+                } else if let Some(s) = s.strip_prefix(METHOD) {
+                    let dot = find(s, '.')?;
+                    validate_kebab(&s[..dot])?;
+                    validate_kebab(&s[dot + 1..])?;
+                    ParsedKebabName::Method { dot: dot as u32 }
+                } else if let Some(s) = s.strip_prefix(STATIC) {
+                    let dot = find(s, '.')?;
+                    validate_kebab(&s[..dot])?;
+                    validate_kebab(&s[dot + 1..])?;
+                    ParsedKebabName::Static { dot: dot as u32 }
+                } else {
+                    validate_kebab(s)?;
+                    ParsedKebabName::Normal
+                }
+            }
+            ComponentExportName::Interface(s) => {
+                let colon = find(s, ':')?;
+                validate_kebab(&s[..colon])?;
+                let slash = find(s, '/')?;
+                let at = s[slash..].find('@').map(|i| i + slash);
+                validate_kebab(&s[colon + 1..slash])?;
+                validate_kebab(&s[slash + 1..at.unwrap_or(s.len())])?;
+                if let Some(at) = at {
+                    let version = &s[at + 1..];
+                    let version = version.parse::<Version>();
+                    match version {
+                        Ok(ver) => ParsedKebabName::RegistryId {
+                            colon: colon as u32,
+                            slash: Some(slash as u32),
+                            at: Some(At {
+                                index: at as u32,
+                                semver: Semver::Semver(ver),
+                            }),
+                        },
+                        Err(e) => {
+                            bail!(offset, "failed to parse version: {e}")
+                        }
+                    }
+                } else {
+                    ParsedKebabName::RegistryId {
+                        colon: colon as u32,
+                        slash: Some(slash as u32),
+                        at: None,
+                    }
+                }
+            }
+        };
+        Ok(KebabName {
+            raw: name.as_str().to_string(),
+            parsed,
+        })
+    }


I believe this method is largely duplicated with the one below, but could the duplication be removed?

alexcrichton · 2023-09-20T16:15:41Z

crates/wasmparser/src/validator/names.rs

        package: &'a KebabStr,
-        interface: &'a KebabStr,
-        version: Option<Version>,
+        interface: Option<&'a KebabStr>,


To keep things somewhat simpler for now could this be left as &'a KebabStr and effectively only a:b/c works today?

The primary motivator here was that when actually using the new implementation imports, there is no / generally. For example, unlocked imports tend to look something like

(import (unlocked-dep "foo:five@{>=2.0.0 <3.0.0}") (instance (;0;) (type 1)))

It definitely would be nice to simplify if possible, as I added a lot of checks elsewhere to enable the optionality here, but I'm not sure the best way to go about that while simultaneously supporting this use case.

alexcrichton · 2023-09-20T16:16:27Z

crates/wasmparser/src/validator/names.rs

+            ComponentImportName::Unlocked(name) => {
+                let colon = find(name, ':')?;
+                validate_kebab(&name[..colon])?;
+                let slash = maybe_find(name, '/');
+                if let Some(sl) = slash {
+                    let at = name[sl..].find('@').map(|i| i + sl);
+                    validate_kebab(&name[colon + 1..sl])?;
+                    if let Some(at) = at {
+                        validate_kebab(&name[sl + 1..at])?;
+                        let version_string = validate_version_range(&name[at + 1..])?;
+                        ParsedKebabName::RegistryId {
+                            colon: colon as u32,
+                            slash: Some(sl as u32),
+                            at: Some(At {
+                                index: at as u32,
+                                semver: Semver::SemverRange(version_string),
+                            }),
+                        }
+                    } else {
+                        ParsedKebabName::RegistryId {
+                            colon: colon as u32,
+                            slash: Some(sl as u32),
+                            at: None,
+                        }
+                    }
+                } else {
+                    let at = name[colon..].find('@').map(|i| i + colon);
+                    if let Some(at) = at {
+                        validate_kebab(&name[colon + 1..at])?;
+                        let version_string = validate_version_range(&name[at + 1..])?;
+                        ParsedKebabName::RegistryId {
+                            colon: colon as u32,
+                            slash: None,
+                            at: Some(At {
+                                index: at as u32,
+                                semver: Semver::SemverRange(version_string),
+                            }),
+                        }
+                    } else {
+                        ParsedKebabName::RegistryId {
+                            colon: colon as u32,
+                            slash: None,
+                            at: None,
+                        }
+                    }
                }


I believe this is the more-or-less the same parsing code as Locked, so could those be deduplicated?

alexcrichton · 2023-09-20T16:18:17Z

crates/wasmparser/src/validator/names.rs

+impl PartialEq for Semver {
+    fn eq(&self, other: &Semver) -> bool {
+        match self {
+            Self::Semver(version) => match other {
+                Semver::Semver(other_version) => version == other_version,
+                Semver::SemverRange(_) => false,
+            },
+            Self::SemverRange(version_range) => match other {
+                Semver::Semver(_) => false,
+                Semver::SemverRange(other_range) => version_range == other_range,
+            },
+        }
+    }
+}


I think this can be replaced with #[derive(PartialEq)] on the type?

alexcrichton · 2023-09-20T16:20:08Z

If you'd like I don't mind taking it from here and landing this as well. Implementing these sorts of changes can be pretty tricky so I don't want to put too much on you by accident

* Fold the `Naked` case into the preexisting `Kebab` case with a new payload. * Parse `integrity` with surrounding parens * Make the `integrity` field on `Locked` optional

alexcrichton · 2023-09-28T14:48:46Z

If it's ok with you @macovedj I'd prefer to hold off on this until WebAssembly/component-model#253 is resolved since that will change the binary and textual syntax of imports again. Additionally it will likely change the internals of representation within wasm-tools crates, so I think it would be best to handle that all at once rather than having intermediate states to deal with.

macovedj · 2023-09-28T14:56:40Z

If it's ok with you @macovedj I'd prefer to hold off on this until WebAssembly/component-model#253 is resolved since that will change the binary and textual syntax of imports again. Additionally it will likely change the internals of representation within wasm-tools crates, so I think it would be best to handle that all at once rather than having intermediate states to deal with.

Makes sense to me!

alexcrichton · 2023-10-24T23:50:22Z

I've opened up #1262 which is intended to be the successor to this PR which catches up with other component model developments as well

macovedj added 7 commits July 7, 2023 16:41

impl import

06bdb32

Merge branch 'main' into impl-imports

243755f

generalize to include url

f72be91

registry imports

25b25dd

fmt

54d8f63

pass tests

21aea66

integrity and cleanup

6bef0f7

macovedj force-pushed the impl-imports branch from 0e4ac34 to ce7aaeb Compare August 3, 2023 16:55

macovedj force-pushed the impl-imports branch from ce7aaeb to 8bc5969 Compare August 16, 2023 17:38

This was referenced Aug 16, 2023

Lock components and bundle components #1166

Closed

Implementation Imports #1168

Closed

macovedj force-pushed the impl-imports branch from 8bc5969 to 0854363 Compare August 17, 2023 16:01

Merge branch 'main' into impl-imports

d9ee05c

macovedj force-pushed the impl-imports branch from 0854363 to d9ee05c Compare August 17, 2023 16:32

macovedj added 2 commits August 17, 2023 13:24

updated syntax

778e31e

naked imports

c7cc01b

macovedj force-pushed the impl-imports branch from 5a60a32 to c7cc01b Compare August 17, 2023 20:46

impl imports not kebab id

dfbff67

macovedj force-pushed the impl-imports branch from 01eef38 to dfbff67 Compare August 18, 2023 14:22

validate impl import names

53bed21

macovedj marked this pull request as ready for review August 18, 2023 16:44

alexcrichton mentioned this pull request Aug 21, 2023

Really check rustfmt on all files in CI #1174

Merged

alexcrichton reviewed Aug 21, 2023

View reviewed changes

macovedj added 2 commits August 22, 2023 11:38

split externname into import and exportname and flattin impl import s…

73ee5cd

…truct

Merge branch 'main' into impl-imports

06ef6fc

macovedj force-pushed the impl-imports branch 2 times, most recently from e932d23 to a10792e Compare August 22, 2023 20:46

macovedj force-pushed the impl-imports branch from a10792e to 06b439f Compare August 23, 2023 15:28

tests and validate impl import names

5d3226f

macovedj force-pushed the impl-imports branch from 06b439f to 5d3226f Compare August 23, 2023 15:40

Merge branch 'main' into impl-imports

298aeef

macovedj force-pushed the impl-imports branch from ff75590 to 298aeef Compare September 11, 2023 16:32

use RegistryId for interfaces

c71706f

alexcrichton reviewed Sep 20, 2023

View reviewed changes

alexcrichton added 7 commits September 25, 2023 09:24

Use Foo(...) instead of Foo((...))

dc5f5c2

Fix encoding/parsing of optional integrity

1235e90

Touch up component import parsing

181575e

Update manifest syntax

0911024

Remove dead code

f4ad6a3

Remove some more dead code

9dd935e

Remove a stray comment

fb351ce

alexcrichton mentioned this pull request Sep 25, 2023

Inferring import kind from string syntax WebAssembly/component-model#253

Closed

Reorganize parsing of new imports

6b6047a

* Fold the `Naked` case into the preexisting `Kebab` case with a new payload. * Parse `integrity` with surrounding parens * Make the `integrity` field on `Locked` optional

This was referenced Sep 26, 2023

wasm-tools: implementation of imports naming implementations (component-model #222) #1198

Closed

Clarify what's in Preview 2 WebAssembly/component-model#255

Merged

alexcrichton mentioned this pull request Oct 24, 2023

Remove "interface imports" from binary syntax, expand accepted strings #1262

Merged

macovedj closed this Oct 30, 2023

Impl imports #1146

Impl imports #1146

Uh oh!

Conversation

macovedj commented Jul 26, 2023

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexcrichton commented Aug 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexcrichton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexcrichton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

alexcrichton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

macovedj Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

alexcrichton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

alexcrichton Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Sep 20, 2023

Uh oh!

alexcrichton commented Sep 28, 2023

Uh oh!

macovedj commented Sep 28, 2023

Uh oh!

alexcrichton commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants