[CS2] Throw an error for ambiguous get or set keywords or function calls

[GeoffreyBooth]

This PR implements our consensus for how to handle getters and setters, or more precisely the get and set keywords, in CS2:

The get and set shorthand syntax is too infrequently used, and a discouraged practice, for CoffeeScript to support directly. Getters and setters can be created via the Object.defineProperty method, so they technically already are supported in CoffeeScript; supporting the shorthand syntax as well just makes them more convenient to use, but Douglas Crockford argues that we should rarely if ever be using them.

So the task for CS2 is to have the compiler throw an error when it appears that a get or set shorthand syntax keyword is being used. Things like the following:

class A
  get b: ->

c =
  get d: ->

e = ->
  get f: ->

And set for all of the same. These should throw compiler errors, without prohibiting people from using variables or functions named get or set elsewhere in the code. Basically when a call to a function named get or set is given an argument that is an object, and that function is called without parentheses, we throw an error. All someone has to do to use their function named get or set is to call it with parentheses, e.g. get({b: ->})

If we someday change our minds and decide to support get and set shorthand keywords, the work has already been done in this PR to recognize the tokens. They would need to be added to the grammar as keywords and output accordingly. Thanks to this PR, it wouldn’t be a breaking change.

@connec @mrmowgli @carlsmith

[vendethiel]

The [g|s] should simply be [gs].

[GeoffreyBooth]

Damn. You win.

[lydell]

For the record, that's not just shorter but also more correct. [g|s]et would also match |et.

[vendethiel]

Would it make sense to catch class then get @a = 5? (adding properties to the prototype).

[vendethiel]

get "a": 5 will compile to get({a: 5});, and get "#{a}": 5 compiles to get({[`${a}`]: 5});.

[vendethiel]

The PR will also catch {get a: b} = c, but I'm fairly sure that's fine :) (on master, it desugars it to {get({a: b})} = c, and error on get).

[lydell]

I would have said LGTM unless I had read @vendethiel's comments. Now I'm not sure.

[GeoffreyBooth]

Okay, I’ve caught get and set before static methods:

[stdin]:1:12: error: 'get' cannot be used as a keyword, or as a function call without parentheses
class then get @a = 5
           ^^^

Catching interpolated property names took some doing, but I managed to figure it out:

[stdin]:1:1: error: 'get' cannot be used as a keyword, or as a function call without parentheses
get "a": 5
^^^

[stdin]:1:1: error: 'get' cannot be used as a keyword, or as a function call without parentheses
get "#{a}": 5
^^^

And {get a: b} = c is supposed to be caught.

So my first attempt at this was to try to implement it within nodes.coffee’s class Call, but I realized there was no way to determine if a call was “spaced,” or a function call without parentheses, within Call. That info is known to the lexer and not passed on. So either we need to pass the spaced property onward for every IDENTIFIER token so that it can eventually be read by Call, which seems like a rather major change and potential performance impact, or we do this check entirely within the lexer and we need to work around the fact that we don’t yet know that a particular IDENTIFIER might eventually become a Call. Anyway it seems like we actually can catch all the cases we need to (or can think of) just within the lexer, so it looks like I got lucky.

@vendethiel thanks for being so thorough!

[GeoffreyBooth]

@vendethiel or @lydell, do you think you’ll have any further notes on this? I’m eager to start wrapping up the PRs for 2.0.0-beta1 (of which this is one).

[vendethiel]

I think those two lines can be replaced with @tokens[@tokens.length - 1] - if there's no token, it'll be undefined

[vendethiel]

I tried to think of a way to detect string properties, but outside of creating a special "get/set spaced" token type that the parser would catch, I don't think it's feasible.
So LGTM, looks good, thank you.

[GeoffreyBooth]

@vendethiel How about this? 2d1addf

[vendethiel]

I think we've reached the point of diminishing return, as we now error out on get 'item: 1'. I don't think there's a simple and unambiguous solution.

[GeoffreyBooth]

Well I should revert my last commit at least. The question is, so I also revert the support for throwing an error on get 'a': ->, string properties? I guess the safest is to leave that as is, and the parentheses are required for aget call to any string; or we can just overlook string property names as an edge case not worth burdening all get calls with strings.

[vendethiel]

I 👍 the latter – not worth burdening.

[GeoffreyBooth]

@lydell do you agree?

[lydell]

Yes, I agree.

[GeoffreyBooth]

@lydell @vendethiel okay, I took out the check for “calls” to objects with dynamically-named properties. Care to do a final check before we merge this in?

[joallard]

Hey folks, I need to pitch in here: I've been running into the need for getters and setters A LOT lately. This is becoming more and more of a pain from using Coffeescript and is making me slowly reconsider that, and I'm speaking as a defender and long time user for years.

The problem is that the workaround is pretty long winded, and hard to organize neatly. I've read the documentation about it probably a dozen times now, I'm still not able to learn it by heart. I haven't used the workaround once.

I'm not even sure someone will read this, but I needed to document this somewhere. It seems like more and more, frameworks and libraries are offering getters and setters as the standard way to do things (I'm working with Vue at the moment). Moreover, they're a quite useful tool for a lot of cases. CS, with the experience it provides, pretty much discourages the practice.

Is there really no other syntax that would work? Any workaround would be better:

{property.get -> ...}
# or
{`get property`: -> ...}

anything, but please don't make me type Object.defineProperty!

Anyway, I don't want to rain on anyone's decisions, it's just important to me to give some feedback about the impact of this on my development experience. The reason I use Coffeescript is because it makes my experience better, and this is starting to make a dent in that, unfortunately.

[GeoffreyBooth]

I also use Vue with CoffeeScript, and I haven't run into this issue. Perhaps you can give a realistic example?

I do find myself creating computed properties with getters and setters, but the syntax looks like this:

computed: {
  ...mapGetters 'user', ['profile']

  token:
    get: -> @profile?.token or ''
    set: (val) -> @newToken = val
}

The issue with get and set is that they're not reserved words; it's possible to have a function named get and to call it, e.g. get(). So CoffeeScript get foo already has a meaning: get(foo). We can't change that without it being a breaking change, and seeing as how common get is as a function name (see Express, for example) that would be a pretty disruptive change.

You're right in that getters and setters seem to be getting more popular as frameworks use them to enable reactivity. I would be open to a shorthand syntax if it doesn't create a breaking change. If you can think of one (play around with https://coffeescript.org/#try until your proposed syntax throws an error on the right) then please feel free to propose it.

[joallard]

@GeoffreyBooth Thanks for considering this.

About your example, unless I'm mistaken, this doesn't really create computed properties. So maybe Vue just understands that magically? If so, neat.

I forget my other use cases because I have terrible memory, but this time I was trying to avoid making obj.value() into a function call and keep it as obj.value. The module calling obj.value was elsewhere and I was concerned with maintaining the interface of a property. Basically, this is a case where one needs to keep it as a property (getter) call.

I completely agree that get should be kept useable as it is already. I can see the use. I would thus suggest an alternative syntax. It being ugly I wouldn't mind that much, as long as it's not as contorted as it is now. I can think of a few things, you folks will have more experience than me at understanding what it means for the CS codebase and proposing improvements.

Throwing a few ideas:

obj = {
  hiddenToken: 1
  token:
    .get: -> @hiddenToken
    .set: (val) -> @hiddenToken = val
}

"""
[stdin]:4:5: error: unexpected .
    .get: -> @hiddenToken
    ^
"""

obj = {
  hiddenToken: 1
  token:
    :get: -> @hiddenToken
    :set: (val) -> @hiddenToken = val
}

obj = {
  hiddenToken: 1
  token:
    @get: -> @hiddenToken
    @set: (val) -> @hiddenToken = val
}

# Same error

# Other available prefixing symbols:
# % ? & * =

obj = {
  hiddenToken: 1
  token.get: -> @hiddenToken
  token.set: (val) -> @hiddenToken = val
}

"""
[stdin]:3:12: error: unexpected :
  token.get: -> @hiddenToken
           ^
"""

obj = {
  hiddenToken: 1
  `get token()`: -> @hiddenToken
  `set token()`: (val) -> @hiddenToken = val
}

# or something similarly literal

"""
[stdin]:3:3: error: unexpected get token()
  `get token()`: -> @hiddenToken
  ^^^^^^^^^^^^^
"""

# If I try to get creative, maybe some symbols make some deeper sense?

obj = {
  hiddenToken: 1
  *token: -> @hiddenToken
  &token: (val) -> @hiddenToken = val
}

"""
[stdin]:3:3: error: unexpected *
  *token: -> @hiddenToken
  ^
"""

# Borrowing from C/Yaml's dereferencing operator, and Ruby setter methods:

obj = {
  hiddenToken: 1
  token*: -> @hiddenToken
  token=: (val) -> @hiddenToken = val
}

I'm happy to pitch in some dev efforts if we find something that has some kind of consensus.

[GeoffreyBooth]

I think this syntax is the one most likely to find consensus:

obj =
  hiddenToken: 1
  `get token`: -> @hiddenToken
  `set token`: (val) -> @hiddenToken = val

Mainly because it's not actually about getters and setters at all. It's just adding another place where the PASSTHROUGH_LITERAL token is supported in the grammar. We should probably do this even if we also invent some new syntax for shorthand getters and setters.

One issue though is that at least in my tests, it appears that getters and setters require the method syntax? So like this:

obj = {
  hiddenToken: 1,
  get token() { return this.hiddenToken; },
  set token(val) { this.hiddenToken = val; }
};

Whereas get token: function() is invalid JavaScript syntax. This is an issue because currently CoffeeScript token: -> compiles to token: function() {}. We would need to change the output format for object properties that are functions to use the method syntax, and we would need to do that all the time, since CoffeeScript doesn't know what's inside a passthrough block.

The good news is, I think that should be fine, and an improvement. If you look at classes, we already output the method syntax there; see https://coffeescript.org/#try:class%20A%0A%20%20a%3A%20-%3E%20123. There's been an open issue for a while to bring that syntax over to objects: #5075.

So step one would be to change the output of objects to use method syntax, e.g.:

# Input:
obj =
  fn: -> 123

// Current output:
obj = {
  fn: function() {
    return 123;
  }
};

// Desired output:
obj = {
  fn() {
    return 123;
  }
};

That should be its own PR, and would resolve #5075. Then step two would be to support a JavaScript passthrough as the method name:

obj =
 `fn`: -> 123

Once that compiles (and produces the same output as if you had fn without backticks) then you're done. Users could replace fn with get fn and get the output they want.

[joallard]

That would be a huge step forward actually. So we could have:

obj = 
  val: 1
  `get value`: -> @val
  `set value`: (val) -> @val = val

compiles to

obj = {
  val: 1,
  function get value() {
    return this.val;
  },
  function set value(val) {
    return this.val = val;
  }
};

This doesn't seem too much of a stretch from the current behavior.

So if I understand correctly, after #5075, such a PR would be welcome?