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

[JavaScript] Don't try to scope entire call paths #3830

Merged

Conversation

Thom1729
Copy link
Collaborator

Fix #3790.

The scope naming guidelines recommend giving e.g. all of foo.bar.baz() the scope meta.function-call. For reasons discussed in #3790 and in #2575, this is quite difficult to implement in a reliable manner. Currently, the JavaScript syntax definition tries to do this using lookaheads, but it is far from reliable, leading to observable highlighting bugs such as #3790.

This PR rips out the existing code scoping function call paths and instead just scopes the function/method identifier and arguments. This should be sufficient for any of the actual use cases discussed in the linked issues while also being much more reliable. Notes:

  • It's still not perfectly reliable, because we're still detecting the function parens (and in TS, type arguments) via a lookahead, but at least it's a simpler lookahead. It should work well in most cases and fail gracefully in other cases.
  • Originally, the JavaScript syntax scoped method calls as meta.function-call.method. This is not in the guidelines and no other core syntax did this, so I removed it for simplicity.
  • Also, the JavaScript syntax did not scope the arguments list as the more specific meta.function-call.arguments, which is widely done, so I added that in.
  • This does result in over-scoping when referring to, but not calling, a builtin method. The solution there, if we care, is probably to slightly restructure the way those support scopes work.

@Thom1729
Copy link
Collaborator Author

Fixed the related failure in the PHP syntax test.

@deathaxe
Copy link
Collaborator

The current way of applying meta.function-call would cause a gap in scope if argument list does not directly follow a function name.

    bar (baz),
//  ^^^ meta.function-call
//     ^ - meta.function-call
//      ^^^^^ meta.function-call.arguments

@Thom1729
Copy link
Collaborator Author

The current way of applying meta.function-call would cause a gap in scope if argument list does not directly follow a function name.

Yeah, I was wondering if anyone would notice or care. I'll count that as a “yes”. 😉

I also found a really minor bug. In foo?.(), the ?. gets the .arguments scope, which is not correct. I'll tweak it a bit, probably cut a couple of contexts that aren't needed anymore.

@Thom1729
Copy link
Collaborator Author

Should be all good now. The part that gets the whitespace after a variable name is a bit of a kludge, but it was either that or do something with the arrow function detection code, and nah.

@Thom1729 Thom1729 merged commit 059e429 into sublimehq:master Sep 3, 2023
2 checks passed
@Thom1729 Thom1729 deleted the javascript-dont-scope-call-paths branch September 3, 2023 18:10
@ratijas
Copy link
Contributor

ratijas commented Nov 14, 2023

This required me to update my downstream QML syntax once again. Which is fine in itself. But I do wonder how to keep CI green when it's running against multiple versions of Sublime Text, so obviously packages are going to be mix-versioned? For now I removed JS-specific scopes from the tests, but ideally it would be good to have them in-place as well.

@deathaxe
Copy link
Collaborator

In case core syntax changes effect inheriting 3rd-party packages those need to ship ST build specific releases by either leave older package versions alone or maintain multiple branches in parallel.

Less, Liquid and other packages are doing so, already. They maintain tags such as <st_build>-<semver> (e.g.: 4107-1.1.0, 4134-1.1.0) to ship dedicated releases with the prefix denoting least required ST build.

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.

[TypeScript] Method calls aren't scoped when they have type arguments
4 participants