Add syntax-highlighting for types in variable-declarations

[josteink]

As mentioned in #112 (comment):

so far it handles the following cases fine:

• let a: SomeType;
• private b: SomeType;
• private someFunc(var: SomeType) {
• private array: SomeType[]
• private generic: SomeType<Foo>
• private genericArray: SomeType<Foo>[]
• function testFunc(): SomeType<> {>
  ...

For now I'm letting this change live in a feature-branch to see if it causes any obvious negative/incorrect highlightings.

If it doesn't, I think I may be leaning towards wanting this merged to master, because it really helps distinguish types in type-heavy projects.

I've been giving this branch a go as my "main" typescript-mode for over a month now, and I'm considering it a 100% positive change. Whenever I'm back at master I'm aching for those highlighted type-declarations, and I've yet to find any adverse or weird highlightings in any idiomatic TypeScript-code I've touched.

I'm all for the conceptual purity of having highlighting based on an actual, proper syntax-analysis (as promoted by @lddubeau ), but in the mean time I'm also realistic enough to see that's not going to happen anytime soon.

That leaves the pragmatic parts of me wanting this noticable improvement, even though it's not done 100% "proper".

In short: I'm now 100% in support of this change.

If anyone has any idiomatic code which gets highlighted incorrectly based on these change, please do let me know.

Otherwise I'd now like to start the count-down clock for getting this branch merged.

Any disagreements on that part? Anyone who would feel highly upset over this change?

Not to mention: Any actual constructive criticism of the code here is very much wanted.

cc: @lddubeau , @tam5, @Fuco1

[lddubeau]

There's no pursuit of "conceptual purity" on my part.

I've merely pointed out in order to accurately fontify TypeScript code, you need a parser that produces an AST (or something just as complex as a parser that produces an AST).

The main issue is that, while I do find fontification useful, I find that it is of relatively limited usefulness even when it is done perfectly. I also find that the usefulness of fontification is not linear with the amount of highlighted material or (more generally) with the complexity of the fontification. Quite the contrary, I find that I quickly reach a point where more highlighting (or more complex highlighting) reduces the overall usefulness of the fontification. Level 4 fontification in typescript-mode operates past that point. Here's a quick graph illustrating what I mean:

In addition, if I cannot trust the highlighting to be 100% correct, this dramatically harms the usefulness of the fontification. This is not some rarefied theoretical conceit but a practical consequence of using a tool that I cannot trust to correctly inform me as to what's what. I am very much aware of the cognitive burden that I have to pay every time I have to overrule what my editor shows. Or the cognitive burden that I have to pay every time I cannot be sure that those bits painted in some specific color really cover all the cases they should be covering.

Fontification derived from an AST would take care only of the accuracy of the fontification. In particular, the usefulness vs complexity curve would not change.

---

As long as highlighting at level 3 and below remains sane, I've stopped caring about what happens at level 4 and above. I won't block changes to level 4 and above nor approve them. I regularly have to work in contexts where I get less detailed fontification than provided by level 4. I never miss it.

[josteink]

Thanks for the feedback @lddubeau. That's (as always) very thorough and highly informative, and definitely clears up any misconceptions I've had about your opinions about these changes.

As long as highlighting at level 3 and below remains sane

Yeah. I don't think anyone plans on making any major changes at those levels anytime soon. If so, I'll make sure to keep you tightly in the loop.

[josteink]

Note to self: This PR requires tests before merging to master!

[tam5]

@josteink I've checked out the branch and tested the cases I could think of off the top of my head, seems to work great as far as I can tell.

I also understand @lddubeau's thoughts and have noticed some similar points of view in the issues which is why I think it's good to keep level 3 as is. As the culprit who introduced level 4 in the first place, I guess I'll just offer that I personally don't rely on 'correctness' as much in order for the font-lock to be useful. For me, the usefulness is simply in giving the code a bit more visual structure, so similar structures pop and it's easier to focus on certain parts at a glance.

TLDR you have my +1

[josteink]

Thanks for the feedback!

I’ve so far found one thing not handled (sufficiently) well by these changes: namespaced classes, like Office.EmailAddressDetails.

When I get around to adding tests, I’ll try to make sure that gets included as well.

[josteink]

Ok. I've refactored the code slightly, added more tests and added support for namespaced type-names as well.

Unless anyone objects, I'll consider this feature-complete, test-complete and generally speaking the final revision before I merge to master.

Get ready for a shinier level4 sometime soon 😄