Repository navigation
For module names with apostrophe, generated javascript is invalid #3601
Description
Activity
Thanks for the report. I’m in favour of disallowing primes in module names entirely.
I haven't thought through everything here, but I think the answer could be as simple as replacing
'with$inLanguage.PureScript.CodeGen.JS.Common.moduleNameToJsmoduleNameToJs :: ModuleName -> Text moduleNameToJs (ModuleName pns) = let name = T.replace "'" "$" (T.intercalate "_" (runProperName `map` pns)) in if nameIsJsBuiltIn name then "$$" <> name else name
This 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 validmodule JsVarName (JsVarName, mkJsVarName) where import Data.Text newtype JsVarName = JsVarName Text -- Guaranteed to return a valid javascript variable name, or fail mkJsVarName :: Text -> JsVarName -- or use Either mkJsVarName = ...
Did a little more looking in that same module and I think there's an even simpler solution using existing functionality:
moduleNameToJs :: ModuleName -> Text moduleNameToJs (ModuleName pns) = anyNameToJs name where name = T.intercalate "_" (runProperName `map` pns)
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.
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.
but in effect this is changing the language specification
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.
Reacted by Harry GarroodYes, 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).
There's a language specification? 😛
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.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.
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?
No, I don’t think this is urgent and cst will most likely be in the next release anyway.
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)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
- Im using nix, so it’s a bit of a special situation :) although if it were in the .ghci I think it would have worked…On Wed, Apr 17, 2019 at 19:46 Harry Garrood ***@***.***> wrote: 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 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3601 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG3BN3MHGD3HMYFYCCC56TPQ7AFRANCNFSM4HFT7XGA> .
- added a commit that references this issue
on May 7, 2019 - added a commit that references this issue
on May 7, 2019
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
contains the following import
purs compile ...completes without error, but trying to call the main module withspago runcauses the following error: