bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

[-- Attachment #1: Type: text/plain, Size: 9441 bytes --]

When compiling a TypeScript project using tsc (or other tooling) from Emacs,
compilation errors and warnings are not highlighted by compilation-mode.

Consider the following output:

src/resources/document.ts:140:22 - error TS2362: The left-hand side of 
an arithmetic operation must be of type 'any', 'number', 'bigint' or an 
enum type.

140       return `File-${new Date() * 1}${ext}`;
                          ~~~~~~~~~~

This output should cause compilation-mode to highlight the error and 
provide code-navigation.

I know we explicitly added support for this to typescript.el back in the 
days, but I'm not
sure what the "right" thing to do is now that typescrip-ts-mode is a 
first class Emacs citizen.

Should we add compilation-mode patterns directly to the major-mode, or 
should we provide
patches to compile.el instead?

Does anyone have any strong opinion or guidance here?

--
Jostein


In GNU Emacs 29.0.60 (build 3, x86_64-pc-linux-gnu, GTK+ Version
  3.24.36, cairo version 1.17.6) of 2023-01-26 built on thinkpad-t14s
Repository revision: f8c95d1a7681e861fc22d2a040cda0ddfe23eff4
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12201007
System Description: Arch Linux

Configured using:
  'configure --with-json --with-tree-sitter'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: JSON

Minor modes in effect:
   global-git-commit-mode: t
   magit-auto-revert-mode: t
   electric-pair-mode: t
   highlight-symbol-mode: t
   flycheck-mode: t
   editorconfig-mode: t
   company-mode: t
   which-function-mode: t
   helm-mode: t
   helm-minibuffer-history-mode: t
   shell-dirtrack-mode: t
   helm--remap-mouse-mode: t
   async-bytecomp-package-mode: t
   delete-selection-mode: t
   global-auto-revert-mode: t
   yas-global-mode: t
   yas-minor-mode: t
   global-nlinum-mode: t
   nlinum-mode: t
   ido-yes-or-no-mode: t
   override-global-mode: t
   server-mode: t
   global-hl-line-mode: t
   pixel-scroll-precision-mode: t
   doom-modeline-mode: t
   tooltip-mode: t
   global-eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   column-number-mode: t
   line-number-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   hs-minor-mode: t

Load-path shadows:
/home/jostein/.emacs.d/elpa/transient-20230107.1528/transient hides 
/home/jostein/build/emacs/lisp/transient

Features:
(shadow sort mail-extr emacsbug help-fns radix-tree cl-print
json-ts-mode dired-aux textsec uni-scripts idna-mapping ucs-normalize
uni-confusable textsec-check gnutls network-stream url-http url-gw nsm
url-cache url-auth goto-addr misearch multi-isearch flyspell ispell
magit-extras magit-submodule magit-obsolete magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
magit-diff smerge-mode diff git-commit log-edit message sendmail
yank-media rfc822 mml mml-sec epa derived epg rfc6068 epg-config
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mailabbrev gmm-utils mailheader pcvs-util magit-core
magit-autorevert magit-margin magit-transient magit-process with-editor
magit-mode transient magit-git magit-base magit-section crm helm-command
helm-elisp helm-eval edebug helm-info vc-git diff-mode vc-dispatcher
disp-table bug-reference winner ffap tramp-archive tramp-gvfs
tramp-cache time-stamp zeroconf dbus face-remap executable markdown-mode
color add-log elec-pair typescript-ts-mode js c-ts-common cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs treesit vc-svn ido-completing-read+ memoize minibuf-eldef
elisp-slime-nav paredit highlight-symbol flycheck editorconfig
editorconfig-core editorconfig-core-handle editorconfig-fnmatch
company-oddmuse company-keywords company-etags etags fileloop generator
company-gtags company-dabbrev-code company-dabbrev company-files
company-clang company-capf company-cmake company-semantic
company-template company-bbdb company eglot external-completion array
jsonrpc ert ewoc debug backtrace flymake-proc flymake warnings
which-func hideshow eww url-queue thingatpt shr pixel-fill kinsoku
url-file svg xml dom puny mm-url gnus nnheader gnus-util mail-utils
range mm-util mail-prsvr helm-imenu helm-mode helm-misc helm-files
image-dired image-dired-tags image-dired-external image-dired-util xdg
image-mode dired dired-loaddefs exif tramp tramp-loaddefs trampver
tramp-integration cus-edit pp cus-load wid-edit files-x tramp-compat
shell parse-time iso8601 ls-lisp helm-buffers helm-occur helm-tags
helm-locate helm-grep helm-regexp helm-utils helm-help helm-types helm
helm-global-bindings helm-easymenu helm-core async-bytecomp helm-source
helm-multi-match helm-lib async pcase imenu ob-plantuml org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint org-pcomplete
pcomplete org-list org-footnote org-faces org-entities time-date
noutline outline icons ob-emacs-lisp ob-core ob-eval org-cycle org-table
ol org-fold org-fold-core org-keys oc org-loaddefs find-func cal-menu
calendar cal-loaddefs org-version org-compat org-macs format-spec delsel
autorevert filenotify yasnippet nlinum linum ido-yes-or-no advice ido
edmacro kmacro use-package-bind-key bind-key easy-mmode xref project
server hl-line pixel-scroll cua-base compile-eslint compile
text-property-search comint ansi-osc ansi-color ring doom-modeline
doom-modeline-segments doom-modeline-env doom-modeline-core
all-the-icons all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons shrink-path
rx f f-shortdoc s dash compat dracula-theme cl-extra help-mode
use-package-ensure use-package-core finder-inf yasnippet-autoloads
ido-yes-or-no-autoloads elisp-slime-nav-autoloads cmake-mode-autoloads
flycheck-autoloads pkg-info-autoloads magit-autoloads
all-the-icons-autoloads crontab-mode-autoloads powershell-autoloads
doom-modeline-autoloads undo-tree-autoloads rust-mode-autoloads
magit-section-autoloads paredit-autoloads dracula-theme-autoloads
cargo-autoloads yaml-mode-autoloads helm-autoloads popup-autoloads
queue-autoloads nlinum-autoloads bmx-mode-autoloads company-autoloads
git-commit-autoloads multiple-cursors-autoloads dap-mode-autoloads
lsp-treemacs-autoloads treemacs-autoloads cfrs-autoloads
posframe-autoloads hydra-autoloads pfuture-autoloads
ace-window-autoloads avy-autoloads bui-autoloads transient-autoloads
ido-completing-read+-autoloads memoize-autoloads with-editor-autoloads
compat-autoloads epl-autoloads lsp-docker-autoloads yaml-autoloads
highlight-symbol-autoloads expand-region-autoloads lsp-mode-autoloads
lv-autoloads markdown-mode-autoloads spinner-autoloads ht-autoloads
shrink-path-autoloads f-autoloads dash-autoloads s-autoloads info
editorconfig-autoloads helm-core-autoloads async-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 903185 104000)
  (symbols 48 46499 0)
  (strings 32 231885 9884)
  (string-bytes 1 6411082)
  (vectors 16 124883)
  (vector-slots 8 2338056 282804)
  (floats 8 984 1046)
  (intervals 56 17029 3032)
  (buffers 984 62))

