atom / language-javascript

JavaScript language package for Atom
Other
194 stars 235 forks source link

Operators #686

Open icecream17 opened 4 years ago

icecream17 commented 4 years ago

Description of the Change

Edit: update

Adds optional chaining a?.b a?.[5] a?.() Adds **= Adds logical assignment &&= ||= ??= Add shift class to >>= >>>= <<= Add **

Ugh unfinished edit this later

Alternate Designs

I don't see how to make the ?. and . distinct without making everything that uses it seperate:

It's much bigger than this but the idea is the same:

Original: "var.prop" -> "variable" "meta.delimiter.period" "property"

Two way I thought of to support ?. "var(?. or .)prop" -> "var" "meta.delimiter" "property"

or

"var.prop" -> "var" "meta.delimiter.period" "property"
"var?.prop" -> "var" "meta.delimiter.optional" "property"

I still added ?. as "meta.delimiter.optional" in the tree-sitter, but in the non-tree-sitter, they're the same. Edit: They're separate selectors, there's a later update below.

I've added tests for the ?. operator, and included ?? in the logic operator test. (Works!!!!!)

Benefits

Fixes #680 Fixes #640 Fixes #715 (duplicate)

Possible Drawbacks

A lot of comments There might be a way to make them separate without adding a bunch of redundant code, but I haven't figured it out yet. I actually did figure it out, again there's an update below

Applicable Issues

640 #680

icecream17 commented 3 years ago

Only seems to fix ?? right now, so the current code doesn't do what I thought (of course...) image

It thinks the ? is ternary, and the period to be a.. regular period.

icecream17 commented 3 years ago

Oh the latest issue made me realize I didn't even put in hmm?.[2] and hmm.jump?.()

I think I should reset the files to atom:master to retry fixing this

utkarshgupta137 commented 3 years ago

Shouldn't we also include "??=" in operators?

icecream17 commented 3 years ago

I added the missing operators but I broke something and the tests are failing

darangi commented 3 years ago

Hi @icecream17 could you take a look into the failing tests?

icecream17 commented 3 years ago

Current status exampleFunc?.() not supported exampleArr[7]?.() not supported a?.b should be support if it works a?.b() should also be supported if it works

Shouldn't we also include "??=" in operators?

Oh yeah. That's added As for the errors, I try to figure them out tomorrow - I don't know what's wrong, except that regular unrelated code isnt parsing so there's some catastrophic error somewhere

For later testing: https://github.com/atom/language-javascript/pull/666#issuecomment-737891449

icecream17 commented 3 years ago

359 fails, now down to 66, though https://github.com/atom/language-javascript/pull/686/commits/6f32f5ad84fe1ca2034c104a27e34c39a4ec7c3a has only 13 errors, so maybe I should just use that instead....

maybe I should provide less updates? edit: Now at 18 wrong edit 2: It is sooo close to working

icecream17 commented 3 years ago

It passed!!!!!!

Summary of changes: image

Non tree-sitter scope changes:

optional matches "?."

meta.delimiter.method.js -->
   meta.delimiter.method.period.js
   meta.delimiter.method.optional.js
meta.delimiter.property.js -->
   meta.delimiter.property.period.js
   meta.delimiter.property.optional.js

keyword.operator.assignment.compound.logical.js added
   used by &&= ||= ??=
icecream17 commented 3 years ago

Oh wait, it only works in the Non-Tree Sitter Parser

icecream17 commented 3 years ago

There's some precedence thing with the operator parsing. I'm not sure how it works.

I found the actual parser: https://github.com/tree-sitter/tree-sitter-javascript And the grammar file: https://github.com/tree-sitter/tree-sitter-javascript/blob/master/src/grammar.json


Note to self: https://github.com/atom/language-javascript/blob/master/grammars/tree-sitter-javascript.cson just maps parsed nodes to names.

icecream17 commented 3 years ago

image

The tree-sitter parser atom uses can't actually parse some operators yet. Will update when it can.

Edit: The tests at the repo above show that it does support optional chaining.... but it doesn't parse correctly still Theoretically it would work if the parser worked too.

Edit 3?: ...see https://github.com/tree-sitter/tree-sitter-javascript/pull/152

Edit 6: See #692

icecream17 commented 3 years ago

Update: See https://github.com/atom/atom/issues/22129 Because of that error it's blocked until atom is updated: https://github.com/atom/atom/pull/22130

icecream17 commented 3 years ago

Ok, even though it still doesn't work in tree-sitter (see blocking), it could still be merged as long as there's no bad changes.


Incompatible changes

Currently I've "added scopes" in https://github.com/atom/language-javascript/pull/686/commits/743fcd5b42fe9efe1f91c61ea7c9aee0c089f946 but https://github.com/atom/language-javascript/pull/690/files takes care of that.

However the scopes for rest vs spread on lines 233/234 are still needed