Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Operators #686

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Operators #686

wants to merge 58 commits into from

Conversation

icecream17
Copy link
Contributor

@icecream17 icecream17 commented Nov 13, 2020

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

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

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

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

"delete" is not listed as an operator like typeof... not changing for now
add "optional", "**=", and other test
@icecream17
Copy link
Contributor Author

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

@darangi
Copy link
Contributor

darangi commented Jan 25, 2021

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

@icecream17
Copy link
Contributor Author

icecream17 commented Jan 26, 2021

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:
#666 (comment)

@icecream17
Copy link
Contributor Author

icecream17 commented Jan 30, 2021

359 fails, now down to 66, though 6f32f5a 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

grammars/javascript.cson Outdated Show resolved Hide resolved
@icecream17
Copy link
Contributor Author

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.

The colon is used for a lot of different things. I'm using what the non-tree sitter labels them. Although there's probably better names.
The atom console is outdated
This is a visual commit.
Really it's about precedence, how specific the selectors are. Since they're the same it should work no matter what.

The reason ?. isn't detected is because of the parser itself.

See https://github.com/jcs-PR/tree-sitter-javascript/pull/1/files and tree-sitter/tree-sitter-javascript#150
@icecream17
Copy link
Contributor Author

icecream17 commented Feb 12, 2021

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 tree-sitter/tree-sitter-javascript#152

Edit 6: See #692

@icecream17
Copy link
Contributor Author

Update:
See atom/atom#22129
Because of that error it's blocked until atom is updated: atom/atom#22130

@icecream17
Copy link
Contributor Author

icecream17 commented Jul 14, 2021

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 743fcd5 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

icecream17 added a commit to icecream17/pulsar that referenced this pull request Oct 2, 2022
The non-tree sitter version is almost completely copied so nothing to say there.

There are some inconsistencies/missing operators in the tree-sitter version, for example:
- `this`
- `new.target`
- `import.meta`
- The keyword.operator order looks more wrong the more I look at it but that's for another day because of the possible scope regularity change in the future: atom/language-javascript#690
- btw atom/language-javascript#691 looks good to me I should probably do that

Even though tree-sitter will have some above changes I added the new operators (`** **= ?? &&= ||= ??= void`) anyway.
Well `void` is not new it's just missing.
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators for a list of operators. Of course while the comma is technically an operator, hopefully it's used more to separate elements in arrays or properties in objects.

Also `delete` is an operator not control.
@icecream17 icecream17 mentioned this pull request Oct 2, 2022
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants