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

Remove implicit import behavior of 'use' #402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

lukewagner
Copy link
Member

Currently, this world is valid:

interface a {
  resource r;
}
interface b {
  use a.{r};
  foo: func() -> r;
}
world w {
  import b;
}

and implicitly contains an import of a due to the use of a inside b. This causes some confusion, especially in the case of exports (which also implicitly add imports for uses in exported interfaces, which is often what you actually want, but it's surprising when it's not).

This PR removes this implicit-import behavior and instead simply requires a used interface name to have already been imported or exported. So, e.g., to be valid, w would have to be rewritten:

world w {
  import a;
  import b;
}

A concrete consequence of this change is that wasi:http/imports would require the addition of import types;.

This change would complement #308 in helping to clarify and better support advanced (virtualization) cases where a world imports and exports the same interface. This PR contains a NOTE about the remaining ambiguous case that requires #308 to address.

To help P2 WITs transition, perhaps the current WIT tooling could maintain the current implicit-import behavior, but issue a deprecation warning and then at some later date we could turn the warning into an error.

Thoughts?

@tschneidereit
Copy link
Member

tschneidereit commented Oct 1, 2024

I'm a bit concerned about this creating a lot more complexity for toolchains in the long run. With the current behavior, exposing an interface as part of a world is simple, with the complex parts being handled in a single place, and not being a concern for most kinds of tools.

With this change, all tools that want to emit worlds now have to understand all the dependencies, and all the details of how those are handled. And, if I'm not missing something, all that to finally get to a result that is exactly the same as what would happen with the current behavior: you can't use interface b without also pulling in its dependencies, after all.

@lukewagner
Copy link
Member Author

