typescript.el icon indicating copy to clipboard operation
typescript.el copied to clipboard

Single-line object-constructors are fontified incorrectly when prefixed on a line with a generic operation

Open josteink opened this issue 4 years ago • 5 comments

Take the line of typical REST service boilerplate:

return this.httpClient.post<IContactOrPerson[]>(url, { Name: name, ContactId: contactId })

In Emacs this gets fontified incorrectly: image

Modified to include a line-break it almost looks good, but still has issues:

image

This can be verified using the following ERT test-case:

(ert-deftest font-lock/type-name-generics-negatives ()
  "Do not fontify other common uses with : in syntax."

  (test-with-fontified-buffer
      "return this.httpClient.post<IContactOrPerson[]>(url, { Name: name, ContactId: contactId });"
    (should (not (eq (get-face-at "url" 'font-lock-type-face))))
    (should (not (eq (get-face-at "name" 'font-lock-type-face))))
    (should (not (eq (get-face-at "contactId" 'font-lock-type-face))))))

From inspecting the font-highlighting using font-lock-studio the following rule is singled out as the culprit: https://github.com/emacs-typescript/typescript.el/blob/bc37bc4b1527f0ad382bbdcecf2ea3623717aebb/typescript-mode.el#L1992-L1998

@tam5 : You were the one who introduced this code. Can you take a look at this case and see if you can make it behave better? (And at least pass my ERT test-case?)

josteink avatar Mar 09 '20 08:03 josteink

I've looked into this some more, and this code contains elements from both PR #110 and #119, but as far as I can see, the error was already there in #110.

josteink avatar Mar 09 '20 08:03 josteink

Looking futher into this test-case it only seems to fail when the generic type-argument contains an array.

Consider the following examples:

goodTestCase<Test>(boo: hoo, zoo:doo ) {
}

badTestCase<Test[]>(boo: hoo, zoo:doo ) {
}

This renders as following:

image

This makes the error less severe, as not all generic operations are affected, but it's still annoying.

Edit: Tracing this fontification code using font-lock-studio, the "generics support"-code gets repeated again and again and again for component following the Test[] expression. Clearly the regexp used as a the "anchor" for this rule ends up in a too wide" mode when encountering the array-declaration..

josteink avatar Mar 09 '20 08:03 josteink

After I merged #129 to git master, I'm able to make this look "good" with the following patch:

@@ -1994,8 +1994,8 @@ This performs fontification according to `typescript--class-styles'."
 
     ;; generics support
     ,(list
-      (concat typescript--name-re "\\s-*" "<\\s-*" typescript--name-start-re)
-      (list (concat "\\(" typescript--name-re "\\)\\(\\s-*>[^<]*\\)?")
+      (concat typescript--name-re "\\s-*" "<\\s-*[A-Z]")
+      (list (concat "\\(" typescript--type-name-re "\\)\\(\\s-*>[^<]*\\)?")
             '(backward-char)
             '(end-of-line)
             '(1 font-lock-type-face)))

I'll create a PR for it.

josteink avatar Mar 09 '20 08:03 josteink

Update: While my above patch solves the simple test-cases of goodTestCase and badTestCase, it doesnt work for more complex scenarios:

image

Now it's just weird. I'll leave it at this for now, and hopefully you can work this out @tam5.

josteink avatar Mar 09 '20 08:03 josteink

I have seen this, not ignoring it. And @josteink you are right about the ending being the issue. It is like this I think so that we can handle nested generics i.e. method<Array<T>>(). Will need to think of a way to accommodate both.

tam5 avatar Mar 12 '20 22:03 tam5

Typescript-support is now part of core Emacs, where this is fixed. Closing this issue.

josteink avatar Oct 11 '23 07:10 josteink