Stream: git-wasmtime

Topic: wasmtime / issue #4562 Allow multiple `_` definitions in ...

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:37):

github-actions[bot] commented on issue #4562:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 01 2022 at 16:24):

elliottt commented on issue #4562:

It might be better to implement full shadowing instead, which would resolve #3526.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:25):

elliottt commented on issue #4562:

@cfallin I went the route of implementing shadowing instead, as it seems like a more complete solution to the problem. To resolve the issue of restoring the previous bindings, I turned the TypeEnv::sym_map field into a vector of maps, ensuring that the vector is never empty. I'd be happy to replace the vector of maps with a more efficient implementation :+1:

Wasmtime GitHub notifications bot (Aug 01 2022 at 20:01):

elliottt commented on issue #4562:

Thanks for pushing further and doing full shadowing!

However I think there may be a simpler way to do this. I didn't expect the symbol-interning itself to become aware of nested scopes, and in fact that could (?) lead to unexpected behavior later if we remove a binding. Basically I had meant for that to be an interning layer so that we could use Sym indices everywhere as a substitute for string identifiers.

Instead I had expected bindings to handle the scope entry and exit. It was actually structured originally with the intent of supporting shadowing some day: it remains an ordered list of bindings (in order of introduction). We truncate away all bindings within the let after we process the let's body. I suspect all we need to do is to remove the "am I shadowing another binding" check (line 1954 in the original sema.rs), and ensure that the search for a binding occurs as a reverse search from the end (I _believe_ it already might). Does that make sense?

This makes sense, and is indeed a much simpler approach. The var lookup already searches backwards through the bindings, so this really might be as simple as removing the check for duplicate bindings.

Last updated: Oct 23 2024 at 20:03 UTC