-- 
Vennlig hilsen
*Jostein Kjønigsen*

jostein@kjonigsen.net 🍵 jostein@gmail.com
https://jostein.kjønigsen.no <https://jostein.kjønigsen.no>

[-- Attachment #2: Type: text/html, Size: 13086 bytes --]

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Eli Zaretskii]

> Cc: Yuan Fu <casouri@gmail.com>, Theodor Thornhill <theo@thornhill.no>
> Date: Fri, 27 Jan 2023 21:14:30 +0100
> From: Jostein Kjønigsen <jostein@secure.kjonigsen.net>
> 
> When compiling a TypeScript project using tsc (or other tooling) from Emacs,
> compilation errors and warnings are not highlighted by compilation-mode.
> 
> Consider the following output:
> 
> src/resources/document.ts:140:22 - error TS2362: The left-hand side of an arithmetic operation must be of
> type 'any', 'number', 'bigint' or an enum type.
> 
> 140       return `File-${new Date() * 1}${ext}`;
>                          ~~~~~~~~~~
> 
> This output should cause compilation-mode to highlight the error and provide code-navigation.
> 
> I know we explicitly added support for this to typescript.el back in the days, but I'm not
> sure what the "right" thing to do is now that typescrip-ts-mode is a first class Emacs citizen.
> 
> Should we add compilation-mode patterns directly to the major-mode, or should we provide
> patches to compile.el instead?

