Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

js operators #79

Merged
merged 10 commits into from
May 19, 2023
Merged

js operators #79

merged 10 commits into from
May 19, 2023

Conversation

icecream17
Copy link
Contributor

@icecream17 icecream17 commented Oct 2, 2022

Description of the Change

Adds optional chaining a?.b a?.[5] a?.()
Adds exponentiation, **= **
Adds logical assignment &&= ||= ??= and nullish operator ??
Add shift specifier (css class) to >>= >>>= <<= (non-tree-sitter)
Add void (it's not new but it's missing) (tree-sitter)

I've added tests for the ?. operator, and included ?? in the logic operator test. (Works!!!!!)
One test is deleted because it's a duplicate

Port of the original pr

Alternate Designs

I didn't try too much on tree-sitter scopes because of the possible atom/language-javascript#690

Benefits

Fixes atom/language-javascript#640
Fixes atom/language-javascript#680
Fixes atom/language-javascript#715 (duplicate)

Possible Drawbacks

Some comments

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:

Applicable Issues

atom/language-javascript#640
atom/language-javascript#680
atom/language-javascript#715

Todo

  • Update JS tests
  • Remove rest scope since there's no way to tell whether it's rest or spread (Edit: In some contexts you can guarantee one or the other)

Unrelated

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 marked this pull request as draft October 3, 2022 12:26
@icecream17 icecream17 changed the title migrate https://github.com/atom/language-javascript/pull/686 js operators Oct 3, 2022
@kaosine
Copy link
Member

kaosine commented Oct 4, 2022

Is there any way to also put this on pulsar-edit/core-languages? I want to use this as a test for importing things that way, so that hopefully it's easier to update those without having so much in here....especially, so maybe it's easier to disable stuff that doesn't work or whatever as needed....

@icecream17
Copy link
Contributor Author

how does the core-languages repo work?

@kaosine
Copy link
Member

kaosine commented Oct 4, 2022

It's basically going to be an organized mono-repo of our language support as a test to organizing everything in regard to a discussion in the org level stuff. Just haven't been able to get it all uploaded, since just migrating over this specific language's commit history is taking forever on my end if I did it right in fish.

Since I'm trying to migrate(have it synced) it with this command as to what the current status is of that directory over here:

git log --pretty=email --patch-with-stat --reverse --full-index --binary -m --first-parent -- packages/languages-javascript >> (cd ../packages/core-languages && git am --committer-date-is-author-date)

I probably need to do even more research on how to do this right since I just kinda went with the first SO that I found that seemed to be what I needed and just converted the command to fish terms...

@icecream17
Copy link
Contributor Author

icecream17 commented Dec 3, 2022

what's the status of core-languages? / this pr?

@Spiker985
Copy link
Member

@icecream17 We've actually monorepo'd the languages into the pulsar repo, so this is still the proper location for it.

If this request is ready for review, we can move it from a Draft status into a Ready status

@icecream17
Copy link
Contributor Author

icecream17 commented Dec 19, 2022

the language-js tests still need to be updated since some of them are failing, working on it

?. is illegal in new

update console methods
timeStamp has the same support as profile and both are nonstandard so they're both included

update global classes
Wait a minute, Generator and GeneratorFunction are _not_ global...
so actually just any class specified in ECMA262
non global additions:
- AsyncFunction
- AsyncGeneratorFunction
- TypedArray
luckily MDN's sidebar of
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
shows all of these
Wait... JSON? That's not a class
Math has the scope support.class.math.js for some reason so I guess add "json" likewise for json

Never knew about WeakRef and FinalizationRegistry
Wonder why it's so unknown; this is a common idea in Rust at least....

So yeah, JSON is separate just like Math
Lots of copying and pasting

rest vs spread depends on context
I don't know why that test failed
@icecream17
Copy link
Contributor Author

don't understand why the >>= et al shift operators are failing especially since those tests passed at atom/language-js ... even weirder is that the right side of the to be message is wrong

@icecream17
Copy link
Contributor Author

