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

Difference in highlighting some TypeScript React files (Sublime vs. Syntect) #97

Closed
slimsag opened this issue Aug 30, 2017 · 8 comments
Closed

Comments

@slimsag
Copy link
Contributor

slimsag commented Aug 30, 2017

Syntect fails to properly syntax highlight some TypeScript React (tsx) files in contrast to Sublime Text 3 which highlights them fine.

To reproduce this:

  1. You'll need to first download Microsoft's official TypeScriptReact.tmLanguage file and save it somewhere.
  2. Convert that .tmLanguage file to a .sublime-syntax file, using Sublime Text 3's builtin Plugin Development: Convert Syntax to .sublime-syntax command (important: only shows up when viewing .tmLanguage files)
  3. Save the new .sublime-syntax file into e.g. testdata/Packages/TypeScript/TypeScriptReact.sublime-syntax
  4. Run make assets.

Alternatively, my fork's branch here has the above steps done so you don't have to perform them. The .sublime-syntax file is also in the root of the repo.

Once you've done the above or cloned my fork,

  1. Save this BasePicker.tsx test file anywhere.
  2. Save TypeScriptReact.sublime-syntax into e.g. ~/Library/Application\ Support/Sublime\ Text\ 3/Packages/User/ on Mac (and uninstall the TypeScript Sublime plugin, to prevent any concern over the tmLanguage -> sublime-syntax conversion potentially being the issue).

When the file is viewed in Sublime, it looks like this:

image

In contrast, the HTML rendered by cargo run --example synhtml ./BasePicker.tsx > out.html ends up looking like this:

image

Of course, the above two look different due to the theme in use, but notice that even just in the syntect result alone there are two different colors in use for the attributes:

image

I found that changing this line of code to remove the addition operator:

                suggestedDisplayValue={ suggestedDisplayValue }
-               aria-activedescendant={ 'sug-' + this.suggestionStore.currentIndex }
+               aria-activedescendant={ this.suggestionStore.currentIndex }
                aria-owns='suggestion-list'

Fixes the highlighting in Syntect:

image

If it wasn't for Sublime rendering this file with the same sublime-syntax, I would assume the syntax file was broken somehow -- but given the difference between Sublime and Syntect's rendering, I assume this must be a bug in Syntect (but I do not understand Syntect well enough to even begin tracking it down :) )

@trishume
Copy link
Owner

Interesting, I wonder if this is related to the ASP syntax tests still failing. I never got around to fixing all of those. See #51.

@robinst
Copy link
Collaborator

robinst commented Feb 12, 2018

(For anyone wondering, v2.0.0 still has this issue, just tested.)

@robinst
Copy link
Collaborator

robinst commented Apr 26, 2018

Was wondering whether #146 changes this, it doesn't.

But what I noticed is that cargo run --example syncat doesn't have the same problem, only synhtml. I wonder what they do differently, it might give a clue as to what the problem is. But it also might just be a bug in syncat or synhtml :)..

@trishume
Copy link
Owner

@robinst huh weird, yah probably a bug in synhtml.

@trishume
Copy link
Owner

Okay I diagnosed this hoping to fix it before the next release, unfortunately it's tricky.

Basically synhtml uses the nonewlines mode, which uses hacky regex rewriting, which is incomplete. The \s metacharacter matches newlines and we don't do anything about it.

That causes this rule to fail to match on the \s+ in this regex:

  jsx-tag-in-expression:
    - match: |-
        (?x)
        (?<=[({\[,?=>:*]|&&|\|\||\?|\Wreturn|^return|\Wdefault|^)\s*
        (?!<\s*[_$[:alpha:]][_$[:alnum:]]*((\s+extends\s+[^=>])|,)) # look ahead is not type parameter of arrow
        (?=(<)\s*
        ([_$a-zA-Z][-$\w.]*(?<!\.|-))
        (?=\s+(?!\?)|/?>))

A minimal test case file that differs in parsing (in parsing on the first line, in highlighting with base16-ocean-dark only on the second) is:

<div
ref="hi"

For now the workaround is just to use newlines mode. It's just better.

@robinst
Copy link
Collaborator

robinst commented Apr 29, 2018

Ah nice, that makes sense. Should we also make synhtml use newlines mode by default?

@trishume
Copy link
Owner

@robinst probably, and maybe in the mean time change the docs to suggest that nonewlines is known to be mostly working but a little bit broken and point to this issue.

I wonder if eventually a mode could be added to fancy-regex to hallucinate a \n at the end of strings, although that may run into the problem of regex not supporting it.

slimsag added a commit to sourcegraph/syntect_server that referenced this issue May 10, 2018
slimsag added a commit to sourcegraph/syntect_server that referenced this issue May 10, 2018
* cargo update -p syntect

* README: update with new server output

* switch to newlines mode

See trishume/syntect#97 (comment)
@Enselic
Copy link
Collaborator

Enselic commented Nov 10, 2021

A similar problem still exists in latest syntect master with the latest TypeScriptReact.tmLanguage:

cargo run --example syncat -- ~/Downloads/BasePicker.tsx --no-newlines

Screen Shot 2021-11-10 at 06 33 43

However, as before, not using nonewlines behaves correct:

cargo run --example syncat -- ~/Downloads/BasePicker.tsx

Screen Shot 2021-11-10 at 06 36 35

And by now there is clear documentation that avoiding nonewlines is recommended.

So I think we can go ahead and close this issue. Feel free to reopen if you disagree, of course.

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

No branches or pull requests

4 participants