[[FIX]] Computed names for object getters and setters

[sstern6]

Added logic in src/jshint.js that checks the next value of a getter and/or setter in ES6 so errors will not be thrown if a developer writes getters and/or setters in their JS file, if the next value is an open bracket [, then we assign a variable to the already implemented computedPropertyName, and if thats not the next character we call propertyName(), which was originally the only function call that was being assigned to i.

This issue fixes #2871

[coveralls]

Coverage increased (+0.001%) to 97.674% when pulling c221d41 on sstern6:es6-computed-names-get-set into d800e44 on jshint:master.

[jugglinmike]

This comment is a bit literal; on its own, the code already communicates this information.

[sstern6]

Thank you, will remove.

[jugglinmike]

Thanks for the patch! In master today, the tersely-named i variable is
expected to contain a primitive string value. With this patch applied, it will
occasionally contain an object value. This interferes with the heuristic in use
by the saveAccessor function to identify duplicate keys. For example, the
following program should not trigger a warning:

// jshint esversion: 6
var o = {
  get [1](),
  get [2]()
};

...but the proposed solution triggers the following:

line 5, col 10, Duplicate key '{b}'. (W075)

Relatedly, the following code produces the expected errors, but the messages
are incorrect:

// jshint esversion: 6
var o = {
  get [2](x) {},
  set [2]() {}
};

Errors:

line 4, col 10, Unexpected parameter 'x' in get {b} function. (W076)
line 5, col 10, Expected a single parameter in set {a} function. (W077)

We should also make sure that the long-deprecated members
directive continues to function correctly.

[sstern6]

@jugglinmike took some thought with your comments and this is what I have came up with and would like your feedback.

// jshint esversion: 6
var o = {
    get [1](){ return 1; },
    get [2](){ return 1; }
};

var x = {
    get ["a"]() { return 1; }
};

var v = {
    get [2](x) {},
    set [2]() {}
};

When running this snippet of code, I am now only getting these errors which is I believe the correct implementation and expected result.

../../jshint_file.js: line 12, col 12, Unexpected parameter 'x' in get 2 function.
../../jshint_file.js: line 13, col 12, Expected a single parameter in set 2 function.

I changed the logic a bit around in order to give i the string value it was expecting i = computedPropertyName().value; and adjusted the conditional by checking if the item's next type is a punctuator and if its next id is an open bracket.

QUESTION: Your last comment asks to make sure with this path we continue to support the members functionality, which is deprecated. I cannot find anywhere online where people are using examples of members to ensure this is accounted for correctly. In addition, the docs specify A directive for telling JSHint about all properties you intend to use.. Does this mean to declare // members 'get', 'set' using the above code snippet as an example and to check the correct error messages are being displayed? The test suite is passing just want to make sure everything continues to work.

Thank you and let me know your thoughts.

[coveralls]

Coverage increased (+0.001%) to 97.7% when pulling 521ef75 on sstern6:es6-computed-names-get-set into ee0acab on jshint:master.

[sstern6]

@jugglinmike was just checking in to see if you had any additional feedback for this PR.

Thank you

[jugglinmike]

Not yet :)

[jugglinmike]

This is closer, but it's still not quite done. Take a closer look at the return
value of the computedPropertyName function. It always returns a token, but
that token doesn't necessary describe the value of the expression. Here are
some examples where that is relevant:

({
  get [a]() {},
  get a() {},
  get [1 + 2]() {},
  get '+'() {}
});

This code does not necessarily produce any duplicate keys, so JSHint should not
warn about that.

Basically, if i is not a literal value, then we can't know what will happen
when it is evaluated, and we have to just settle for ignoring it. In those
cases, I believe you can set i to null and avoid the linting checks for
property names.

We'll also need dedicated tests for the edge cases I've mentioned so far. Would
you mind adding them to this patch?

As for members, unfortunately, we can't really know who is using it or in
what contexts. I agree that it probably isn't being used very widely, but as
long as it's in the codebase, we have to do due diligence to make sure it's
technically correct. I think your approach will get you the right solution "for
free," but we should have a test to make sure. Something like:

/* members abc */
({
  get ['a' + 'b' + 'c'](), // <-- this is fine: no warning
  get ['d'](), // <-- violation: warn about "d"
  get [a]() // <-- we can't know if this is okay: no warning
});

Thanks for your hard work and your patience!

[sstern6]

@jugglinmike thanks for the input, will continue on with your feedback and yes will write tests for the edge cases we have been discussing. Will let you know if I have any questions while looking into updating my changes

[sstern6]