icecream17 commented Jan 10, 2023

Woah #299 might've been legendary, can someone retry the tests?

Edit: Also marking as ready since I don't plan on making changes unless there's some bug

@icecream17 icecream17 marked this pull request as ready for review January 10, 2023 03:07
@confused-Techie
Copy link
Member

Woah #299 might've been legendary, can someone retry the tests?

Not sure we need to retry them, when I authored that PR it fixed all tests for that package locally, and iirc resolved them in the pr tests

@confused-Techie
Copy link
Member

Woah #299 might've been legendary, can someone retry the tests?

Edit: Also marking as ready since I don't plan on making changes unless there's some bug

Would you mind updating this branch from master? Since especially here our tests have improved a ton

@icecream17
Copy link
Contributor Author

I clicked a button to merge 504 commits, hopefully that "just works"

@confused-Techie
Copy link
Member

So that's for merging the master branch here, but it seems on the language test package bundle, we are having 34 failures when we would expect only 18 on this bundle.

So something isn't quite happy. If you'd like a reference for the 18 tests that we expect to fail compared to yours, you can look at the tests from this recent PR

@kaosine
Copy link
Member

kaosine commented Jan 10, 2023

@icecream17 We've actually monorepo'd the languages into the pulsar repo, so this is still the proper location for it.

If this request is ready for review, we can move it from a Draft status into a Ready status

Yeah it was me trying something and just wanting a duplicate for that side of things even tho it didn't work out the way I wanted too(and I think languages may have changed and broken anyways since that point from what I was trying to do....)

@confused-Techie
Copy link
Member

Alright, so like I mentioned we are getting some more failures here than we would hope.

But I would like to see us be able to merge these changes if they improve our handling of JavaScript, so maybe once #307 is merged I'll go ahead and update this PR from master and see if we are still getting the failures. (That PR contains many many fixes to our test runner, tests, and easier viewing of the test output after the fact. Meaning we can say exactly this causes these tests in language-javascript to fail, or know they are unrelated.

Thanks for the patience

@icecream17
Copy link
Contributor Author

I've gone ahead and merged 162 commits to include the now merged #307

@icecream17
Copy link
Contributor Author

And here's some affected code

non-tree-sitter

let a;
a ??= a ?? true
a **= a ** true
a &&= false
a ||= true
a |= 5
a &= 4
a ^= 3
a <<= 2
a >>= 1
a >>>= 0
(...rest) => [...spread]

obj?.prop
a()?.b()
a.b?.()
a()?.MY_CONSTANT
a?.b()
a?.b?.()
a?.[3]
a[4]?.()
new a?.b // < should display as illegal

// useless ?. support in api
Math?.PI
Relay?.QL``

// barely any difference (now detected as `support.function.json` or similar instead of `entity.name.function`)
// similar to the difference between `console.log` and `console.customfunction`
// some api additions, like
JSON?.parse()
console.timeStamp()
Promise.allSettled()
BigInt
WeakRef

tree-sitter

let a;
a ??= true
a &&= false
a ||= true
a **= a ** a
a = [2, 3]
delete a[1]
a = void a 

@confused-Techie
Copy link
Member

@icecream17 I really appreciate you getting this branch updated with all of our new CI. Made things super easy to review.

Firstly the intel_mac build failed, but seems to be some of the flakiness within node-gyp and not the cause of your changes.

As for package tests we have the following which looks great:

  • find-and-replace: 50 Failures (Within Expected Range)
  • symbols-view: 2 Failures (Expected 2)
  • tree-view: 2 Failures (Expected 2)

But then unfortunately we do have some failures within language-javascript which would be awesome if you could take a look at:

JavaScript grammar ES6 tagged Relay.QL string templates tokenizes them as strings
JavaScript grammar ES6 tagged Relay.QL string templates with interpolation tokenizes them as strings

given that the entire Relay.QL was entity.name.function, it's not surprising here
Therefore this pr just adds "meta.delimiter.<>" to the delimiter in Relay.QL or Relay?.QL
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've just now seen that @icecream17 had fixed the tests when I originally requested.

