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

quoted js* expressions substitution #33

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bendlas
Copy link

@bendlas bendlas commented Dec 12, 2024

As discovered when trying to compose the helix react wrapper with missionary, cloroutine SSA transformation can introduce substitution errors, when processing js* forms in cljs.

That's because js* has fexpr-like semantics, where (js) AST snippets are substituted into the js source verbatim. For expressions that are quoted by javascript, this can make local variable names show up in javascript values:

(not=
  (run (cr {} (js* "'~{}'" "Result")) "\"Result\"")
  "\"Result\"" "cr28374_place_0")

(not=
  (run (cr {} (js->clj (js-obj "key" "val"))) {:key "val"})
  {:key "val"} {"cr28381_place_1" "val"})

(note that js-obj has a compiler macro, that expands to (js* "({~{}:~{}})" "key" "val"), in which then the key expression is mangled by cr)

Analysis

I believe just skipping introduction of local variables for :op :const AST nodes during SSA (maybe even just when used as js* argument?) should fix most any case that we care about: For anything but a constant, relying on the quoted JS form would be very brittle and probably an error.

I've added two tests, one for strings and one for object keys. There may be other affected edge cases when building up JS syntax, e.g. for labelled statements, but since cr necessarily needs to work without knowledge whether an expression will be quoted by JS, fixing :op :const nodes should fix all cases that can be reasonably fixed.

Things done

For now, I've just added tests, since I'm not that familiar with cr's SSA transformer. Help is welcome and I may reach out on slack.

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.

1 participant