Skip to content

Conversation

@maxbrunsfeld
Copy link
Contributor

After merging #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 tree-sitter/tree-sitter-typescript#84.

@maxbrunsfeld maxbrunsfeld merged commit 3d54934 into master Sep 22, 2020
@maxbrunsfeld maxbrunsfeld deleted the new-optional-chain-approach branch September 22, 2020 00:18
@mjambon
Copy link
Contributor

mjambon commented Sep 22, 2020

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

@maxbrunsfeld
Copy link
Contributor Author

maxbrunsfeld commented Sep 22, 2020

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
Copy link
Contributor

mjambon commented Sep 22, 2020

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
Copy link
Contributor

mjambon commented Sep 22, 2020

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.

mjambon pushed a commit to semgrep/tree-sitter-typescript that referenced this pull request Sep 22, 2020
maxbrunsfeld pushed a commit to tree-sitter/tree-sitter-typescript that referenced this pull request Sep 22, 2020
* Fix for compatibility with tree-sitter-javascript
tree-sitter/tree-sitter-javascript#138 (commit 3d54934)

* Add optional chaining to 'call_expression', similar to what is done for
the javascript grammar.

Co-authored-by: Martin Jambon <Martin Jambon>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants