Repository navigation
For module names with apostrophe, generated javascript is invalid #3601
Description
I've noticed quite a few past issues related to codegen and name mangling (e.g., #2558, #2847) and also an issue specifically about module names (#3254).
This may be a new issue not yet addressed by any of the prior.
As of version 0.12.3, module names and qualifiers can have apostrophes, but the corresponding variable names in generated javascript aren't mangled properly.
For example, javascript for the two purescript modules
module Foo' where
foo = "foo"
module Main where
import Foo'contains the following import
var Foo' = require("../Foo'/index.js");purs compile ... completes without error, but trying to call the main module with spago run causes the following error:
Build succeeded.
~/proj/output/Main/index.js:6
var Foo' = require("../Foo'/index.js");
^^^^^^^^^^^^^^^^^^^^^
SyntaxError: Unexpected string
at new Script (vm.js:73:7)
at createScript (vm.js:245:10)
at Object.runInThisContext (vm.js:297:10)
at Module._compile (internal/modules/cjs/loader.js:657:28)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
at Module.load (internal/modules/cjs/loader.js:599:32)
at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
at Function.Module._load (internal/modules/cjs/loader.js:530:3)
at Module.require (internal/modules/cjs/loader.js:637:17)
at require (internal/modules/cjs/helpers.js:20:18)
spago: Running failed, exit code: 1
Activity
hdgarrood commentedon Apr 13, 2019
Thanks for the report. I’m in favour of disallowing primes in module names entirely.
adnelson commentedon Apr 17, 2019
I haven't thought through everything here, but I think the answer could be as simple as replacing
'with$inLanguage.PureScript.CodeGen.JS.Common.moduleNameToJsThis seems (again, untested) like it would fix at least the surface-level issue -- perhaps there's a deeper issue here though; for example rather than using
Textdirectly, a wrapper type could be used which would guarantee that the variable name is validadnelson commentedon Apr 17, 2019
Did a little more looking in that same module and I think there's an even simpler solution using existing functionality:
hdgarrood commentedon Apr 17, 2019
Using the existing name mangling for proper names should indeed work for JS, but I'd prefer to just disallow primes in module names, as allowing primes might require more awkward mangling in other backends. I also doubt that the extra flexibility would be particularly useful to many people anyway; nobody has specifically asked for it to be allowed as far as I can remember.
adnelson commentedon Apr 17, 2019
Sure, I can see the argument against allowing primes in module names for simplicity's sake, but in effect this is changing the language specification, which seems like an unnecessary step when the functionality already exists and a simple bugfix would suffice. For what little it's worth, I'd advocate for fixing the bug first and then making a decision on whether to change the language.
natefaubion commentedon Apr 17, 2019
There's a language specification? 😛
I'm pretty sure the bug is that we are allowing primes to go through. To me it seems clear from existing validation code that the intention was to disallow it.
hdgarrood commentedon Apr 17, 2019
Yes, I'm almost certain that primes were never intended to be allowed in module names, and that this was an unintentional side-effect of a change which was only meant to relate to other proper names (like types, classes, or data constructors).
adnelson commentedon Apr 17, 2019
Theoretical only 😆
OK, well Haskell supports primes in module names, so I figured PureScript was meant to as well. But let's say for the sake of argument that that's not desired. I would argue that's the topic of a separate pull request. The real bug here is that the code is currently is capable of producing invalid javascript. Regardless of whether primes are desired or not, we should still probably be using
anyNameToJswhen converting module names. Or adopting a more type safe solution.hdgarrood commentedon Apr 17, 2019
I think having something like a newtype over Text denoting strings which are safe to use as JS variable names would indeed be good, yes.
matthew-hilty commentedon Apr 17, 2019
Would the closed pull request #3602 work to fix this issue too? It both disallows primes in module names and mangles them just in case they do get through.
I closed it because it seemed that hdgarrood and natefaubion thought the best fix for this issue would be cst. Maybe, though, a simpler fix is needed in the interim?
hdgarrood commentedon Apr 17, 2019
No, I don’t think this is urgent and cst will most likely be in the next release anyway.
adnelson commentedon Apr 17, 2019
I got the itch and spent some time creating a
JsIdenttype and replacing usage ofTextwith that. https://github.com/purescript/purescript/compare/master...adnelson:jsident?expand=1It's type checking, but the tests are currently failing -- not sure why. If I can get them passing I might open a PR
I also annoyingly had some issues being forced to use a
MonadFailconstraint. I'm assuming that's related to the version of GHC I'm on (8.6)hdgarrood commentedon Apr 18, 2019
The package.yaml file should enable -XNoMonadFailDesugaring, so I am surprised you’re seeing that. I’m not sure why it would come up at all
adnelson commentedon Apr 18, 2019
Add tests for grammar fixes addressed by CST
Add tests for grammar fixes addressed by CST (#3629)