What we see in practice today is that WIT packages tend to define little worlds that have "all the stuff" and get included by bigger worlds, which means that most cases of assembling worlds in pieces won't be affected. In the more ad hoc cases, most tools shouldn't need to do a complex dependency analysis (that would reintroduce the automatic behavior that's causing the confusion); tools can just pass the imports through and WIT validation can fail with a validation error that suggests the missing interfaces to import or export. I imagine only special producer tools like a dep-solver might need to care about this, but I think it would need to care about transitive dependencies in any case.

@tschneidereit
Copy link
Member

tools can just pass the imports through and WIT validation can fail with a validation error that suggests the missing interfaces to import or export.

That is exactly the scenario that worries me: forcing developers to fill in information manually that we can, and currently do, derive automatically. I could easily see reactions along the lines of "all I wanted to do is make use of this function—why does that require more than use-ing the function itself?"

I guess another question to ask: is there precedent for this kind of behavior? In the context of programming languages it seems unusual, but maybe it's more established for IDLs?

@lann
Copy link
Contributor

lann commented Oct 2, 2024

A smaller change that would still address most of the confusion would be to require explicit imports only where an exported interface depends on that imported interface. Or even incrementaler: only require the explicit import if an export depends on it and it is not already transitively imported.

@lukewagner
Copy link
Member Author

@lann That's not a bad idea, and perhaps at the very least we should do something like that.

That being said in revisiting this old design choice recently based on @lann's and others helpful feedback, there are a few new reasons that have come to mind for why EIBTI when it comes to worlds:

  • worlds are mostly read and often (and increasingly) generated. When reading a world, seeing the complete set of imported/exported interfaces is definitely beneficial. If it led to 100s of lines of verbosity, that'd be bad of course, but we fortunately have interfaces to group functions into logical units, so I think we'd still end up with the "right" level of explicitness that doesn't feel like verbosity.
  • include seems to be used in practice as the mechanism for pulling in coherent collections of imports+exports (for the cases where I'm manually authoring or generating WIT), and removing implicit-imports doesn't affect include.
  • worlds seem like a potentially excellent artifact to audit as part of an overall security process in an organization. For this use case, I worry that implicit imports add unnecessary hazards. We're still in the early days so the depth of use dependencies in practice is still rather shallow so maybe it doesn't feel hazardous now, but you could imagine Serious Enterprise(TM) WITs where the transitive implications of a single visible import is far less obvious.

@alexcrichton
Copy link
Collaborator

Personally I'd lean towards @lann's suggestion. I feel that strikes a good balance between authorship, understandability, and readability. For fully elaborating a world it's pretty trivial to do with a tool and what I would hope is that for the cases of auditing or documentation or similar that it would be reasonable to assume a tool is between the reader and the WIT itself. For example online registries could fully elaborate all worlds and locally auditing could be based on the output of wasm-tools component wit (or something like that)

@lukewagner
Copy link
Member Author

Thinking about @lann proposal again, I worry that it introduces a new set of rules that WIT authors need so that they can understand the corner cases where validation fails (e.g., "why do I have to add an explicit import in this case and not other cases?"). In contrast, with the always-explicit rule, the rule is trivial to state and fix when you encounter it (the validation error message can tell the whole story).

Again, given how WIT-based workflows are developing, it doesn't seem like we need to be optimizing for speed of manual authoring of WIT vs. reading of WIT, the latter of which I think very much benefits from seeing all imports laid out plainly (without needing to run a special tool to elaborate). I may be wrong, but removing implicit imports is also a conservative choice, one that we could later relax. And because WIT worlds could end up serving a useful security-related purpose, I think it's important that we take this seriously.

@ydnar
Copy link

ydnar commented Oct 8, 2024

Having been bit by this a couple of times, I’m sympathetic to both sides of the argument for and against implicit imported types. I appreciate the discussion here.

A smaller change that would still address most of the confusion would be to require explicit imports only where an exported interface depends on that imported interface. Or even incrementaler: only require the explicit import if an export depends on it and it is not already transitively imported.

Expanding on @lann’s comment—what if we lean harder into implicit imports, and:

  1. explicitly declare that "import" is the default, and document how and why it works this way
  2. warn if an exported interface or function depends on an implicitly imported interface or type
  3. add a flag to tooling like -Werror that treats this warning as an error

Related to this (the general topic of WIT not being able to fully express concepts expressible in WAT), I think we should upstream the imported/exported direction of an interface, function, and type into the wit-parser crate: bytecodealliance/wasm-tools#1497

@alexcrichton
Copy link
Collaborator

For me the high-order bit here is solving the problem of today it's relatively easy to write something that looks like it works when in fact it means something different and doesn't work. Completely solving this I think will require use import ... and use export ... but that's considered "too big" of a change right now so this is the next-best stopgap we have. I don't disagree that most users are reading worlds rather than writing worlds, but at the same time I don't think we should necessarily penalize one over the other. For example there's quite a lot of tests and things today that are going to require updates if we require explicit imports I think and I wouldn't personally look forward to that.

I think it's also worth considering what we want the "final end state" to look like and if that's use import and use export then I personally think it would be reasonable to never require explicitly listing imports/exports and instead let them get inferred (just like imports through interfaces). If this is the case then I don't think we'd want to shift the lever to "maximally specify all imports" just to switch the lever back to "ok now you maximally don't have to specify transitive dependencies" if/when use import is implemented.

Overall what I would expect from a change like this is no one will really remember the exact rules here and will instead let themselves be guided by tooling. In that sense the error message for a missing import is going to be required to basically generate something suitable for copy/paste into a world.

If we want to change the balance of readability/writability of world by requiring imports all be listed I think that might be best to decouple from solving the confusion here of using a type from an import by accident. @lukewagner you mention that explicit imports is the conservative choice and while in the abstract I agree with this we're also not currently in such a situation because we accept implicit imports today. In that sense it's far more conservative to continue to allow that and just add a minor error for this edge case rather than risking breaking the majority of worlds in the wild today. The more worlds that break from a change like this the more we'll have to invest in infrastructure to have a smooth transition between the two specifications.


@ydnar one of the implicit assumptions I think you're making is that we have a robust system in place to deliver such warnings to users. Currently though the WIT tooling is very spread out across tooling ecosystems and we didn't originally rely on such a warning system being in place so I think it would be quite difficult to retroactively add that. For example we could pretty easily handle this via the wit-bindgen CLI tool but that wouldn't handle the wit-bindgen crate and its generate! macro. In the Rust case there's not actually any way to programmatically emit a warning from a macro so I'm not sure how in such a situation warnings would be delivered to the end user.

Overall I like the idea of doing something with a warning that users can opt-in to being an error, but I don't think we're at a spot with the tooling and integration of WIT to make such a warning feasible. I do think there's a case to be made to develop such infrastructure/frameworks to make changes like this easier in the future, but that's also further beyond the scope of just the change here unfortunately.

@ydnar
Copy link

ydnar commented Oct 22, 2024

@alexcrichton thanks for the feedback. In your mind, what is the concrete proposal now?

FWIW, I was thinking about having the warning logic live in wasm-tools, perhaps in the wit-parser crate, which would transitively be available in wit-bindgen and friends, exposing a broad audience to the change.

@alexcrichton
Copy link
Collaborator

My thinking of what the proposal would be hasn't changed, it's the same as what was mentioned above:

require explicit imports only where an exported interface depends on that imported interface

which is targeted at solving the confusing nature of this world:

interface a {
  resource r;
}

world w {
  export a;
  use a.{r};
  foo: func() -> r;
}

The world w here looks like it's exporting a and then the foo function refers to a. In fact the use a.{r} here is injecting import a; which can be surprising. This explicit export of foo depends on an interface which is not explicitly imported (e.g. import a;), so this world which is accepted today would become an error tomorrow.

FWIW, I was thinking about having the warning logic live in wasm-tools, perhaps in the wit-parser crate, which would transitively be available in wit-bindgen and friends, exposing a broad audience to the change.

Agreed! My point is that this is not sufficient. The wit-parser crate is used in so many places it's not easy to plumb warnings through every single one. Basically despite wit-parser being central to parsing WIT it's not well-defined how to have a side-channel to emit warnings since there's not just a single tool using wit-parser. Some examples are:

  • Above I mentioned the wit_bindgen::generate! macro in Rust. There's no way for that to generate a warning.
  • Another would be wasm-tools component wit --json, where would that put warnings? Emitting wouldn't be great since that's an internal tool and otherwise this would have to be a new JSON field for example.
  • All wasm-tools commands would need to be updated along with wit-bindgen to emit warnings, as an example of multiple places to update.

I'm by no means saying this can't be done, I'm instead pointing out "just add it to wit-parser" is only half the battle and the next half of "integrate that in all the tooling" is likely going to be a significant chunk of effort. Basically it's not for-free by just adding to wit-parser.

@lukewagner
Copy link
Member Author

Sorry for the slow reply! (#405 stole all my mental cycles and this required a bit more thought.)

Ok, so even though I'm still rather uncomfortable with implicit imports, I do appreciate that, given all the other things in flight, it's probably a good idea to come up with a minimally-impacting fix in the short-term that addresses the flagrant sources of confusion.

As for what the precise rule is, I actually prefer @lann's "even more incrementaler" option:

only require the explicit import if an export depends on it and it is not already transitively imported.

Another way to summarize this option would be: exports never inject implicit imports (only imports can). And the simple rationale is that with exports there's an ambiguity (between injecting an import or export), but with imports there's no such ambiguity. A familiar example that is allowed today and would be continued to be allowed with this more-permissive option is:

interface handler {
  use types.{request, response, error-code};
  handle: func(request) -> result<response, error-code>;
}
world proxy {
  import handler;
  export handler;
}

With the @lann's first option that @alexcrichton quoted above, I think this world would fail validation because handler uses types which is not explicitly imported and thus the change would break popular worlds out there, which it seems like our goal is to avoid.

The world w in @alexcrichton's example would however fail validation b/c there are no imports. But because that example is so surprising (I would've guessed use a resolved to the export a), until we implement the "real" fix (use import/use export), I propose we also reject this world in the interim:

interface a { resource r; }
world w {
  import a;
  export a;
  use a.{r};
  foo: func() -> r;
}

as "ambiguous" (an ambiguity that is only resolvable once we add use import/use export). The general rule would be use a at world-level fails if a is both imported and exported.

How do those two rules sound? Happy to update the PR if that sounds good.

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