tree-sitter / tree-sitter-typescript

TypeScript grammar for tree-sitter
MIT License
341 stars 104 forks source link

Conditional ternary operator is not highlighted properly #200

Closed carlosala closed 2 years ago

carlosala commented 2 years ago

The following piece of code is parsed properly but the highlight of the ? is not correct, should be red instead of white. The same exact code in javascript is highlighted properly. Screenshot from 2021-12-15 23-25-26

resolritter commented 2 years ago

Should be transferred to https://github.com/tree-sitter/tree-sitter-javascript since that is where the "?" ternary expression token is defined

https://github.com/tree-sitter/tree-sitter-javascript/blob/fdeb68ac8d2bd5a78b943528bb68ceda3aade2eb/grammar.js#L792

carlosala commented 2 years ago

I don't think so, as javascript highlights properly this example. Should be easy to fix.

Wilfred commented 2 years ago

This issue is affecting my project too. The code true ? 1 : 2; is parsed as:

{Node program (0, 0) - (1, 0)}
  {Node expression_statement (0, 0) - (0, 13)}
    {Node ternary_expression (0, 0) - (0, 12)}
      {Node true (0, 0) - (0, 4)} "true"
      {Node ? (0, 7) - (0, 7)} ""
      {Node number (0, 7) - (0, 8)} "1"
      {Node : (0, 9) - (0, 10)} ":"
      {Node number (0, 11) - (0, 12)} "2"
    {Node ; (0, 12) - (0, 13)} ";"

Note that the position of the ? token is empty.

Wilfred commented 2 years ago

Interestingly the ternary_expression rule is inherited from the javascript grammar, but it doesn't have this problem. true ? 1 : 2; is parsed in JS as:

{Node program (0, 0) - (1, 0)}
  {Node expression_statement (0, 0) - (0, 13)}
    {Node ternary_expression (0, 0) - (0, 12)}
      {Node true (0, 0) - (0, 4)} "true"
      {Node ? (0, 5) - (0, 6)} "?"
      {Node number (0, 7) - (0, 8)} "1"
      {Node : (0, 9) - (0, 10)} ":"
      {Node number (0, 11) - (0, 12)} "2"
    {Node ; (0, 12) - (0, 13)} ";"
Wilfred commented 2 years ago

Given that the grammar rule in the typescript parser reuses the JS rule, I'm guessing it's an issue in scanner.h.

Wilfred commented 2 years ago

Removing this loop seems to fix it:

https://github.com/tree-sitter/tree-sitter-typescript/blob/e8e8e8dc2745840b036421b4e43286750443cb13/common/scanner.h#L175-L180

Wilfred commented 2 years ago

Unfortunately the ? is an anonymous token _ternary_qmark, and the tests don't seem to include any way to verify positions of any token.

carlosala commented 2 years ago

The main issue with this is that ? in javascript is only used for ternary operator, whether in typescript is also used for optional arguments in interfaces, etc.

cmdcolin commented 2 years ago

not sure if related to this issue but I found something sort of similar with

export function foo() {
  return {
    ...(1 > 0
      ? {
          onClick: () => alert('hello'),
        }
      : {}),

    label: 'hello',
    onClick: () => alert('hello'),
  }
}

Screenshot from 2022-06-28 09-06-17

With 1>0 it looks ok to me, but replacing 1>0 with true makes the highlighting go away.

Screenshot from 2022-06-28 09-06-35

This appears specific to .ts file type, .js seems ok

tom95 commented 2 years ago

Hi @cmdcolin, I was not able to reproduce the error, potentially because I have a different highlights configuration than you?

If possible, you could try and run tree-sitter with this potential fix for this issue https://github.com/tree-sitter/tree-sitter-typescript/pull/215 and see if it does indeed also fix the issue you reported.

carlosala commented 2 years ago

I'll check that later today or tomorrow, we should push it because it's really shitty to not have proper highlighting

cmdcolin commented 2 years ago

@tom95 I think it is reproducible on a pretty stripped down config. i tried with neovim nightly (0.8 branch) and 0.7.2 and it's the same

the config is just including nvim-treesitter and nothing else

how would i test out the branch?

extra info on version/checkhealth just for ref

out2.txt out.txt

tom95 commented 2 years ago

@cmdcolin looking at neovim docs, I believe you should be following these steps to add a custom version of typescript highlighting: https://github.com/nvim-treesitter/nvim-treesitter#adding-parsers

As URL, you should specify https://github.com/tom95/tree-sitter-typescript and as branch ternary-range-200.

cmdcolin commented 2 years ago

I actually ran ":TSInstall typescript" and it fixed my issue (presumably this updated the typescript tree-sitter module) so my pre-mentioned issue with the codesnippet is probably unrelated to this issue possibly....will hide comments