Just wanna say sorry this has been around for so long without further discussion.

But at this point the changes here look solid, and tests seem to be happy with it.

So thanks again for all the awesome work here, we really appreciate it!

As for everyone else @pulsar-edit/core how do we feel about merging this one?

And for some confirmation, the tests:

  • find-and-replace: 58 Failures, within expected range
  • symbols-view: 2 Failures, within expected range
  • tree-view: 2 Failures, within expected range
  • intel_mac build: Seems to have failed when building native modules. I think this was an issue we had fixed after bumping NPM in PPM?
  • windows: Timed out, so likely not an actual issue with this PR

So with that said, things look basically where they should be, but even for those two failures I don't expect them to be the fault of this PR. So how do we feel about merging? And getting some modern features supported by language-javscript?

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 4, 2023

I will re-run the Cirrus CI.

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 4, 2023

Oh, the intel macOS Cirrus thing is down to #384 not being on this "older" branch.

October 2, 2022

👀 ❗

I'll run a branch with that fix just to make sure it passes CI, no need to complicate the commit history on this PR branch I'll do it separately.

EDIT: See status of the CI runs here: icecream17-js-operators-rebase --> https://cirrus-ci.com/build/4880197007179776

EDIT 2: Cirrus is looking good now. (See links just above).

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 4, 2023

✅ lite approve, as language grammars are a bit over my head, but I like the spirit of this, and it's something I can generally be positive on.

I leave more in-depth and technical reviewing to others who may be more familiar with this stuff.

@icecream17
Copy link
Contributor Author

icecream17 commented Mar 27, 2023

I've gone ahead and rebased 165 commits again to include #384

@confused-Techie
Copy link
Member

The windows test had failed here, so I've rerun them otherwise, as before (with these new changes) tests are looking great!

  • find-and-replace: 47 Failures ✅
  • settings-view: 1 Failures ❌ (Not an issue on this PR, we are seeing this repo wide)
  • symbols-view: 2 Failures ✅
  • tree-view: 2 Failures ✅

So with how long we've had to sit on this on, and the fact that nobody has found any issues with it, how do we @pulsar-edit/core how do we feel about merging this one? And if anything I can merge at the end of the day, if that sounds alright.

savetheclocktower added a commit to savetheclocktower/pulsar that referenced this pull request Mar 28, 2023
…thData`

This hadn't occurred to me until I tried to mark illegal optional-chaining operators in certain contexts, much like how it's done in pulsar-edit#79. But it's a powerful way to match things regardless of their exact ancestry.
@icecream17
Copy link
Contributor Author

Oop, that latest mention said that optional chaining is illegal in the left hand side of an assignment. I don't think I implemented that; should probably add tests...

@icecream17
Copy link
Contributor Author

icecream17 commented May 16, 2023

unfortunately, the general pattern a?.prop = 7 isn't checkable without massive work so I think after I add tests this pr is ready. i'm not completely sure that the ternary expression true?.5:1 parses either

another thing a future pr might do is update the picture in README.md since now ?? should be syntax highlighted

Edit: The commit before the merge failed one test and I'm not sure why, so merging 334 commits.

@confused-Techie
Copy link
Member

@icecream17 Do those last few commits addressed the issue you pointed out above?

Since right now all tests but the windows build (which is failing for unrelated reasons) is looking good, and I'd be inclined here to get this merged so you can stop worrying about it, and we can make use of your awesome contributions here

@icecream17
Copy link
Contributor Author

yes, my concerns are resolved

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to say @icecream17 I really do appreciate your efforts here. Sorry they have taken so long to fully address, but it's awesome that you've stayed up to date here, and keeping this one working.

I'll go ahead and merge this one, appreciate you

@confused-Techie confused-Techie merged commit e7909cb into pulsar-edit:master May 19, 2023
@icecream17 icecream17 deleted the js-operators branch February 16, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants