Fontification of parameters in function-declaration differs from method-declaration

[josteink]

Consider the following code:

export function someFunction(param1: Type1, param2: Type2): string {
    // TODO
}

class SampleClass {
    someMethod(param1: Type1, param2: Type2): string {
        // TODO
    }
}

someFunction will have the params (and types!) highlighted using font-lock-variable-name-face, while someMethod will have the params highlighted using the default-face:

This has at least 2 errors:

• highlighting is inconsistent.
• In the case of functions, types are highlighted as variable-names.

Edit: Lambda's are consistent with methods, in that they don't get much highlighting at all, as seen in the following example:

let someLambda = (param1: Type1, param2: Type2): string => {
    // TODO
};

[josteink]

If my git blame skills are to be trusted, this bug has actually been there since the initial commit. Oh my!

If so, the following lines can be blamed:

typescript.el/typescript-mode.el

Lines 1878 to 1886 in 1bafd27

	;; formal parameters
	,(list
	(concat
	"\\_<function\\_>\\(\\s-+" typescript--name-re "\\)?\\s-*(\\s-*"
	typescript--name-start-re)
	(list (concat "\\(" typescript--name-re "\\)\\(\\s-*).*\\)?")
	'(backward-char)
	'(end-of-line)
	'(1 font-lock-variable-name-face)))

That's really so overly broad, I might consider removing it all together.

Edit: Yup. Commenting out those lines, makes things consistent, although somewhat boring.

Will probably need to sleep on it.

[tam5]

you mentioned 2 issues, there is a different challenge for each:

In the case of functions, types are highlighted as variable-names

This is basically due to the lack of support for highlighting types via the : Thing syntax. And that topic probably covered in #104, which I'll add a comment to.

The other issue

highlighting is inconsistent

is due to lack of coverage for all cases of a function declaration, as you pointed to above. The lambda function should actually be quite easy to implement. The 'class function' would likely be much harder, as it would require understanding the context so as not to be confused with a method invocation.

I think, we might be able to do something similar to this https://github.com/emacs-typescript/typescript.el/blob/master/typescript-mode.el#L1539-L1549 to solve it, but not really sure. I can try to give it a spin

[josteink]

This issue is somewhat alleviated by a change I'm trying out now.

It's a font-lock change based on previous discussion about using popular conventions for syntax-highlighting, more than just the syntactical context.

Overly simplified it recognizes anything following a colon (:) which starts with a upper-case letter as a type. In practice it's a bit more restrictive than that, but 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<> {

It may not be production-quality and handle all cases of everything, but at least it should handle fairly common things, and as such should provide immediate improvements to most typescript projects.

If I remember correctly, I understand that you @tam5 was considering something like this too? I know @lddubeau is against such a change due to it being convention-based, not syntax-based, but I'd still like to at least propose these changes in code-form, so that people can try it in practice.

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.

Feedback appreciated.

[Fuco1]

I cloned the repo now and was literally about to begin implementing what you describe.

I've also found some other inconsistencies, for example null is highlighted as a keyword not a constant (even though you have the regexps for it, but the ordering of the font-lock rules is incorrect).

There's a host of advanced font-locking magic that can handle almost any syntax imaginable, I've written about these in the past:

• https://fuco1.github.io/2017-06-11-Font-locking-with-custom-matchers.html
• https://fuco1.github.io/2017-06-01-The-absolute-awesomeness-of-anchored-font-lock-matchers.html

I quite enjoy hacking around font-lock. Maybe it would be helpful to create some roadmap for font-locking so we could split the work in some reasonable manner (so as not to duplicate effort) and slowly get it done.

[Fuco1]

A quick demonstration on how the anchored matcher can be used to fontify types in function:

Before

After

(font-lock-add-keywords
   nil
   '(("function"
      (":\\(.*?\\)\\(,\\|)\\)"
       (save-excursion (or (search-forward ")" nil t) (point-max)))
       (re-search-backward ")")
       (1 font-lock-type-face))
      (")[[:space:]]*:\\(.*?\\)\\({\\|$\\)"
       (save-excursion (or (search-forward "{" nil t) (point-max)))
       nil
       (1 font-lock-type-face)))))

[josteink]

Hey @Fuco1.

Sorry about the slow reply here, but I’m currently really busy with a few things at work.

I promise to review your changes first thing when I have a spare minute 🙂

[josteink]

Hey @Fuco1.

Your code here looks really simple. Right now there's a few things it doesn't do "right", but that's no reason to reject this idea for how to solve it outright.

I like the simplicity of the solution.

There's a few I would like to see addressed though:

• built-in types and values (null, number, string) should be highlighted consistently with the rest of typescript-mode, and not be given font-lock-type-face.
• some syntax-elements are currently highlighted using font-lock-type-face as well (|, < ,> etc). IMO this doesn't really look that good.

If we could somehow fix those 2 issues, I would be 100% for a solution like the one you are proposing.

[Fuco1]

Personally, I prefer the built-in types and values to be fontified as types when in type context. It helps to distinguish the use-case. This is actually one thing that I find a bit confusing when skimming code and "looking for colors" and they pop up where I would not expect them and the reason I looked up this issue in the first place.

We can probably introduce something like typescript-type-builtin face which could be configured by default as you propose but leave the user an option to change it.

In general it's a good practice to have your own set of faces which inherit by default from the emacs built-in "font lock faces" but leave the user a place to customize. php-mode does this very well.

About the second point, I think I agree there.

My code is simplistic but was more ment as a demonstration than actual implementation. I'm sure it breaks in various other scenarious (arrow functions being one).

[josteink]

I guess none of us are technically speaking wrong here: string, number, null etc are built ins, but they are also types.

Which makes this more of a preference than something objectively correct.

We can probably introduce something like typescript-type-builtin face which could be configured by default as you propose but leave the user an option to change it.

I think this sounds like a good solution. I support this.

About the second point, I think I agree there. My code is simplistic but was more ment as a demonstration than actual implementation.

Fair enough. I guess we can leave "fixing" that until we have the rest of the stuff specced out and agreed upon.

[tam5]

some syntax-elements are currently highlighted using font-lock-type-face as well (|, < ,> etc). IMO this doesn't really look that good.

Definitely agree that this would need to be fixed.

We can probably introduce something like typescript-type-builtin face which could be configured by default as you propose but leave the user an option to change it.

This is something I had entertained once before. Maybe it's time to revisit #78

[Fuco1]

I'm working on some of these in #170

[Fuco1]

I think this is mostly fixed with #170 (and other couple PRs) merged. I would suggest closing this generic issue and we can later open new ones for specific instances of new issues. @josteink WDYT?

[josteink]

Sure. Will close.