Why isn't this part of compilation-error-regexp-alist-alist?

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

On 1/27/23 21:30, Eli Zaretskii wrote:
>
> Why isn't this part of compilation-error-regexp-alist-alist?

That is obviously what I think we should do.
We already have good and tested matcher expressions for this.

I was just uncertain about what the conventional way of adding those 
expressions to Emacs:

- adding it directly in the relevant major-mode's source-file, to 
improve code-locality?
- adding it in compile.el, to improve ability to oversee all 
expressions, and be able to optimize those?

I see csharp-mode.el has the expressions added directly there. Should I 
go about preparing patches doing
the same for typescript-ts-mode too?

--
Jostein

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Eli Zaretskii]

> Date: Fri, 27 Jan 2023 21:52:24 +0100
> Cc: 61104@debbugs.gnu.org, casouri@gmail.com, theo@thornhill.no
> From: Jostein Kjønigsen <jostein@secure.kjonigsen.net>
> 
> On 1/27/23 21:30, Eli Zaretskii wrote:
> >
> > Why isn't this part of compilation-error-regexp-alist-alist?
> 
> That is obviously what I think we should do.
> We already have good and tested matcher expressions for this.
> 
> I was just uncertain about what the conventional way of adding those 
> expressions to Emacs:
> 
> - adding it directly in the relevant major-mode's source-file, to 
> improve code-locality?

I don't think doing this in the major-mode file will improve locality,
because we have compilation-minor-mode, which should be able to do its
thing even if the relevant major mode is not yet loaded.

> - adding it in compile.el, to improve ability to oversee all 
> expressions, and be able to optimize those?
> 
> I see csharp-mode.el has the expressions added directly there. Should I 
> go about preparing patches doing
> the same for typescript-ts-mode too?

I don't see why not.

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]


On 1/28/23 08:23, Eli Zaretskii wrote:
>
> I don't think doing this in the major-mode file will improve locality,
> because we have compilation-minor-mode, which should be able to do its
> thing even if the relevant major mode is not yet loaded.
Fair enough!
>
>> - adding it in compile.el, to improve ability to oversee all
>> expressions, and be able to optimize those?
>>
>> I see csharp-mode.el has the expressions added directly there. Should I
>> go about preparing patches doing
>> the same for typescript-ts-mode too?
> I don't see why not.

Ok. Attached is a patch which adds Typescript tsc-support to compile.el. 
Is this OK to install in emacs-29?

I also see in retrospect that my comment about csharp-mode above may 
been somewhat ambiguous and easy to misunderstand (and seemingly you did).

To be clear: csharp-mode includes the compilation-mode regexps in the 
major-mode, not in compile.el. Is this also something we should aim to 
fix for emacs-29, or should we leave that for master?

--
Jostein

[-- Attachment #2: 0001-Add-support-for-Typescript-compilation-to-compilatio.patch --]
[-- Type: text/x-patch, Size: 1476 bytes --]

From b60c0686fc925290ff201ed79399e48ebc47d6d0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jostein=20Kj=C3=B8nigsen?= <jostein@kjonigsen.net>
Date: Sat, 28 Jan 2023 15:23:11 +0100
Subject: [PATCH] Add support for Typescript compilation to compilation-mode

---
 lisp/progmodes/compile.el | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 5758eadf996..1e57d0b7bb2 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -649,6 +649,24 @@ compilation-error-regexp-alist-alist
     ;; we do not know what lines will follow.
     (guile-file "^In \\(.+\\..+\\):\n" 1 nil nil 0)
     (guile-line "^ *\\([0-9]+\\): *\\([0-9]+\\)" nil 1 2)
+
+    ;; Typescript compilation prior to tsc version 2.7, "plain" format:
+    ;; greeter.ts(30,12): error TS2339: Property 'foo' does not exist.
+    (typescript-tsc-plain
+     ,(concat
+      "^[[:blank:]]*"
+      "\\([^(\r\n)]+\\)(\\([0-9]+\\),\\([0-9]+\\)):[[:blank:]]+"
+      "error [[:alnum:]]+: [^\r\n]+$")
+     1 2 3 2)
+
+    ;; Typescript compilation after tsc version 2.7, "pretty" format:
+    ;; src/resources/document.ts:140:22 - error TS2362: something.
+    (typescript-tsc-pretty
+     ,(concat
+       "^[[:blank:]]*"
+       "\\([^(\r\n)]+\\):\\([0-9]+\\):\\([0-9]+\\) - [[:blank:]]*"
+       "error [[:alnum:]]+: [^\r\n]+$")
+     1 2 3 2)
     ))
   "Alist of values for `compilation-error-regexp-alist'.")
 
-- 
2.39.1

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

Any news on this one? Will this be merged? :)

Vennlig hilsen
*Jostein Kjønigsen*

jostein@kjonigsen.net 🍵 jostein@gmail.com
https://jostein.kjønigsen.no <https://jostein.kjønigsen.no>
On 1/28/23 15:28, Jostein Kjønigsen wrote:
>
> On 1/28/23 08:23, Eli Zaretskii wrote:
>>
>> I don't think doing this in the major-mode file will improve locality,
>> because we have compilation-minor-mode, which should be able to do its
>> thing even if the relevant major mode is not yet loaded.
> Fair enough!
>>
>>> - adding it in compile.el, to improve ability to oversee all
>>> expressions, and be able to optimize those?
>>>
>>> I see csharp-mode.el has the expressions added directly there. Should I
>>> go about preparing patches doing
>>> the same for typescript-ts-mode too?
>> I don't see why not.
>
> Ok. Attached is a patch which adds Typescript tsc-support to 
> compile.el. Is this OK to install in emacs-29?
>
> I also see in retrospect that my comment about csharp-mode above may 
> been somewhat ambiguous and easy to misunderstand (and seemingly you 
> did).
>
> To be clear: csharp-mode includes the compilation-mode regexps in the 
> major-mode, not in compile.el. Is this also something we should aim to 
> fix for emacs-29, or should we leave that for master?
>
> -- 
> Jostein

[-- Attachment #2: Type: text/html, Size: 2449 bytes --]

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2 February 2023 22:01:11 CET, "Jostein Kjønigsen" <jostein@secure.kjonigsen.net> wrote:
>Any news on this one? Will this be merged? :)
>

My guess is that it should go on master. I'll look at it today - sorry it took some time :)

Theo


>Vennlig hilsen
>*Jostein Kjønigsen*
>
>jostein@kjonigsen.net 🍵 jostein@gmail.com
>https://jostein.kjønigsen.no <https://jostein.kjønigsen.no>
>On 1/28/23 15:28, Jostein Kjønigsen wrote:
>> 
>> On 1/28/23 08:23, Eli Zaretskii wrote:
>>> 
>>> I don't think doing this in the major-mode file will improve locality,
>>> because we have compilation-minor-mode, which should be able to do its
>>> thing even if the relevant major mode is not yet loaded.
>> Fair enough!
>>> 
>>>> - adding it in compile.el, to improve ability to oversee all
>>>> expressions, and be able to optimize those?
>>>> 
>>>> I see csharp-mode.el has the expressions added directly there. Should I
>>>> go about preparing patches doing
>>>> the same for typescript-ts-mode too?
>>> I don't see why not.
>> 
>> Ok. Attached is a patch which adds Typescript tsc-support to compile.el. Is this OK to install in emacs-29?
>> 
>> I also see in retrospect that my comment about csharp-mode above may been somewhat ambiguous and easy to misunderstand (and seemingly you did).
>> 
>> To be clear: csharp-mode includes the compilation-mode regexps in the major-mode, not in compile.el. Is this also something we should aim to fix for emacs-29, or should we leave that for master?
>> 
>> -- 
>> Jostein

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Eli Zaretskii]

> Date: Fri, 03 Feb 2023 06:30:29 +0100
> From: Theodor Thornhill <theo@thornhill.no>
> CC: 61104@debbugs.gnu.org, casouri@gmail.com
> 
> On 2 February 2023 22:01:11 CET, "Jostein Kjønigsen" <jostein@secure.kjonigsen.net> wrote:
> >Any news on this one? Will this be merged? :)
> >
> 
> My guess is that it should go on master. I'll look at it today - sorry it took some time :)

I'm okay with installing this on emacs-29 if the patch looks good.
Typescript mode is new in Emacs 29, so it's okay to make this change
now.  But please be sure that the relevant tests still pass, i.e. that
this change doesn't break something else in compilation-mode.

Thanks.

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 03 Feb 2023 06:30:29 +0100
>> From: Theodor Thornhill <theo@thornhill.no>
>> CC: 61104@debbugs.gnu.org, casouri@gmail.com
>> 
>> On 2 February 2023 22:01:11 CET, "Jostein Kjønigsen" <jostein@secure.kjonigsen.net> wrote:
>> >Any news on this one? Will this be merged? :)
>> >
>> 
>> My guess is that it should go on master. I'll look at it today - sorry it took some time :)
>
> I'm okay with installing this on emacs-29 if the patch looks good.
> Typescript mode is new in Emacs 29, so it's okay to make this change
> now.  But please be sure that the relevant tests still pass, i.e. that
> this change doesn't break something else in compilation-mode.
>
> Thanks.

Ok, I'll do that then.  I'll add this in addition in the next commit, if
that's ok.

Thanks, Jostein and Eli :)

Theo

diff --git a/test/lisp/progmodes/compile-tests.el b/test/lisp/progmodes/compile-tests.el
index 53dc7f2a13..22721563df 100644
--- a/test/lisp/progmodes/compile-tests.el
+++ b/test/lisp/progmodes/compile-tests.el
@@ -382,9 +382,13 @@ compile-tests--test-regexps-data
     ;; sun-ada
     (sun-ada "/home3/xdhar/rcds_rc/main.a, line 361, char 6:syntax error: \",\" inserted"
      1 6 361 "/home3/xdhar/rcds_rc/main.a")
+    (typescript-tsc-plain "/home/foo/greeter.ts(30,12): error TS2339: Property 'foo' does not exist."
+     1 12 30 "/home/foo/greeter.ts")
+    (typescript-tsc-pretty "src/resources/document.ts:140:22 - error TS2362: something."
+     1 22 140 "src/resources/document.ts")
     ;; 4bsd
     (edg-1 "/usr/src/foo/foo.c(8): warning: w may be used before set"
-     1 nil 8 "/usr/src/foo/foo.c")
+           1 nil 8 "/usr/src/foo/foo.c")
     (edg-1 "/usr/src/foo/foo.c(9): error: w is used before set"
      1 nil 9 "/usr/src/foo/foo.c")
     (4bsd "strcmp: variable # of args. llib-lc(359)  ::  /usr/src/foo/foo.c(8)"
@@ -495,7 +499,7 @@ compile-test-error-regexps
           (compilation-num-warnings-found 0)
           (compilation-num-infos-found 0))
       (mapc #'compile--test-error-line compile-tests--test-regexps-data)
-      (should (eq compilation-num-errors-found 98))
+      (should (eq compilation-num-errors-found 100))
       (should (eq compilation-num-warnings-found 35))
       (should (eq compilation-num-infos-found 28)))))
 

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Theodor Thornhill <theo@thornhill.no> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Date: Fri, 03 Feb 2023 06:30:29 +0100
>>> From: Theodor Thornhill <theo@thornhill.no>
>>> CC: 61104@debbugs.gnu.org, casouri@gmail.com
>>> 
>>> On 2 February 2023 22:01:11 CET, "Jostein Kjønigsen" <jostein@secure.kjonigsen.net> wrote:
>>> >Any news on this one? Will this be merged? :)
>>> >
>>> 
>>> My guess is that it should go on master. I'll look at it today - sorry it took some time :)
>>
>> I'm okay with installing this on emacs-29 if the patch looks good.
>> Typescript mode is new in Emacs 29, so it's okay to make this change
>> now.  But please be sure that the relevant tests still pass, i.e. that
>> this change doesn't break something else in compilation-mode.
>>
>> Thanks.
>
> Ok, I'll do that then.  I'll add this in addition in the next commit, if
> that's ok.
>
> Thanks, Jostein and Eli :)

The changes are now installed on the emacs-29 branch, thanks!

Closing this :)

Theo

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Mattias Engdegård]

Jostein, thank you for contributing a new compilation-mode pattern!

We generally want these regexps to be as tight as possible while still doing their job. This is partly because the large and ever-growing number of rules tend to interfere with one another, and over-broad patterns have shown to be a source of performance problems in the past.

I hope you don't mind helping us finishing the job:

First of all, both regexps match arbitrary amounts of horizontal whitespace at the beginning of a line, but neither message example you supplied contains any such leading whitespace. This means that either the set of test cases needs to be extended, or we could safely remove this leading whitespace matcher.

If leading whitespace indeed can occur, then when and how, exactly? Spaces or tabs, and how many? Please give us examples from actual compiler output.

Similarly the patterns match arbitrary whitespace before the word "error". This seems equally questionable -- would a single space do? If not, please provide actual output demonstrating it that could be added to the test suite, and tell us how it varies (tabs vs spaces, amount of whitespace, etc).

The following is a minor point that we'll fix but I thought you may want to know:

The use of [[:blank:]] and [[:alnum:]] is very likely more expensive than required since they accept Unicode whitespace and letters which obviously never will occur where matched so if it's all the same to you we'll reduce them to ASCII patterns.

Similarly, the inclusion of \r in patterns seems to be a misunderstanding: the tail part, "[^\r\n]+$", does not make sense -- normally, carriage returns aren't seen in buffers because line terminator translation convert everything to a single \n, and if a stray CR did occur then that pattern would never match anyway (why?).

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

Hey there and thanks for the valuable feedback!

I'll try to do my best to provide the info I can, so that we can create 
the tightest regexps for this, which still are functional on the level 
users would expect.

On 2/4/23 12:59, Mattias Engdegård wrote:
> First of all, both regexps match arbitrary amounts of horizontal whitespace at the beginning of a line, but neither message example you supplied contains any such leading whitespace. This means that either the set of test cases needs to be extended, or we could safely remove this leading whitespace matcher.

I've gone looking, but I really can't find confirmation that this 
whitespace is required, at least not when building directly through the 
tsc TypeScript compiler.
I can see in the old test-suite for the MELPA package these two variants 
were the only test-cases present as well.

As such I think it's defiintely safe to remove this leading whitespace.

> Similarly the patterns match arbitrary whitespace before the word "error". This seems equally questionable -- would a single space do? If not, please provide actual output demonstrating it that could be added to the test suite, and tell us how it varies (tabs vs spaces, amount of whitespace, etc).
I can't see any real use-case for this either. Let's snip it.
> The following is a minor point that we'll fix but I thought you may want to know:
>
> The use of [[:blank:]] and [[:alnum:]] is very likely more expensive than required since they accept Unicode whitespace and letters which obviously never will occur where matched so if it's all the same to you we'll reduce them to ASCII patterns.
I've given this a try, and it seems to work fine. I'm OK with such a change.
> Similarly, the inclusion of \r in patterns seems to be a misunderstanding: the tail part, "[^\r\n]+$", does not make sense -- normally, carriage returns aren't seen in buffers because line terminator translation convert everything to a single \n, and if a stray CR did occur then that pattern would never match anyway (why?).

Fair enough. I've changed the code to only looks for \n instead.

Attached is a patch which codifies all these changes, and from what I 
can tell, still does the job. You make take it as is, or you may further 
work on it, if you think that is still needed.

--
Jostein

[-- Attachment #2: 0001-Optimize-compilation-mode-expressions-for-TypeScript.patch --]
[-- Type: text/x-patch, Size: 1488 bytes --]

From 1c6a71cd1db5b589ff9fc5f4fe76e9357b7bedbd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jostein=20Kj=C3=B8nigsen?= <jostein@kjonigsen.net>
Date: Sun, 5 Feb 2023 21:34:08 +0100
Subject: [PATCH] Optimize compilation-mode expressions for TypeScript

- lisp/progmodes/compile.el: remove unneeded and expensive checks.
---
 lisp/progmodes/compile.el | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 1e57d0b7bb..7700e5f7b1 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -654,18 +654,16 @@ compilation-error-regexp-alist-alist
     ;; greeter.ts(30,12): error TS2339: Property 'foo' does not exist.
     (typescript-tsc-plain
      ,(concat
-      "^[[:blank:]]*"
-      "\\([^(\r\n)]+\\)(\\([0-9]+\\),\\([0-9]+\\)):[[:blank:]]+"
-      "error [[:alnum:]]+: [^\r\n]+$")
+      "\\([^(\r\n)]+\\)(\\([0-9]+\\),\\([0-9]+\\)): "
+      "error [A-Z0-9]+: [^\n]+$")
      1 2 3 2)
 
     ;; Typescript compilation after tsc version 2.7, "pretty" format:
     ;; src/resources/document.ts:140:22 - error TS2362: something.
     (typescript-tsc-pretty
      ,(concat
-       "^[[:blank:]]*"
-       "\\([^(\r\n)]+\\):\\([0-9]+\\):\\([0-9]+\\) - [[:blank:]]*"
-       "error [[:alnum:]]+: [^\r\n]+$")
+       "\\([^(\r\n)]+\\):\\([0-9]+\\):\\([0-9]+\\) - "
+       "error [A-Z0-9]+: [^\n]+$")
      1 2 3 2)
     ))
   "Alist of values for `compilation-error-regexp-alist'.")
-- 
2.39.1

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

5 feb. 2023 kl. 21.36 skrev Jostein Kjønigsen <jostein@secure.kjonigsen.net>:

> Attached is a patch which codifies all these changes, and from what I can tell, still does the job. You make take it as is, or you may further work on it, if you think that is still needed.

Thank you! I translated the regexps to rx (which I personally find more maintainable and has plenty of precedence in this context) and tightened them up further. They are now anchored at beginning-of-line again, and file names cannot start with whitespace (for disambiguation and speed).

I also removed the part that matches the actual message text since it isn't needed, and it would highlight (with an underline in the standard theme) the whole text which is a bit ungainly to read. If the message is multi-line, which earlier examples in this discussions indicated might be the case, then only the first would be highlighted this way which wasn't ideal either.

Patch attached, please tell me what you think.

Does tsc distinguish between warnings, errors, and 'informational' messages (such as locations of interest that are not errors in their own right)? The examples you supplied all had the word "error" in a prominent place.

It would be possible to join the two tsc rules into a single one which would be slightly faster, but having them separate could also be an advantage since it would allow for them to be disabled individually in case of clashes.


[-- Attachment #2: 0001-Simplify-typescript-compilation-mode-patterns-bug-61.patch --]
[-- Type: application/octet-stream, Size: 3086 bytes --]

From d6d336cfe588dd5d8a5bf647c5beb9e45b47ef3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 6 Feb 2023 11:45:33 +0100
Subject: [PATCH] Simplify typescript compilation-mode patterns (bug#61104)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/progmodes/compile.el (compilation-error-regexp-alist-alist):
Tighten regexps and simplify.  Translate to rx.
* etc/compilation.txt: Add examples.

In collaboration with Jostein Kjønigsen.
---
 etc/compilation.txt       | 14 ++++++++++++++
 lisp/progmodes/compile.el | 28 ++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/etc/compilation.txt b/etc/compilation.txt
index 672cbebafff..5f6ecb09cc2 100644
--- a/etc/compilation.txt
+++ b/etc/compilation.txt
@@ -639,6 +639,20 @@ symbol: weblint
 index.html (13:1) Unknown element <fdjsk>
 
 
+* Typescript prior to tsc version 2.7, "plain" format
+
+symbol: typescript-tsc-plain
+
+greeter.ts(30,12): error TS2339: Property 'foo' does not exist.
+
+
+* Typescript after tsc version 2.7, "pretty" format
+
+symbol: typescript-tsc-pretty
+
+src/resources/document.ts:140:22 - error TS2362: something.
+
+
 * Directory tracking
 
 Directories are matched via 'compilation-directory-matcher'.  Files which are
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 1e57d0b7bb2..ccf64fb670b 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -653,19 +653,31 @@ compilation-error-regexp-alist-alist
     ;; Typescript compilation prior to tsc version 2.7, "plain" format:
     ;; greeter.ts(30,12): error TS2339: Property 'foo' does not exist.
     (typescript-tsc-plain
-     ,(concat
-      "^[[:blank:]]*"
-      "\\([^(\r\n)]+\\)(\\([0-9]+\\),\\([0-9]+\\)):[[:blank:]]+"
-      "error [[:alnum:]]+: [^\r\n]+$")
+     ,(rx bol
+          (group (not (in " \t\n()"))   ; 1: file
+                 (* (not (in "\n()"))))
+          "("
+          (group (+ (in "0-9")))        ; 2: line
+          ","
+          (group (+ (in "0-9")))        ; 3: column
+          "): error "
+          (+ (in "0-9A-Z"))             ; error code
+          ": ")
      1 2 3 2)
 
     ;; Typescript compilation after tsc version 2.7, "pretty" format:
     ;; src/resources/document.ts:140:22 - error TS2362: something.
     (typescript-tsc-pretty
-     ,(concat
-       "^[[:blank:]]*"
-       "\\([^(\r\n)]+\\):\\([0-9]+\\):\\([0-9]+\\) - [[:blank:]]*"
-       "error [[:alnum:]]+: [^\r\n]+$")
+     ,(rx bol
+          (group (not (in " \t\n()"))   ; 1: file
+                 (* (not (in "\n()"))))
+          ":"
+          (group (+ (in "0-9")))        ; 2: line
+          ":"
+          (group (+ (in "0-9")))        ; 3: column
+          " - error "
+          (+ (in "0-9A-Z"))             ; error code
+          ": ")
      1 2 3 2)
     ))
   "Alist of values for `compilation-error-regexp-alist'.")
-- 
2.32.0 (Apple Git-132)

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Jostein Kjønigsen]

On 2/6/23 12:19, Mattias Engdegård wrote:
> Thank you! I translated the regexps to rx (which I personally find 
> more maintainable and has plenty of precedence in this context) and 
> tightened them up further. They are now anchored at beginning-of-line 
> again, and file names cannot start with whitespace (for disambiguation 
> and speed). 
Thanks. These are all good changes.
> I also removed the part that matches the actual message text since it isn't needed, and it would highlight (with an underline in the standard theme) the whole text which is a bit ungainly to read. If the message is multi-line, which earlier examples in this discussions indicated might be the case, then only the first would be highlighted this way which wasn't ideal either.
Seconded. No issues with that either.
> Does tsc distinguish between warnings, errors, and 'informational' messages (such as locations of interest that are not errors in their own right)? The examples you supplied all had the word "error" in a prominent place.

I don't think so. There are code-analysis warnings which seems to be 
provided to your editor of choice through LSP. These can be made into a 
compilation-error with the appropriate config-flag, but I can't out of 
the blue find any "middle ground" where they are emitted compile-time as 
warnings.

Someone please correct me if I'm wrong.

> It would be possible to join the two tsc rules into a single one which would be slightly faster, but having them separate could also be an advantage since it would allow for them to be disabled individually in case of clashes.

To me that sounds like an optimization going one step too far. It will 
result in a more complex expression, which is harder to work 
with/maintain, and might also be more computationally expensive due to 
back-tracking complexity.

I personally think that having 2 self-documented expressions side by 
side works better, but I'm no authority on compilation-mode and 
performance :)

> Patch attached, please tell me what you think.
I've tested that patch myself, and from what I can tell, it still works 
just fine :)

--
Jostein

bug#61104: 29.0.60; typescript-ts-mode does not provide compilation-mode support

[Mattias Engdegård]

6 feb. 2023 kl. 18.05 skrev Jostein Kjønigsen <jostein@secure.kjonigsen.net>:

> I've tested that patch myself, and from what I can tell, it still works just fine

Excellent, now pushed to emacs-29. We're done!