search

estools / esquery

ECMAScript AST query library.
http://estools.github.io/esquery/
Other
825 stars 91 forks source link

disallow empty attribute names in query language #108

Closed MarkKoester closed 4 years ago

MarkKoester commented 4 years ago

Currently, an attribute name can be any sequence of identifiers and dots. For example, the parser will except a selector like:

[...hi..hello]

The getPath function will probably end up returning undefined in any case when it indexes an object using an empty string.

This pull request modifies the attribute name rule to only accept identifiers separated by exactly one dot.

The resulting array from the rule is flattened using the array reduce function. I noticed a comment further down in the grammar that seemed to indicate that the array flat() function is not supported. If it is supported, the transformation can be simplified to 

return [a, ...as.flat()].join('');

It seems like there's a unit test that expects the parser to accept attribute names with a leading dot. I'm not sure what the purpose of this is since the getPath function will always return undefined in this case, but if this is the intended behavior then the grammar rule can easily be modified to accept it.

michaelficarra commented 4 years ago

While there's nothing theoretically wrong with accepting the empty string as a property name, I don't really see a practical need for it, so I'm okay with this.

You're going to need to drop that test that you mention for CI to pass.

michaelficarra commented 4 years ago

Thanks for the contribution, @MarkKoester!

brettz9 commented 4 years ago

As far as the change, I'd personally worry that people might want, e.g., to make ESLint rules which checked for empty string values or such. But I admittedly don't have a personal need for it.

As far as the coverage, FWIW, IIRC, the coverage I was speaking of in that test was in regard to the JS parser file, which one can meaningfully get coverage of in using the fork in this PR (which adds ignore statements to PEG.js boilerplate or else branches of generally less interest).

MarkKoester commented 4 years ago

Yeah I actually didn't know you were allowed to use the empty string as a key but I suppose that would actually be a valid use. I still think this is a good change to make since there are already a lot of valid JavaScript object keys that technically wouldn't parse, for example ">".

Another minor point is that I think if you actually wanted to support using the empty string as an object key it should still parse as an identifierName with the value of an empty string. It was only technically supported before because two dots in a row happened to output the same thing as an identifier with an empty value. The fact that it was allowed in the first place was sort of unintentional.