tree-sitter / tree-sitter-javascript

Javascript grammar for tree-sitter
MIT License
323 stars 109 forks source link

New approach to handling optional chains #138

Closed maxbrunsfeld closed 3 years ago

maxbrunsfeld commented 3 years ago

After merging https://github.com/tree-sitter/tree-sitter-javascript/pull/137, I realized that there was an even simpler solution to the problem of resolving conflicts between new_expression, call_expression, and all of the optional-chaining rules:

The ?. operator in a call_expression should have the same precedence as the ?. and . operators in member expressions and subscript expressions.

This avoids having a runtime conflict between new_expression and call_expression, and I think is more intuitive than the previous solution. I've also done a bit of cleanup, including renaming _constructable_expression to _primary_expression, since it is used in more places than just new_expression now. So I think we'll have to do the same rename in tree-sitter-typescript.

/cc @mjambon - I think this may simplify the work required for https://github.com/tree-sitter/tree-sitter-typescript/pull/84.

mjambon commented 3 years ago

Interesting. I'm trying this right away on typescript.

maxbrunsfeld commented 3 years ago

Awesome! I haven't looked into optional chain support in typescript, but I just ran tree-sitter generate in the typescript repo, and noticed I needed to do this just to get it to generate:

--- a/common/define-grammar.js
+++ b/common/define-grammar.js
@@ -99,7 +99,7 @@ module.exports = function defineGrammar(dialect) {

       new_expression: $ => prec.right(PREC.NEW, seq(
         'new',
-        field('constructor', $._constructable_expression),
+        field('constructor', $._primary_expression),
         field('type_arguments', optional($.type_arguments)),
         field('arguments', optional($.arguments))
       )),
@@ -677,7 +677,6 @@ module.exports = function defineGrammar(dialect) {
         'boolean',
         'string',
         'symbol',
-        'void',
         'export',
         previous
       ),

I'm not sure why void was added to the _reserved_identifier rule, but I think it is fine to remove it; no tests failed as a result.

mjambon commented 3 years ago

I'm battling npm issues. I thought I upgraded my dependency on tree-sitter-javascript, but I actually I didn't. I selected the commit in package.json as follows:

  "devDependencies": {
    "tree-sitter-cli": "^0.16.9",
    "tree-sitter-javascript": "github:tree-sitter/tree-sitter-javascript#3d54934"
  },

After rm -rf node_modules and npm install, node_modules/ ends up with an older version of tree-sitter-javascript.

What do you use to depend on the correct version of a dependency, tree-sitter-javascript in this case?

Edit: I was looking at the history of tree-sitter-typescript because .git is not preserved in node_modules/tree-sitter-javascript. Still running into weird errors though.

mjambon commented 3 years ago

ok, I'm still not sure about how npm install is supposed to work, but tree-sitter generate and tree-sitter test work with just your changes. I'm now trying to figure out conflicts when I add optional chaining.