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

compile Racket symbols to js Symbol values? #246

Closed
stchang opened this issue Jul 22, 2021 · 5 comments · Fixed by #250
Closed

compile Racket symbols to js Symbol values? #246

stchang opened this issue Jul 22, 2021 · 5 comments · Fixed by #250

Comments

@stchang
Copy link
Member

stchang commented Jul 22, 2021

js has Symbol values now: https://developer.mozilla.org/en-US/docs/Glossary/Symbol

Would it be appropriate to compile Racket symbols to these js values?

(@samth Do you know if these would be compatible with Racket symbols, eg interning?)

@samth
Copy link
Collaborator

samth commented Jul 22, 2021

Basically, yes. Symbol.for creates an interned symbol. Symbol("foo") is uninterned. Symbol() is basically gensym.

@stchang
Copy link
Member Author

stchang commented Jul 22, 2021

Oh so it should be straightforward then and we can get rid of our Symbol class, nice.

@DarrenN
Copy link
Contributor

DarrenN commented Jul 24, 2021

I'm working my way through the source to understand how the AST is compiled to the IL and then to JS, so please bear with me. It looks like this is where symbol?s are tagged(?) to call the core.Symbol class: https://github.com/racketscript/racketscript/blob/master/racketscript-compiler/racketscript/compiler/transform.rkt#L617

Am I correct in thinking that instead we would remove that and allow symbol? to fall through to line 661 to become an ILValue and we would later use native Symbols instead?

Thinking in assemble-value we could do something like:

[(symbol? v)
     (emit "Symbol(~a)" v)]

@vishesh
Copy link
Member

vishesh commented Jul 24, 2021

Hey, thanks for taking this. Yes, that sounds right. You can also probably just replace the part where we do (ILApp 'core.Symbol.make) to just (ILApp 'Symbol ... ) as it basically looks just like function application. However, what you propose is IMO better.

DarrenN added a commit to DarrenN/racketscript that referenced this issue Jul 26, 2021
Fixes racketscript#246

Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol)

Swaps in native Symbol to provide interned (Symbol.for()) and
uninterned (Symbol()) symbols. `/runtime/core/symbol.js` is now mostly
a collection of utils to create and compare values.
@stchang
Copy link
Member Author

stchang commented Jul 27, 2021

I'll be able to take a look at this early next week. Thanks again for doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants