Coffeescript runtime bug: private static class fields

[greghuc]

trafficstars

Coffeescript bug (runtime error)

"civet coffeeCompat"

class API

  HTTP_GET = 'GET'
  HTTP_HEAD = 'HEAD'
  HTTP_VERBS = [HTTP_GET, HTTP_HEAD]

  callApi: ->
    console.log HTTP_VERBS[0]

(new API()).callApi()
# Next line errors in CS as HTTP_VERBS is private attr
# console.log API.HTTP_VERBS[0]

in the Civet playground this compiles, but then errors at runtime with

ReferenceError: HTTP_GET is not defined

whilst in the coffeecsript playground the code compiles and runs with output

GET

Playground links

Coffeescript link Civet link

Discussion

First off, things are moving along with my migration of multiple Coffeescript codebases to Civet, so thanks for all your hard work! I've moved the codebase build systems from browserify to esbuild with the esbuild-coffeescript plugin. The error above comes from switching the esbuild-coffeescript plugin for the Civet esbuild plugin, hacked to process .coffee files. For this particular codebase, all coffeescript is compiling in Civet with no parse errors, and this is the first runtime error I've seen.

Re the particular error, I often use this pattern for class level private constants (which can be referenced by both other class or class-instance functions). In modern JS terms, the closest concept seems to be a private static field.

In the interests of converting Coffeescript to Civet, it would great if this could be handled via "coffeeCompat". However, longer term, I'd be happy for this pattern not to be in Civet if it doesn't fit philosophically, and could be replaced directly with JS private static fields. If that were the case, it would nice if this pattern triggered a compile time parse error - rather than a run time error - so that the errors could be worked through and addressed one at a time.

[edemaine]

I had proposed that CoffeeScript change this behavior here: https://github.com/jashkenas/coffeescript/issues/4552#issuecomment-1019527774 and the response seemed to be that this could considered buggy behavior for CS2, and a change was warranted. That said, I doubt the behavior will actually change at this point, and given that you're relying on it, supporting it probably makes sense as a coffee-compat flag.

I don't see an easy way to map this feature to private static fields. At least, every reference to the fields would then need to be detected and rewritten, and the resulting class would be different from CoffeeScript's output. I think matching CoffeeScript's output (with an IIFE block that does the assignments and returns the class) might make the most sense. It's a shame we can't just have a class declaration within a braced block, but that prevents the class from being visible at the top level...

[STRd6]

I agree that we should match the CS2 behavior when in coffee-compat mode.

For migrating to Civet/ES semantics I would recommend pulling out the = field assignments as := outside the class and explicitly wrapping with a block if you need to limit visibility. I've found that having const assignments alleviates a lot of the trouble around CoffeeScript scoping since you can't accidentally reassign an outer scoped const.

[greghuc]

Three points

• Matching the CS behaviour in coffee-compat mode would be great. That would enable coffeescript to run safely under civet.
• In terms of then moving coffee files fully from coffee-compat mode to full civet, would it be possible to throw a parse error on the offending patterns during compilation, so they can be fixed manually? I.e. civet coffeeCompat -coffeeCrazyClassAttrs throws parse errors. Otherwise if no parse errors are thrown - but runtime errors occur later instead (like now) - then it'll be unclear if a full coffee->civet migration has been successful until the code is exercised.
• The pattern of civet coffeeCompat -coffeeCrazyClassAttrs throwing parse errors on coffeescript cases that work under full coffee-compat mode, but are now illegal in full civet (or the behaviour has changed) seems sane to me. It means the developer can review the offending cases for a particular directive, fix them manually, and then move on to removing the next directive, confident that they are moving towards full civet. I confess I haven't check to see if coffee-compat mode does this already - I'm still battling with moving to esbuild!

[STRd6]

It would be slightly difficult to add compile time checking since it's valid ES (accessing a global variable inside the method body). Currently opening the file in VSCode with the Civet LSP installed will underline the (now undeclared) global references. I recall there also was interest in a tool to use the same LSP code to do typechecking on .civet files from the CLI so that could be one way of solving this as well.

[greghuc]

@STRd6 lack of civet compiler knowledge showing here but.. I suppose I'm thinking if the compiler is running under coffee-compat mode:

• [case of civet coffeeCompat +coffeeCrazyClassAttrs] compiler detects the pattern described in this issue, and generates code as per @edemaine's comment (with an IIFE block that does the assignments and returns the class)
• [case of civet coffeeCompat -coffeeCrazyClassAttrs] compiler detects the pattern described in this issue, and blows up with parse error instead.

And this pattern applies for all coffee-compat directives.

[STRd6]

Currently coffeeCompat is implemented as a meta flag that enables a collection of more specific options. Adding "civet coffeeCompat -coffeeCrazyClassAttrs" would have all the other coffee compat items but omit the class compatibility. This way when someone is ready to rewrite the class code to a new style they would add -coffeeCrazyClassAttrs and adjust their code.

In coffeeCompat it wouldn't be an error and we would match the existing CS2 compilation.

I'm not sure there would be any advantage to a specific error other the TS error about Cannot find name HTTP_GET

It would be possible to add a custom linting step for detecting this pattern but it would be less general and more brittle than using the existing TS error checking and I think the effort could better go towards civet+TS command line type checking.

[greghuc]

@STRd6 thanks for the explanation. Make sense. Getting coffeescript running fully under civet coffee-compat would be fantastic regardless. Once that's possible, I'll take a deeper look into the sanest way to jump from coffee to full civet. Seems like using VSCode+Civet LSP would be useful. My prior thoughts/suggestions were motivated by having the safest coffee->civet transition: having compiler parse errors has made it very clear what still needs fixing.

[STRd6]

I finally realized what CS2 means by class definitions are blocks of executable code

CoffeeScript 2

I don't think that makes implementing this any harder, it just clarifies the scope for CS2 compatibility.