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

Syntax highlighting for types #104

Closed
xged opened this issue Mar 30, 2019 · 21 comments
Closed

Syntax highlighting for types #104

xged opened this issue Mar 30, 2019 · 21 comments

Comments

@xged
Copy link

xged commented Mar 30, 2019

I see types in type aliasing (before =) being highlighted (font-lock-type-face), but not in other places when they are used (default). Would like to see types being highlighted wherever they are.

@josteink
Copy link
Member

josteink commented Apr 4, 2019

That would be nice, but it would require typescript-mode to posess a full parse-tree/AST of the buffer at hand to know first hand if something is a type or not.

Doing this with pure regexps (like we are doing now) is simply not possible, without causing other things to be accidentally highlighted like types without actually being so.

Basically, we agree, but it's hard and would take a total rewrite. Pull requests welcome?

@AdamNiederer
Copy link

AdamNiederer commented May 22, 2019

I've recently implemented a somewhat-sophisticated syntax highlighting system for ng2-mode, which uses typescript-mode as its parent. It's mostly regex-based, except for types in import blocks and, eventually, identifiers in destructuring patterns.

The only requirement users need follow is starting types with capital letters or symbols and starting non-types with lowercase letters most of the time (which most good typescript code does anyway). Perhaps we could add some of the less-risky stuff there?

In the mean time, you can also just enable ng2-mode in your normal typescript files - it works with lsp et al and the only angular-specific stuff is in the keybinds.

@josteink
Copy link
Member

josteink commented Jun 1, 2019

This sounds great, and sorry for not taking the time to look into it properly right now.

I'm on paternity leave and kids take all my time, but if nobody else has picked this up by the time I'm back at work, I will definitely dig into this 100%.

@josteink
Copy link
Member

Back from paternity leave now.

The only requirement users need follow is starting types with capital letters or symbols and starting non-types with lowercase letters most of the time (which most good typescript code does anyway). Perhaps we could add some of the less-risky stuff there?

I agree with this. That's a fairly simple solution which should work well with idiomatic TypeScript-code without requiring a full rewrite.

Anyone have any objections to such an approach?

@lddubeau
Copy link
Collaborator

@josteink Welcome back!

Anyone have any objections to such an approach?

I do. There are multiple contributors to this project, so I don't know how consistent we've been in this overall, but my approach with regards to limitations has always been "this case is not supported because the TypeScript code in the case is complex beyond the point that we can support". I'm taking about syntactic complexity here. Examples in decreasing order of complexity:

class Foo<T extends Bar = Fnord<string>> extends Baz {
class Foo<T extends Bar = Fnord> extends Baz {
class Foo<T extends Bar> extends Baz {
class Foo<T> extends Baz {
class Foo extends Baz {
class Foo {

The difference in complexity in the lines above is not a matter of personal preference but is a characteristic of the code itself.

Style, on the other hand, is a matter of preference. I'm not fond of an approach that works, or fails to work, based on whether the user's preference agrees with the one baked into the mode.

@josteink
Copy link
Member

Style, on the other hand, is a matter of preference. I'm not fond of an approach that works, or fails to work, based on whether the user's preference agrees with the one baked into the mode.

That's fair enough.

Would you be open to the idea of allowing this based on a preference/defcustom... Or probably better: a custom (non-default?) syntax-highlighting level?

@AdamNiederer
Copy link

Using the built-in font-lock-maximum-decoration to gate different modes of fontification is probably still within the spirit of the setting. typescript.el's highlighting is approximately enough to satisfy level 2, whereas the style-based highlighters in ng2-ts-mode satisfy level 3, for well-styled typescript. For typescript outside the style norms, it will gracefully degrade to a level between 2 and 3.

@lddubeau
Copy link
Collaborator

@josteink

Would you be open to the idea of allowing this based on a preference/defcustom... Or probably better: a custom (non-default?) syntax-highlighting level?

It definitely must be able to be turned off. What @AdamNiederer suggested with the font lock levels is seems to me to be right.

@josteink
Copy link
Member

@lddubeau: I guess I expressed myself somewhat imprecise, but yes I what @AdamNiederer says is exactly what I meant.

If you're happy with that, I guess it's OK for @AdamNiederer to move ahead with a PR 😃

@lddubeau
Copy link
Collaborator

@josteink Really, the only way to address my objection would be to implement it in a way that does not use stylistic shortcuts. The ability to turn it off while still having some highlighting only makes it less objectionable than it would otherwise be. But in this project my objections are not the final word, so the PR can be submitted, over my objection.

@josteink
Copy link
Member

josteink commented Aug 1, 2019

@lddubeau I'm a strong proponent of consistency. And you are probably the biggest contributor to this project both wrt to technical implementations and good, consistent considerations about those.

If you have reasonable objections, I have no desire to override those, and risk make you feel like your input and contributions are not fully and deeply appreciated here.

Basically I'm just trying to be a "diplomat" and make "everyone" happy. Failing at that, I'd rather have happy regular contributors than trying to please other "random people" filing issues.

Sorry @AdamNiederer and @xged. I guess for now, this is case closed until we can find a better approach. 🤷‍♂

@AdamNiederer
Copy link

I'd like to chime in and say that perfect is the enemy of good. Do we reasonably forsee the addition of syntactic highlighting to the project? Such a system is going to require more code than was contributed to this project (excluding tests) in the past year, and syntactic highlighting in typescript.el is not well positioned compared to offerings from other code editors.

@josteink
Copy link
Member

josteink commented Aug 6, 2019

syntactic highlighting in typescript.el is not well positioned compared to offerings from other code editors.

Agreed.

Do we reasonably forsee the addition of syntactic highlighting to the project? Such a system is going to require more code than was contributed to this project (excluding tests) in the past year

Given the generally low activity this project have, I honestly don't foresee this happening ever in its lifetime.

But it's not just typescript-mode: It's a common problem for several major-modes in Emacs trying to support modern, complex languages. They are all "awaiting" this syntactic rewrite which will "solve everything".

But IMO for that to happen, some catalyst needs to suddenly occur, and spark life into new and better major-modes in the wider Emacs ecosystem.

I don't really think I'm overly cynical about this point. I'd rather say it's a somewhat sad, but still realistic observation.

So what do we do?

I'd like to chime in and say that perfect is the enemy of good.

I tend to agree with this statement, and I'm personally OK with convention-based highlighting rules as long as the convention is 99.9% universal.

On the flip side, I do respect people like @lddubeau's thoroughness and sense of finer details, both in terms of code-correctness but also when thinking the bigger picture.

So yeah, I honestly don't know what the best solution is.

@jcs090218
Copy link
Contributor

jcs090218 commented Oct 22, 2019

Hi, I found this thread and start it a new project here. I attempt to create an AST for certain modern programming languages. Do you think this help and maybe could resolve the issue like this? 😕

@zhenwenc
Copy link

The syntax highlighting used to be acceptable, even though it wasn't perfect, but definitely becomes horrible recently! Especially in JSX buffers (highlighting Input type of <Input type="..." /> with font-lock-type-face, because of the <, wat!). I think it's caused by #110 .

I think in general, regular expression isn't powerful enough, especially for languages (with inconsistent 💩 syntax) like JS. Not sure if its possible, but maybe LSP can help with this aspect. However, typescript-mode shouldn't be responsible for solving this problem.

So please stop messing up the syntax regular expressions..

@AdamNiederer
Copy link

Not sure if its possible, but maybe LSP can help with this aspect. However, typescript-mode shouldn't be responsible for solving this problem.

LSP does not handle syntax highlighting because it must be a low-latency operation, and both leaving it in the address space in of the editor and designing it around the editor's display engine is the lowest-latency way of doing it.

@tam5
Copy link
Collaborator

tam5 commented Nov 20, 2019

@zhenwenc I am sorry recent changes have been disrupted to you. I think your case is one we overlooked when implementing generics. By checking out an older revision I can see that previously, jsx blocks would not get any highlighting. I will update our regex to account for this case, so we support the generic highlighting and we restore the jsx highlighting to what it was before, although in my opinion the lack of highlighting is less than ideal, so please read on.

I do recommend you check out web-mode which does a great job of handling file types that are really a 'mix' of other modes, giving you the best of both worlds by highlighting portions of the buffer with the appropriate modes. Personally, I use this for jsx/tsx files which is why I never even noticed this issue to begin with.

Additionally, I would like to point out that the recent additions were added as a new font lock level (4), which means you can always opt out of these changes with something like this:

(setq font-lock-maximum-decoration 3)

Anyway, #119 should address the problem raised

@josteink
Copy link
Member

I think in general, regular expression isn't powerful enough, especially for languages (with inconsistent 💩 syntax) like JS. Not sure if its possible, but maybe LSP can help with this aspect. However, typescript-mode shouldn't be responsible for solving this problem.

Uh. typescript-mode is the piece of code repsonsible for handling syntax-highlighting and indentation for typescript-files in Emacs by default. LSP does not help with anything here.

While I agree regexp-based analysis is woefully insufficient for 100% correct highlighting in all cases, I don't think that's an argument for not trying to improve what we have.

Especially in JSX buffers

Don't you mean TSX? Because why would you use typescript-mode for JSX files?

Anyhow, you may be perfectly right that the current code is probably not optimally tested in TSX-mode, and that the major focus of the developers is plain typescript.

@zhenwenc: If you want this to improve, maybe you can submit some TSX test-cases, so that future changes and improvements to regular typescript doesn't break TSX?

@zhenwenc
Copy link

@josteink Thanks for the reply.

My point is, since it's known that there is no way to provide correct highlighting in all cases, as a major mode wouldn't it better to provide a conservative solution instead? For example in the past, only highlight things like keywords, which are 99% correct in all cases, was already super helpful and good enough.

LSP does not help with anything here.

Don't you mean TSX? Because why would you use typescript-mode for JSX files?

Yes, I mean TSX. And no, I do use typescript-mode for JS and JSX files my config.

@zhenwenc
Copy link

Hi @tam5 , thanks for your reply! Great thanks for your effort of improving typescript-mode, much appreciated! Apology if my last reply sound like bad tone.

@josteink
Copy link
Member

Closing issue due to no activity in months. Hope everyone is happy 😃

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

7 participants