@jugglinmike been looking into computedPropertyName returning a token. My guess is we shouldn't be assigning that to i. My attention was then brought to the advance function because in your most recent example, lets take get[1 + 2](){}, when we run through advance, it separates all values in the token to 1, +, 2.

This makes me think I'll need to adjust the advance function and then from there figure out how to call advance if state.tokens.curr === '[', as computedPropertName does, but without returning a token in the area I have adjusted the code in jshint.js.

Also, in my most recent code push, I tried assigning i to a literal value by calling computedPropertyName().value, but as you pointed out this causes other problems.

I am just confirming that I am thinking about this correctly and not overthinking this because having computedPropertyName return a token as a value is not suitable for a lot of edge cases with computed name getters and setters in es6.

Thanks you for working through this.

[jugglinmike]

You're right; some of the previously-existing call sites for computedPropertyName
probably suffer from bugs of the type I've been describing.

advance is a pretty central function, so I think it's unlikely to need
modification to address this problem. I'm open to the idea, but we should
define the rationale more formally before considering that.

Really, though, I think we can tackle this through computedPropertyName
alone. Basically, if the value it creates is not a literal, then JSHint
cannot know the name. So for the purposes of that function, it should probably
just return null and let the caller decide on the best course of action from
there.

I haven't dug into this very deeply, though, so there may be some requirement
that precludes that approach. If you're interested in continuing along these
lines, then take my advice with a grain of salt. But if we do need to fix
computedPropertyName, we should also take this as an opportunity to document
its behavior with the JSDoc-style comments that are slowly making their way
into the code base.

If this has become more than you have time for, then don't hesitate to let me
know.

[sstern6]

@jugglinmike Think I got it! Writing tests and comments, will have the updated PR ready for review tomorrow!

thanks

[coveralls]

Coverage increased (+0.006%) to 97.725% when pulling 1a2661d on sstern6:es6-computed-names-get-set into 8accbae on jshint:master.

[sstern6]

@jugglinmike updated with most recent changes, comments, member support, and testing. In my PR, on line 4923, I checked if the type of value is not a string and then, assigned value = null. But given the example and issue:

({
  get [1 + 2]() {},
  get '+'() {}
});

I checked if value.left && value.right exist, and then if those values are not strings, then reassign value.value to be the combination of value.left and value.right because the evaluated expression should be compared, not the individual parts of the expression. This feels a little hacky but the only way I could see solving this issue without touching other methods. That being said, if we check, and where I got my inspiration from:

({
  get ["1" + "2"]() {}
})

Value does not have a left and a right property when checking value in computedPropertyName, this means somewhere in the code, it is seeing that this string is an expression and adds the values. This looks like it could be happening in advance or expression.

That being said, i dont know if its wise to allow users to have numbers or numeric string values as getter or setter properties without a warning or error. Per the MDN documentation, it seems like the only values given as examples are strings with chars a-z || A-Z.

Please let me know your thoughts on the PR and if you have any feedback on the implementation, tests, or JSDoc comments.

Thank you!

[jugglinmike]

The logic that evaluates the expression in this latest patch isn't quite right:

if (value.right.type !== "(string)" && value.left.type !== "(string)") {
  var currValue = value;
  value.value = value.left + currValue.value + value.right;
}

In the above code, value.left and value.right are both objects, so the new
property value.value ends up looking like "[object Object]+[object Object]". That means that JSHint would interpret the property names in ({ [1+2]: 0, [3+4]: 0 }) as being equal and issue spurious warnings. It is also a
little inconsistent to define a value attribute for a token of type
(punctuator).

We could continue to refine this approach to allow JSHint to statically
evaluate certain expressions, but given how infrequently such patterns are
likely to appear, I don't think the benefit to the user justifies the
complexity of the code.

I'm sticking with the recommendation in my previous comments: when value is
not a literal primitive, then JSHint should not attempt to enforce any
property-name-related restrictions at all. This should be possible by returning
null in such cases.

It's great to see more documentation! However, JSDoc comments such as the ones
you've added are designed for use with functions. Note how the keywords
@param and @returns don't really apply to a generic list of statements. The
best thing for this patch would be to document computedPropertyName itself,
basically just to make clear that the function will either return a "token"
object or null (and the conditions that determine which).

[sstern6]

Thanks, removed the logic and just have:

  if (value.type !== "(string)") {
    return null;
  }

inside of computedPropertyName. This is what you mean by checking if value is a literal primitive, correct?

I have adjusted the comments, now I have a few tests breaking because I am returning null if it is not a string literal and then JS throws an error because it cannot read property x of null.

Ill work through this and make sure I have all tests passing then I will repush.

Thank you for your patience and working through this with me.