Selectively ignore CS-only keywords in ES imports and exports

[GabrielRatener]

@GeoffreyBooth To address your JS interoperability concerns in #3757, this PR enables usage of CS-only keywords as local names in import lists and as any name in export lists. This will enable people to do the following

// now possible
import {unless as someLocalName} from 'foo'

// but not
import {someImportedName as unless} from 'bar'

// for exports CS keywords are allowed everywhere
export {someLocalVar as unless}
export {unless, loop as until} from 'baz'

[GeoffreyBooth]

Thanks for doing this!

This should also be submitted against master, this is basically a bug in the current modules implementation.

[GeoffreyBooth]

This and the following test should be in error_messages.coffee.

[GeoffreyBooth]

“Loop hack”? Why isn’t this block:

    if id is 'as' and @seenImport
      if @value() is '*'
        @tokens[@tokens.length - 1][0] = 'IMPORT_ALL'
      else if @value() in COFFEE_KEYWORDS
        @tokens[@tokens.length - 1][0] = 'IDENTIFIER'

      @token 'AS', id
      return id.length

[GabrielRatener]

Because the code is more selective with the loop. In your case when encountering an as after an { on an import line, the as would always become an AS token, which would prevent its currently allowed usage as an import variable.

[GeoffreyBooth]

Are you sure about that?

coffee -ne "import { as as as } from 'lib'"
Block
  ImportDeclaration
    ImportClause
      ImportSpecifierList
        ImportSpecifier IdentifierLiteral: as IdentifierLiteral: as StringLiteral: 'lib'

See https://github.com/jashkenas/coffeescript/blob/master/test/modules.coffee#L587-L609

[GabrielRatener]

I think, are you using a version of CS with that code in it?

[GeoffreyBooth]

That’s from 1.11.1. See the link.

[GabrielRatener]

Yes, I haven't tried it but I think those tests would fail with the code you're suggesting.

[GeoffreyBooth]

Those tests pass in 1.11.1. They should still pass after your PR. I didn’t test the code I put in the comment, it was just meant to pose a question about the loop. I don’t like the idea of loop, which becomes while (true). I feel like whatever you’re trying to do here, you should be able to do with a simple if/else tree. Is there a reason we need a loop?

[GabrielRatener]

It's not ideal, I basically wan't to reach the @token 'AS', id and return only for a select few token types. So basically if none of the conditionals run, I want to skip that part and not return. Since CS doesn't have goto, I used this as a hack.

It's possible to do it without a loop, but it would require more code.

[GeoffreyBooth]

Combine these two lines:

if tag is 'IDENTIFIER' and (id in JS_KEYWORDS or id in COFFEE_KEYWORDS) and
not (@exportList and id in COFFEE_KEYWORDS)

[lydell]

If I import {unless} from 'foo', I can't do anything with unless then, can I? What am I missing here?

[GeoffreyBooth]

You would need to do import { unless as somethingElse } from 'foo'. This is such an edge case, I’m not sure it’s worth throwing an error warning people that they’re creating an inaccessible variable. Unless wouldn’t import { unless } from 'foo' be accessible via module.unless?

[GabrielRatener]

@lydell Right, so this PR prevents that.

[GabrielRatener]

I moved the error tests to error_messages.coffee, so is this PR ready to be merged?

[GeoffreyBooth]

The following is valid JavaScript, according to Babel:

import { if } from 'lib';

I’m not sure how you would access this if in a runtime that natively supports modules; module.if/window.if? But regardless of that, there’s simply no way to generate this output via CoffeeScript, either before or after this PR. Maybe we don’t care, especially since even in Babel there’s no way to access this if unless it’s aliased?

[GeoffreyBooth]

This looks good to me. @lydell, @jashkenas?

[GabrielRatener]

@GeoffreyBooth I don't think we need to worry about it for now, plus Babel isn't always standards compliant anyway.

[lydell]

Looks good to me too. Good work!