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>
elliottt commented on issue #4562:
It might be better to implement full shadowing instead, which would resolve #3526.
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:
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 thelet
after we process thelet
's body. I suspect all we need to do is to remove the "am I shadowing another binding" check (line 1954 in the originalsema.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: Dec 23 2024 at 12:05 UTC