Support multiple packages with deps that have the same versions

[jsturtevant]

I mis-configured wit-bingen macro and got the error message:

proc macro panicked
message: assertion failed: prev.is_none()

When trying to debug it I tried using cargo expand and cargo docs to track down what I did wrong but the gave the same error.

After some digging and forking the various repositories, I eventually tracked the error message to this repo which has a bunch of asserts: https://github.com/search?q=repo%3Abytecodealliance%2Fwasm-tools%20%20prev.is_none&type=code

As a developer it would be nice to have a better error message for these asserts to improve the debugging experience.

[alexcrichton]

In theory asserts should never happen and they're only intended for developers of the tool itself, any other error should be signaled through a Result or some sort of other first-class mechanism. In that sense I think the "true bug" here lies in whatever configuration you fed to wit-bindgen and it panicked. The macro itself should never panic ideally.

Although naturally internal assertions do trip (e.g. this) and I don't disagree that having better error messages would be helpful. That being said though I wouldn't want to improve things to expect all users to see them because the true bug is having this trip at all.

[jsturtevant]

The mis-configuration came from using the macro in a way that was intended for testing purposes only (https://docs.rs/wit-bindgen/latest/wit_bindgen/macro.generate.html#options-to-generate)

// Usually used in testing, our test suite may want to generate code
// from wit files located in multiple paths within a single mod, and we
// don't want to copy these files again.

The configuration was bringing in two references to the wasi-io:

wit_bindgen::generate!({
    inline: "
        package wasmtime:test;

        world test {
            include wasi:cli/[email protected];
            include wasi:http/[email protected];
            include wasi:config/[email protected];
            include wasi:keyvalue/[email protected];
            include wasi:tls/[email protected];  #added line
        }
    ",
    path: [
        "../wasi-http/wit", // this also has a deps folder to io
        "../wasi-config/wit",
        "../wasi-keyvalue/wit",
        "../wasi-tls/wit/",  // this has a deps folder to io
    ],
    world: "wasmtime:test/test",
    features: ["cli-exit-with-code", "tls"],
    generate_all,
});

The two wasi-io deps collided with the error "prev.is_none()`. Technically I guess this is an error and not an assert?

[alexcrichton]

Oh dear that looks like something that should be expected to work. That documentation is a bit outdated and the array-of-paths situation is a bit more expected to work nowadays as well.

Would you be able to make a reproduction? If it's still using wit-bindgen that's ok, I can work on reducing it as a reproduction for this repository.

[jsturtevant]

I believe this is a simplified reproduction: https://github.com/jsturtevant/wasi-cli/tree/prev-is-none

I am getting:

cargo build 
  Compiling wit-bindgen-rust-macro v0.39.0
   Compiling wit-bindgen v0.39.0
   Compiling path-repo v0.1.0 (/home/jstur/projects/wit/wasi-cli)
error: proc macro panicked
  --> src/main.rs:2:5
   |
2  | /     wit_bindgen::generate!({
3  | |         inline: "
4  | |             package wasmtime:test;
...  |
17 | |         generate_all,
18 | |     });
   | |______^
   |
   = help: message: assertion failed: prev.is_none()

I wasn't able to figure out how to load multiple deps via just the wasm-tools component wit command

[alexcrichton]

Ok I got a chance to debug and look into this and the best I could come up with is promoting the assertion to an error. Actually fixing this would require a relatively large refactoring/investment which I don't have time for. I'd also like to eventually support this on the CLI but that'll also take some refactoring and care which isn't trivial.

Definitely file more issues though if other internal assertions crop up!

[jsturtevant]

Should we keep this open? I can rename it to reflect the error.

[alexcrichton]

Oh sure yeah, sounds good