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

Sync locals from nvim-tree-sitter. #345

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rwjblue
Copy link

@rwjblue rwjblue commented Sep 7, 2023

  1. Add (block) as a @local.scope
  2. Fix the scope for functions. function_declaration is used for
    abstract functions, while function_definition is used for the
    actual concrete functions (along with their function bodies).
  3. Add (class_parameter)'s name as a local definition.

References:

1. Add `(block)` as a `@local.scope`
2. Fix the scope for functions. `function_declaration` is used for
   abstract functions, while `function_definition` is used for the
   actual concrete functions (along with their function bodies).
3. Add `(class_parameter)`'s `name` as a local definition.

References:
- nvim-treesitter/nvim-treesitter@ee64345
- nvim-treesitter/nvim-treesitter@06075ec
Copy link
Collaborator

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks!

@eed3si9n
Copy link
Collaborator

eed3si9n commented Sep 8, 2023

  ✗ basics.scala
    Failure - row: 79, column: 24, expected highlight 'method', actual highlights: 'none'

@rwjblue
Copy link
Author

rwjblue commented Sep 8, 2023

Thank you for reviewing, I'll dig into those failures!

@rwjblue rwjblue marked this pull request as draft September 8, 2023 15:32
This adds a test that illustrates the problem being fixed by moving
`@local.scope` from `(function_declaration)` to `(function_definition)`.

```scala
def meth_with_params(localParam: Int) {
  var ref_param = c"$localParam $meth_with_params"
//                   ^parameter
//                               ^method
}
var okay1 = s"hello"
var okay = c"$localParam $okay1"
//            ^none
//                        ^variable
```

Specifically, the local function parameter (named `localParam`) should
only be in scope within the body of `meth_with_params`, but on master
(e.g. without the `@local.scope` move to `(function_definition)`) for
`var okay = c"$localParam $okay1"` the `localParam` is highlighted as a
**parameter**!.

Here is the failure for the new test when ran against `master`:

```
    Failure - row: 6, column: 14, expected highlight 'none', actual highlights: 'parameter'
```
@rwjblue
Copy link
Author

rwjblue commented Sep 8, 2023

@eed3si9n - As far as I can tell (I'm still ramping up my debugging skills for tree-sitter), the failure here demonstrates a somewhat fundamental issue with how tree-sitter highlight actually works. This issue is discussed over in nvim-treesitter/nvim-treesitter#170 (and resolved in nvim-treesitter/nvim-treesitter#295 by allowing function definitions to specify that they are valid in the parent scope as well as their own child scope).

To illustrate the problem I just pushed a test that is being fixed by moving @local.scope from (function_declaration) to (function_definition).

def meth_with_params(localParam: Int) {
  var ref_param = c"$localParam $meth_with_params"
//                   ^parameter
//                               ^method
}
var okay1 = s"hello"
var okay = c"$localParam $okay1"
//            ^none
//                        ^variable

Specifically, the local function parameter (named localParam) should only be in scope within the body of meth_with_params, but on master(e.g. without the @local.scope move to (function_definition)) for
var okay = c"$localParam $okay1" the localParam is highlighted as a parameter!

Here is the failure for the new test when ran against master:

    Failure - row: 6, column: 14, expected highlight 'none', actual highlights: 'parameter'

@rwjblue
Copy link
Author

rwjblue commented Sep 8, 2023

FWIW, I created nvim-treesitter/nvim-treesitter#5359 to ensure that the nvim-treesitter locals for scala are correct. As far as I can tell there is no corresponding feature for tree-sitter highlight that we could use. (Please correct me if I'm wrong!!)

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.

2 participants