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

refactor scopes to add top-level and block scope (related!) #5475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cosmicexplorer
Copy link
Contributor

Problem

This investigation was initially motivated by the desire to add notation for explicit var declarations, in particular so that we might be able to attach JSDoc comments to them (see #5377 (comment)). However, I then happened upon the following snippet:

coffeescript/src/nodes.coffee

Lines 3224 to 3230 in 817c39a

checkScope: (o, moduleDeclarationType) ->
# TODO: would be appropriate to flag this error during AST generation (as
# well as when compiling to JS). But `o.indent` isn’t tracked during AST
# generation, and there doesn’t seem to be a current alternative way to track
# whether we’re at the “program top-level”.
if o.indent.length isnt 0
@error "#{moduleDeclarationType} statements must be at top-level scope"

I realized that "block scope" (as per let and const) is related to import and export declarations: because we don't currently have a concept of "block scope", we are also unable to validate module statements, because those must be performed at the top-level scope--not within a block scope!

Solution

  • Scope is now aware of both var (function) and block scope.
  • We instantiate a new type TopLevelScope at the top level.
    • We give this class responsibility for managing import/export bookkeeping.
  • We instantiate a BlockScope type wherever we introduce a new block scope.
    • Important: we don't actually do anything with the block scope right now!
    • It is purely used to identify e.g. whether we're inside an if block or at top-level (module) scope.

Result

No external change: this code is largely an internal refactoring! Some code which would definitely have failed at runtime (attempting to assign to an imported symbol) is now caught at compile-time. I believe this is a desirable feature, but it may cause a command to error which didn't before. We could convert this into a warning if that is considered a breakage.

Otherwise, our solution is general, and works for both compilation and ast generation. The snippet at top has been obsoleted, and these tests now use simply assertErrorFormat():

test "imports and exports must be top-level", ->
assertErrorFormatNoAst '''
if foo
import { bar } from 'lib'
''', '''
[stdin]:2:3: error: import statements must be at top-level scope
import { bar } from 'lib'
^^^^^^^^^^^^^^^^^^^^^^^^^
'''
assertErrorFormatNoAst '''
foo = ->
export { bar }
''', '''
[stdin]:2:3: error: export statements must be at top-level scope
export { bar }
^^^^^^^^^^^^^^
'''

There were some tests which incorrectly interpreted the default import/export identifier, so these were removed and/or fixed as well.

Future Work

This should pave the way for #4985, #5377, #5445, but is completely independent of them.

The big takeaway with this change is that a concept of block scope is intricately related to the top-level scope required for import/export declarations. Having to wrangle the myriad behaviors of ES6 module statements was not exactly fun, but this PR demonstrates that the CoffeeScript compiler is able to have an awareness of block scope without any breaking changes or syntax changes.

- annotate delimiter pairing logic
- generate a var statement for the given node
- [FIX!] update scope to use positions and call .type() recursively!

this fixes the following code:
```coffee
import x from "./lib/coffeescript/index.js"
do ->
  x = 3
  x
```
- rename "scope" to FunctionScope
- break out a separate TopLevelScope so we can look at modules more clearly
- add lots of TODOs and fix namedMethod?()
- remove extraneous 9e9 limit from .splice call
- add quite a lot of notes to compileWithDeclarations
- add block scopes!!!!
- imports and exports are so much better supported now!!!
- assigning to an imported symbol is an error
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