bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

This is coming from:

   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34418
   https://github.com/joaotavora/eglot/pull/640

In the first, Philip wonders why the BUFFER argument to
flymake-make-diagnostic is mostly ignored.

In the second, Theodor requests that the Eglot LSP client manage and
display the diagnostics reported by the LSP server not only for the
current buffer being edited, but for other buffers or files.

The connection between the two will hopefully become apparent soon.

Normally, diagnostics reports are emitted by diagnostic backend for a
single file, usually as a result of a change performed shortly before in
the buffer visiting that file.  This is true for most LSP servers when
acting as Flymake backends.  But some LSP servers seem to take this
opportunity to also "piggy-back" diagnostics for other files, in a kind
of "side-band signal".  Other servers yet will sometimes report
diagnostics upfront for all files in a given project, whether visited by
a buffer or not.

Theodor's pull request handles this side band signal for LSP specifically
in the Eglot client, but it contains some twists and turns that should
not be taken in Eglot.  This functionality should be supported in
Flymake.

* What kind of new API support for Flymake backends is needed for this?
  This is the good news: I think not much.

  The helper function flymake-make-diagnostic, used by Flymake backends
  such as Eglot's, already supports a BUFFER first argument, but, as the
  manual states.
   
     Currently, it is unspecified behavior to make diagnostics for
     buffers other than the buffer that the Flymake backend is
     responsible for.
   
  My idea is to make it specified.  As floated in bug#34418 I think the
  idea of generalizing the argument to mean BUFFER-OR-FILE is decent and
  sufficient.

* What kind of infrastructure changes in Flymake are needed for this?

  That's more complicated: substantial, but not insane.  Here's a
  top-level plan: the current buffer-local variable
  flymake--backend-state should probably first become a global hash
  table indexed by buffer, which should be mostly indistinguishable from
  a buffer-local var.

  If that works, then it should be possible to store the piggy-backed
  diagnostics in (1) file-indexed entries to that global hash table, for
  files which are not being visited by buffers.  It should also be
  possible to add (2) buffer-indexed entries to it for buffers which,
  though live, don't have Flymake properly activated or don't have that
  specific piggy-backing backend in their running backend list.

  These new types of entries will likely not contain the full-blown
  flymake--backend-state structs, i.e. they will be missing the
  'running', 'reported-p' and 'disabled' slots.  All they will have is a
  non-nil list 'diags'.

  If, for situations (1) or (2), we discover that a "proper"
  buffer-indexed entry already exists in the global hash table, we must
  decide if the information in them should take priority.  I'm not sure
  of the answer to this yet.

  Anyway, in flymake--handle-report is where the recording of
  information should happen.  We can have a look at the current
  aforementioned "unspecified" behavior:

         (setq new-diags
              (cl-remove-if-not
               (lambda (diag) (eq (flymake--diag-buffer diag) (current-buffer)))
               report-action))

  (As we can see, the current "unspecification" is just ignoring reports
  for BUFFERs other than the current.)

* In what new UI parts is the augmented information to be useful?

  Currently, I see only one place, the diagnostic listing buffer
  obtained by M-x flymake-show-diagnostics-buffer.  That buffer is
  usually associated with only one source buffer
  (flymake--diagnostics-buffer-source).  Now it should start listing all
  the diagnostics for buffers or files known to belong to the same
  project, using 'project.el' functionality for that.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 29.08.2021 03:53, João Távora wrote:
> * In what new UI parts is the augmented information to be useful?
> 
>    Currently, I see only one place, the diagnostic listing buffer
>    obtained by M-x flymake-show-diagnostics-buffer.  That buffer is
>    usually associated with only one source buffer
>    (flymake--diagnostics-buffer-source).  Now it should start listing all
>    the diagnostics for buffers or files known to belong to the same
>    project, using 'project.el' functionality for that.

 From what I know of this feature is usually organized, we have two 
possible kinds of sources of errors:

1. Per buffer, which is currently being edited.

2. For the whole project, where the notion of project is defined by the 
tool which produces the said list of errors. With that approach, when 
you fix an error which triggered some cascading errors in other files, 
you can see those other errors fixed too.

Is the idea to build a list of "project errors" using sources of type 1? 
That would seem inefficient, though I suppose some people might want 
this too.

But I thought the original discussion was about errors from sources of 
type 2? Then the whole list provided by the source already belongs to 
the same project (how the LSP server understands it, if we use the LSP 
example).

Is there a need for further filtering?

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

On 30 August 2021 01:27:52 CEST, Dmitry Gutov <dgutov@yandex.ru> wrote:
>On 29.08.2021 03:53, João Távora wrote:
>> * In what new UI parts is the augmented information to be useful?
>> 
>>    Currently, I see only one place, the diagnostic listing buffer
>>    obtained by M-x flymake-show-diagnostics-buffer.  That buffer is
>>    usually associated with only one source buffer
>>    (flymake--diagnostics-buffer-source).  Now it should start listing all
>>    the diagnostics for buffers or files known to belong to the same
>>    project, using 'project.el' functionality for that.
>
> From what I know of this feature is usually organized, we have two 
>possible kinds of sources of errors:
>
>1. Per buffer, which is currently being edited.
>

Yes, and some LSP servers may only deliver this. Typically, though, servers send to gravitate towards 2 these days


>2. For the whole project, where the notion of project is defined by the 
>tool which produces the said list of errors. With that approach, when 
>you fix an error which triggered some cascading errors in other files, 
>you can see those other errors fixed too.
>
>Is the idea to build a list of "project errors" using sources of type 1? 
>That would seem inefficient, though I suppose some people might want 
>this too.
>

In the case of elm-language-server, with which I use the most along OmniSharp and F#, all of them report all diags in all files known to the project (as defined by LSP) on the first load, then all potential diags across all files per edit afterwards. This case get super chatty very fast, but since it always flushes everything one can at least rely on what you get. The pr I made works pretty well performance wise in a medium large repo (~70k loc elm), at least considering it isn't very polished, awaiting this discussion.


>But I thought the original discussion was about errors from sources of 
>type 2? Then the whole list provided by the source already belongs to 
>the same project (how the LSP server understands it, if we use the LSP 
>example).
>
>Is there a need for further filtering?

Not in the case of LSP, no. 

In addition, the structure the error buffer uses would be nice to expose so that one can build utility functions like the ones in the pr.

Theo

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 29.08.2021 03:53, João Távora wrote:

> Is the idea to build a list of "project errors" using sources of type
> 1?

No.  The current idea is, in short, to not have Flymake discard
information of what you call "type 2" (and which I described in my
email).  As I wrote there, examples of discarded information can be
project-wide diagnostics sent by LSP servers on server startup, among
others.  For a non-LSP example, consider references to errors .c in
files when editing .h files.

In other words, this is a "best effort service" by flymake.el: the topic
of this bug#50244 is _not_ to augment flymake.el to somehow change the
way that diagnostics are requested so that somehow more of what you call
"type 2" can be gathered.  To be clear, after this feature is done,
diagnosics are _still_ requested per-buffer. Eminently, on buffer
changes and less frequently on occasions like visiting a file.

The idea is simply to not discard what I called "piggy-backed" or
"sideband" information about diagnostics in other places, which will
frequently originate from the aforementioned per-buffer requests.

> But I thought the original discussion was about errors from sources of
> type 2? Then the whole list provided by the source already belongs to
> the same project (how the LSP server understands it, if we use the LSP
> example). Is there a need for further filtering?

Yes, and even in the .c/.h non-LSP example that is true.  But depending
on what data-structures are employed (I'm still thinking about them),
utilizing project.el's basic features for composing M-x
flymake-diagnostics-buffer _may_ come in handy.  This is pretty
secondary at this point, in my view at least.  I first want to have the
information recorded and correctly updated in Elisp memory.  Then I will
think about how to present it to the user.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

> The idea is simply to not discard what I called "piggy-backed" or
> "sideband" information about diagnostics in other places, which will
> frequently originate from the aforementioned per-buffer requests.

Hi, I've been working on this quite intensively and am finally
approaching sometehing stable.  You may check my ongoing work in the
branch scratch/bug-50244.

As expected, I abandoned/reformulated much of my original design ideas
many times as I faced new edge cases.  It was quite challenging to
support the main new command, M-x flymake-show-project-diagnostics, when
e.g. .c files referencing problems in .h files are killed and vice-versa
in various orders.

If you're interested in understanding how it works, the main entry point
are the additions to the existing documentation, the manual that lives
in flymake.texi and the docstrings.

As is explained in the manual, there are two types of Flymake
diagnostics: (1) the usual diagnostics for the current buffer, and (2)
so-called non-domestic diagnostics, which might be "foreign" diagnostics
or "list-only" diagnostics.  The difference is explained in the new
manual section "Foreign and list-only diagnostics".  How does this
matter in practice?

- For the examples of .c/.h files (and the flymake-cc backend, which I
  also updated), "foreign diagnostics" are used.

- For LSP, likely "list-only diagnostics" should be used.  So basically
  Eglot will soon learn to tell Flymake about the "list-only
  diagnostics" that it receives from some LSP servers.  Once that is
  done, M-x flymake-show-project-diagnostics should ideally do most
  things that Theodor wants.

Even though the protocol is already designed, flymake.el is still
missing the implementation of "list-only diagnostics".  It should, in
principle, be much simpler than the "foreign diagnostics", which as I
hinted above are much harder to keep up to date.

I hope you can have a look,
Thank you,
João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

Hi!

On 11.09.2021 04:08, João Távora wrote:
> - For LSP, likely "list-only diagnostics" should be used.  So basically
>    Eglot will soon learn to tell Flymake about the "list-only
>    diagnostics" that it receives from some LSP servers.  Once that is
>    done, M-x flymake-show-project-diagnostics should ideally do most
>    things that Theodor wants.

Does this account for the case of multiple projects?

Like, the user might switch between two windows which show buffers from 
different projects (both, say, are backed by LSP), and they would 
probably want to see the "global" error list (displayed in a third 
window) change right away.

Or maybe you will have unique "show diagnostics" buffers for every 
project, to be invoked manually?

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

On Mon, Sep 13, 2021 at 1:08 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> Or maybe you will have unique "show diagnostics" buffers for every
> project, to be invoked manually?

This.  But it doesn't seem impossible to make a global diagnostics
buffer for every project one has open.


João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> On Mon, Sep 13, 2021 at 1:08 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
>> Or maybe you will have unique "show diagnostics" buffers for every
>> project, to be invoked manually?
>
> This.  But it doesn't seem impossible to make a global diagnostics
> buffer for every project one has open.

The main changes to flymake.el and its documentation are now ready to
push.  There are some bugs regarding sorting in the diagnostics listing,
and the maybe order and length of columns needs rearranging, but these
can be sorted out later.

The only outstanding issue preventing me from landing this in main is
that I need to bump project.el's version so that the new
`project-buffers` API generic function becomes officially available to
the new bumped flymake.el version.  Dmitry is it OK for me to do so?

Here's the trivial patch to project.el.  I'm bumping the minor version
becasue a new backward-compatible feature was added.  I can bump
whatever you prefer if you think it's more in-line with the versioning
scheme you normally use.

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ba95ed094e..a6e231b9d6 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1,7 +1,7 @@
 ;;; project.el --- Operations on the current project  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 2015-2021 Free Software Foundation, Inc.
-;; Version: 0.6.1
+;; Version: 0.7.1
 ;; Package-Requires: ((emacs "26.1") (xref "1.0.2"))
 
 ;; This is a GNU ELPA :core package.  Avoid using functionality that

Another important aspect is that I haven't had a change to test this
with Eglot, which was one of the main motivators behind this change.
The reason is that I don't have easy access to a server which reports
diagnostics project wide (I thought clangd did, but I was mistaken).  So
the only client of the new functionality is the flymake-cc non-LSP
backend, for now.

Theodor, now would be a good time for you to step in with changes to
Eglot that use the new `flymake-list-only-diagnostics` experimental API
in flymake.el.  Likely, some adjustments will have to be made to both
packages.

Thanks,
João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

João Távora <joaotavora@gmail.com> writes:


>
> The main changes to flymake.el and its documentation are now ready to
> push.  There are some bugs regarding sorting in the diagnostics listing,
> and the maybe order and length of columns needs rearranging, but these
> can be sorted out later.
>

Very nice, great job!  I've been looking through the code the last
couple of days.  I think it makes sense to treat the list-only diags the
way you do here, in that we only need to be able to jump to it, in
addition to know that the error is there.

One thing I saw while reading and that might not be needed anymore is
contained in the diff at the bottom.  There might be reasons for this,
but I _think_ this should be covered in the &aux variable?


>
> Another important aspect is that I haven't had a change to test this
> with Eglot, which was one of the main motivators behind this change.
> The reason is that I don't have easy access to a server which reports
> diagnostics project wide (I thought clangd did, but I was mistaken).  So
> the only client of the new functionality is the flymake-cc non-LSP
> backend, for now.
>
> Theodor, now would be a good time for you to step in with changes to
> Eglot that use the new `flymake-list-only-diagnostics` experimental API
> in flymake.el.  Likely, some adjustments will have to be made to both
> packages.
>

I will absolutely jump at this as quickly as possible.  I'm not exactly
sure how fast I'll be, since I'm on parental leave right now.  How would
you want to receive the changes to eglot?  As part of this bug, or as a
new issue over at GH? I'm fine with either. Considering both flymake and
eglot may be affected, maybe this bug is a good place?

Again, thanks - now I can soon stop maintaining a trash fork of eglot to
get this functionality :)

All the best, and I'll get back to you soon!

Theo


diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 83c0b4dd99..543cfe64f4 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -815,8 +815,6 @@ calling convention described in
 to handle a report even if TOKEN was not expected.  REGION is
 a (BEG . END) pair of buffer positions indicating that this
 report applies to that region."
-  (unless state
-    (error "Can't find state for %s in `flymake--state'" backend))
   (cond
    ((null state)
     (flymake-error

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Theodor Thornhill <theo@thornhill.no> writes:

> I will absolutely jump at this as quickly as possible.  I'm not exactly
> sure how fast I'll be, since I'm on parental leave right now.

Congrats And enjoy it (I enjoyed mine very much)! > 

> How would you want to receive the changes to eglot?  As part of this
> bug, or as a new issue over at GH? I'm fine with either. Considering
> both flymake and eglot may be affected, maybe this bug is a good
> place?

Yes, it is (but the PR also works).  I hope to transfer eglot.el to
Emacs core soon anyway.  

Earlier you said project-wide diagnostics is become a trend with LSP
servers.  Besides your elm-language-server where you most need this,
what other servers are doing this?  Maybe I can install one of those for
a language I know and start testing this.

> diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
> index 83c0b4dd99..543cfe64f4 100644
> --- a/lisp/progmodes/flymake.el
> +++ b/lisp/progmodes/flymake.el
> @@ -815,8 +815,6 @@ calling convention described in
>  to handle a report even if TOKEN was not expected.  REGION is
>  a (BEG . END) pair of buffer positions indicating that this
>  report applies to that region."
> -  (unless state
> -    (error "Can't find state for %s in `flymake--state'" backend))
>    (cond
>     ((null state)
>      (flymake-error

Nice catch.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 13.09.2021 21:03, João Távora wrote:
> The only outstanding issue preventing me from landing this in main is
> that I need to bump project.el's version so that the new
> `project-buffers` API generic function becomes officially available to
> the new bumped flymake.el version.  Dmitry is it OK for me to do so?

Yes, please go ahead. All the latest additions seem stable.

Like mentioned previously, I think using project.el for this is probably 
not ideal (rather, the diagnostic source can decide which diagnostics 
belong to the current default-directory), but I don't feel up to having 
an argument about that.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

João Távora <joaotavora@gmail.com> writes:

> Theodor Thornhill <theo@thornhill.no> writes:
>
>> I will absolutely jump at this as quickly as possible.  I'm not exactly
>> sure how fast I'll be, since I'm on parental leave right now.
>
> Congrats And enjoy it (I enjoyed mine very much)! > 

Thanks!  Second time now, only gets better :)

>
>> How would you want to receive the changes to eglot?  As part of this
>> bug, or as a new issue over at GH? I'm fine with either. Considering
>> both flymake and eglot may be affected, maybe this bug is a good
>> place?
>
> Yes, it is (but the PR also works).  I hope to transfer eglot.el to
> Emacs core soon anyway.  
>

Nice.  Consider the lingering PR closed.

> Earlier you said project-wide diagnostics is become a trend with LSP
> servers.  Besides your elm-language-server where you most need this,
> what other servers are doing this?  Maybe I can install one of those for
> a language I know and start testing this.
>

From the ones I've used at work, F# [0], C# [1], Rust [2] and Elm [3]
all support them.  I think the easiest to set up might be the rust one.
However, I can help out with settings if needed.  I've had to subclass
the eglot server to get reliable performance from some of them.


> Nice catch.

No problem!

By the way, I'm testing this version right now, and it seems both faster
and more accurate.  Not sure why or what exactly happens, but it works
flawlessly with eglot atm.  Though still without the project buffer.
Looking into that a little tonight (election in Norway today, so might
stay up a little anyway :P)

Theo

[0]: https://github.com/fsharp/FsAutoComplete
[1]: https://github.com/OmniSharp/omnisharp-roslyn
[2]: https://github.com/rust-analyzer/rust-analyzer
[3]: https://github.com/elm-tooling/elm-language-server

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 13.09.2021 09:48, João Távora wrote:
> On Mon, Sep 13, 2021 at 1:08 AM Dmitry Gutov<dgutov@yandex.ru>  wrote:
> 
>> Or maybe you will have unique "show diagnostics" buffers for every
>> project, to be invoked manually?
> This.  But it doesn't seem impossible to make a global diagnostics
> buffer for every project one has open.

Anyway, I asked about this because because the use of global variable 
(flymake-list-only-diagnostics) seems like it would make supporting 
multiple projects at the same time more difficult.

If global diagnostics are only reported to a particular buffer, perhaps 
we could do without the global variable by tying refreshes to a 
callback. That would require an extra hook, however -- like 
flymake-global-diagnostic-functions.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.09.2021 09:48, João Távora wrote:
>> On Mon, Sep 13, 2021 at 1:08 AM Dmitry Gutov<dgutov@yandex.ru>  wrote:
>> 
>>> Or maybe you will have unique "show diagnostics" buffers for every
>>> project, to be invoked manually?
>> This.  But it doesn't seem impossible to make a global diagnostics
>> buffer for every project one has open.
>
> Anyway, I asked about this because because the use of global variable
> (flymake-list-only-diagnostics) seems like it would make supporting
> multiple projects at the same time more difficult.

I can't guess what you are hinting at, sorry.  You can call M-x
flymake-show-project-diagnostics in two or more projects, of course, and
you'll get separate listings.  I don't know if that counts as
"supporting multiple projects at the same time".

> If global diagnostics are only reported to a particular buffer,
> perhaps we could do without the global variable by tying refreshes to
> a callback.

I don't fully understand what you're suggesting, because I don't
understand your working concepts. In the new manual section "Foreign and
list-only diagnostics", you can read that the new API allows a form of
reporting diagnostics for other buffers/files in a way that is tied to a
callback.  Maybe that that what you mean?

Anyway, for the specific of case of Eglot/LSP -- which reports
neighboring diagnostics _sporadically_ (i.e. not systematically) -- the
"list only" diagnostics seem more suitable.  For the purposes that I
envisioned (only listing, as the name implies) a global variable also
seems suitable.

Again, I don't understand your suggestion, but if you can propose it in
the form of code it would be much better, since there would be no
ambiguity.  A word of caution though: making these things work correctly
in tandem with domestic diagnostics, new file visits and buffer killings
was relatively hard.  I tried many different approaches and settled on
these two ("foreign" and "list-only").  Of course if you can clearly
describe a use case where they are unsuitable, a third style may be
invented.  But I would first exhaust the possibilities in these two.

In the meantime, please say if version-bumping project.el is OK.  That
is the only thing holding up the merge.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 13.09.2021 23:53, João Távora wrote:

>> Anyway, I asked about this because because the use of global variable
>> (flymake-list-only-diagnostics) seems like it would make supporting
>> multiple projects at the same time more difficult.
> 
> I can't guess what you are hinting at, sorry.  You can call M-x
> flymake-show-project-diagnostics in two or more projects, of course, and
> you'll get separate listings.  I don't know if that counts as
> "supporting multiple projects at the same time".

If a backend reports diagnostics through flymake-list-only-diagnostics, 
woudln't one of those values be overridden when two backends offer reports?

> Again, I don't understand your suggestion, but if you can propose it in
> the form of code it would be much better, since there would be no
> ambiguity.  A word of caution though: making these things work correctly
> in tandem with domestic diagnostics, new file visits and buffer killings
> was relatively hard.  I tried many different approaches and settled on
> these two ("foreign" and "list-only").  Of course if you can clearly
> describe a use case where they are unsuitable, a third style may be
> invented.  But I would first exhaust the possibilities in these two.

I was thinking of a way to get by with only two types: "domestic" and 
"foreign", without adding a third one. No code, sorry.

> In the meantime, please say if version-bumping project.el is OK.  That
> is the only thing holding up the merge.

Already replied to that on 13.09.2021, 23:11: yes, sure, go ahead.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.09.2021 21:03, João Távora wrote:
>> The only outstanding issue preventing me from landing this in main is
>> that I need to bump project.el's version so that the new
>> `project-buffers` API generic function becomes officially available to
>> the new bumped flymake.el version.  Dmitry is it OK for me to do so?
>
> Yes, please go ahead. All the latest additions seem stable.

Thanks.

> Like mentioned previously, I think using project.el for this is
> probably not ideal (rather, the diagnostic source can decide which
> diagnostics belong to the current default-directory),

Yes, it can, and it does (have you read the change?) In fact the
diagnostic source knows nothing about projects.  But at a certain point,
someone will have to know about "projects", because the request is not
for "default-directory-wide diagnostics", it's for project-wide
diagnostics.  I don't know what better library to use for segregating
diagnostics into projects other than project.el: it works quite well, to
be frank.

That said, if even that bothers you, the uses of project.el in
flymake.el are minimal and are the simplest part of this whole job.  If
you or someone can get rid of them and somehow keep a coherent M-x
flymake-show-project-diagnostics command, it's not unthinkable. Maybe
you'd prefer it renamed to M-x flymake-show-aggregated-diagnostics to
get the "project" word out of there.  But frankly I think the current
command name and implementation is really spot-on what was requested
here.  At least it seems to make sense to Theodor and me as a user.

Indeed, I even thought that flymake-show-project-diagnostics could go
into the 'C-x p' map.  But users can do that easily if they want to, so
I won't argue for that now.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.09.2021 23:53, João Távora wrote:
>
>>> Anyway, I asked about this because because the use of global variable
>>> (flymake-list-only-diagnostics) seems like it would make supporting
>>> multiple projects at the same time more difficult.
>> I can't guess what you are hinting at, sorry.  You can call M-x
>> flymake-show-project-diagnostics in two or more projects, of course, and
>> you'll get separate listings.  I don't know if that counts as
>> "supporting multiple projects at the same time".
>
> If a backend reports diagnostics through
> flymake-list-only-diagnostics, woudln't one of those values be
> overridden when two backends offer reports?

No.  The variable is an alist (could be another type of map), so
backends only add and remove entries about the files they know about.
The "and remove" bit is still under study.  For now I've chosen to leave
the "remove" responsibility to backends, but I've experimented with
schemes in which flymake.el does the removal strategically.  I'll
re-study that.  Also, perhaps you're being confused by the variable's
docstring, which is not as good as its entry in the manual.  I'll try to
clarify later.

>> Again, I don't understand your suggestion, but if you can propose it in
>> the form of code it would be much better, since there would be no
>> ambiguity.  A word of caution though: making these things work correctly
>> in tandem with domestic diagnostics, new file visits and buffer killings
>> was relatively hard.  I tried many different approaches and settled on
>> these two ("foreign" and "list-only").  Of course if you can clearly
>> describe a use case where they are unsuitable, a third style may be
>> invented.  But I would first exhaust the possibilities in these two.
>
> I was thinking of a way to get by with only two types: "domestic" and
> "foreign", without adding a third one.

Those two already exist.

"foreign" diagnostics belong to a given source buffer's flymake-mode but
don't target the source, rather other buffers or unvisited files.  Like
"domestic" ones they are systematically updated as the source is
updated.

They are well suited for flymake-cc and strongly typed languages where a
compilation unit is created from the strongly coupled aggregation of
many files and checked as a whole.  As you know if you've ever done
C/C++ the locus of a syntactic mistake is usually spread out through one
.cpp file and multiple .h files.

LSP servers don't systematically provide this info -- as far as I have
witnessed, so Eglot can't do much with them.  If I'm wrong, then Eglot
will also use this functionality.  The LSP protocol doesn't forbid it.
Maybe I'll approach clangd developers with the idea.

Currently, what _can_ be seen in the logs is an LSP server shooting out
a once-only batch of diagnostics for the whole project of unvisited
files when the server is started.  That is better suited to "list-only"
diagnostics.  Why "list only"?  Because once the actual file is visited,
it is assumed that flymake-mode will kick in there proper and be able to
request fresh, domestic, highlightable, diagnostics to override the
initial "list only" that came in the starting batch.

> No code, sorry.

If you want a new abnormal hook (as you seemed to propose), you have to
say, at least in some kind of pseudo-code, _when_ that hook is going to
be activated.  Maybe by doing that I could start to see the problem that
it is solving (I also don't see that).

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Theodor Thornhill <theo@thornhill.no> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> Theodor Thornhill <theo@thornhill.no> writes:
>>> I will absolutely jump at this as quickly as possible.  I'm not exactly
>>> sure how fast I'll be, since I'm on parental leave right now.
>> Congrats And enjoy it (I enjoyed mine very much)! > 
> Thanks!  Second time now, only gets better :)

I second that, too ;-)

>> Yes, it is (but the PR also works).  I hope to transfer eglot.el to
>> Emacs core soon anyway.  
> Nice.  Consider the lingering PR closed.

Thanks, and sorry it took me so long to get to it.

> From the ones I've used at work, F# [0], C# [1], Rust [2] and Elm [3]
> all support them.  I think the easiest to set up might be the rust one.
> However, I can help out with settings if needed.  I've had to subclass
> the eglot server to get reliable performance from some of them.

Thanks.  I think I'll try rust-analyzer as I've been meaning to anyway.
Perhaps make it the default Rust server for Eglot.

> By the way, I'm testing this version right now, and it seems both faster
> and more accurate.  Not sure why or what exactly happens, but it works
> flawlessly with eglot atm.

Hmmm that's good, but also odd: i didn't do any performance/accuracy
improvements that I know of.  If anything, because I changed so much
code, I would suspect the inverse: i.e. bugs.  Please keep an eye out.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

>
>>> Yes, it is (but the PR also works).  I hope to transfer eglot.el to
>>> Emacs core soon anyway.  
>> Nice.  Consider the lingering PR closed.
>
>Thanks, and sorry it took me so long to get to it.
>

No worries!


>> From the ones I've used at work, F# [0], C# [1], Rust [2] and Elm [3]
>> all support them.  I think the easiest to set up might be the rust one.
>> However, I can help out with settings if needed.  I've had to subclass
>> the eglot server to get reliable performance from some of them.
>
>Thanks.  I think I'll try rust-analyzer as I've been meaning to anyway.
>Perhaps make it the default Rust server for Eglot.
>

Absolutely, the community seems to have moved on.

>> By the way, I'm testing this version right now, and it seems both faster
>> and more accurate.  Not sure why or what exactly happens, but it works
>> flawlessly with eglot atm.
>
>Hmmm that's good, but also odd: i didn't do any performance/accuracy
>improvements that I know of.  If anything, because I changed so much
>code, I would suspect the inverse: i.e. bugs.  Please keep an eye out.

I might be wrong, of course.  My implementation could also have some defects, also.


By the way- these list only diagnostics do in fact happen on each keystroke in the mentioned servers.  Not just on the first load.  Would this mean the foreign variant is more correct to use?  

Theo  

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Hi Theodor,

I've now pushed this to master, so I'm marking the bug done.  But we
can keep discussing the Eglot patch here (or in another bug, or in
Eglot's tracker, as you prefer...)

Theodor Thornhill <theo@thornhill.no> writes:

>>Thanks.  I think I'll try rust-analyzer as I've been meaning to anyway.
>>Perhaps make it the default Rust server for Eglot.
> Absolutely, the community seems to have moved on.

Actually I did still try with `rls` since that was most handy and it
also provides diagnostics for other files on startup.  So I tested it
with rls and got decent results (see Eglot patch).

> By the way- these list only diagnostics do in fact happen on each
> keystroke in the mentioned servers.  Not just on the first load.
> Would this mean the foreign variant is more correct to use?

Yes, it would, indeed.  I don't know if _every_ server operates like
that, though.  But doesn't matter.  Are you saying that in your server,
making a modification to any file always brings in diagnostics for all
other files, too?

Anyway, I've tested a bit with Eglot and flymake-list-only-diagnostics
and it seems to do the right thing (mostly).  I attach the Eglot patch
which you can use as starting point.

diff --git a/eglot.el b/eglot.el
index 7ebc422..d036a75 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1386,6 +1386,11 @@ Doubles as an indicator of snippet support."
       (font-lock-ensure)
       (string-trim (filter-buffer-substring (point-min) (point-max))))))
 
+(defun eglot--diag-type (severity)
+  (cond ((<= severity 1) 'eglot-error)
+        ((= severity 2)  'eglot-warning)
+        (t               'eglot-note)))
+
 (define-obsolete-variable-alias 'eglot-ignored-server-capabilites
   'eglot-ignored-server-capabilities "1.8")
 
@@ -1788,8 +1793,7 @@ COMMAND is a symbol naming the command."
                      diag-spec
                    (setq message (concat source ": " message))
                    (pcase-let
-                       ((sev severity)
-                        (`(,beg . ,end) (eglot--range-region range)))
+                       ((`(,beg . ,end) (eglot--range-region range)))
                      ;; Fallback to `flymake-diag-region' if server
                      ;; botched the range
                      (when (= beg end)
@@ -1808,16 +1812,26 @@ COMMAND is a symbol naming the command."
                                 (point-at-eol
                                  (1+ (plist-get (plist-get range :end) :line)))))))
                      (eglot--make-diag (current-buffer) beg end
-                                       (cond ((<= sev 1) 'eglot-error)
-                                             ((= sev 2)  'eglot-warning)
-                                             (t          'eglot-note))
+                                       (eglot--diag-type severity)
                                        message `((eglot-lsp-diag . ,diag-spec)))))
          into diags
          finally (cond (eglot--current-flymake-report-fn
                         (eglot--report-to-flymake diags))
                        (t
                         (setq eglot--unreported-diagnostics (cons t diags))))))
-    (jsonrpc--debug server "Diagnostics received for unvisited %s" uri)))
+    (cl-loop
+     with path = (expand-file-name (eglot--uri-to-path uri))
+     for diag-spec across diagnostics
+     collect (eglot--dbind ((Diagnostic) message severity source) diag-spec
+               (setq message (concat source ": " message))
+               (eglot--make-diag path (cons 1 1) nil ; cons 1 1 bcs lazy...
+                                 (eglot--diag-type severity)
+                                 message))
+     into diags
+     finally
+     (setq flymake-list-only-diagnostics
+           (assoc-delete-all path flymake-list-only-diagnostics #'string=))
+     (push (cons path diags) flymake-list-only-diagnostics))))
 
 (cl-defun eglot--register-unregister (server things how)
   "Helper for `registerCapability'.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

On 14 September 2021 13:34:42 CEST, "João Távora" <joaotavora@gmail.com> wrote:
>Hi Theodor,
>
>I've now pushed this to master, so I'm marking the bug done.  But we
>can keep discussing the Eglot patch here (or in another bug, or in
>Eglot's tracker, as you prefer...)
>

Great.


>Theodor Thornhill <theo@thornhill.no> writes:
>
>>>Thanks.  I think I'll try rust-analyzer as I've been meaning to anyway.
>>>Perhaps make it the default Rust server for Eglot.
>> Absolutely, the community seems to have moved on.
>
>Actually I did still try with `rls` since that was most handy and it
>also provides diagnostics for other files on startup.  So I tested it
>with rls and got decent results (see Eglot patch).
>
>> By the way- these list only diagnostics do in fact happen on each
>> keystroke in the mentioned servers.  Not just on the first load.
>> Would this mean the foreign variant is more correct to use?
>
>Yes, it would, indeed.  I don't know if _every_ server operates like
>that, though.  But doesn't matter.  Are you saying that in your server,
>making a modification to any file always brings in diagnostics for all
>other files, too?
>

Yeah. For any edit in any file, _all_ diagnostics for everything gets re-sent.  I've made the assumption earlier that everything is to be flushed every time. 


>Anyway, I've tested a bit with Eglot and flymake-list-only-diagnostics
>and it seems to do the right thing (mostly).  I attach the Eglot patch
>which you can use as starting point.
>

Thank you! I'll follow up on this tonight if time permits :)

>diff --git a/eglot.el b/eglot.el
>index 7ebc422..d036a75 100644
>--- a/eglot.el
>+++ b/eglot.el
>@@ -1386,6 +1386,11 @@ Doubles as an indicator of snippet support."
>       (font-lock-ensure)
>       (string-trim (filter-buffer-substring (point-min) (point-max))))))
> 
>+(defun eglot--diag-type (severity)
>+  (cond ((<= severity 1) 'eglot-error)
>+        ((= severity 2)  'eglot-warning)
>+        (t               'eglot-note)))
>+
> (define-obsolete-variable-alias 'eglot-ignored-server-capabilites
>   'eglot-ignored-server-capabilities "1.8")
> 
>@@ -1788,8 +1793,7 @@ COMMAND is a symbol naming the command."
>                      diag-spec
>                    (setq message (concat source ": " message))
>                    (pcase-let
>-                       ((sev severity)
>-                        (`(,beg . ,end) (eglot--range-region range)))
>+                       ((`(,beg . ,end) (eglot--range-region range)))
>                      ;; Fallback to `flymake-diag-region' if server
>                      ;; botched the range
>                      (when (= beg end)
>@@ -1808,16 +1812,26 @@ COMMAND is a symbol naming the command."
>                                 (point-at-eol
>                                  (1+ (plist-get (plist-get range :end) :line)))))))
>                      (eglot--make-diag (current-buffer) beg end
>-                                       (cond ((<= sev 1) 'eglot-error)
>-                                             ((= sev 2)  'eglot-warning)
>-                                             (t          'eglot-note))
>+                                       (eglot--diag-type severity)
>                                        message `((eglot-lsp-diag . ,diag-spec)))))
>          into diags
>          finally (cond (eglot--current-flymake-report-fn
>                         (eglot--report-to-flymake diags))
>                        (t
>                         (setq eglot--unreported-diagnostics (cons t diags))))))
>-    (jsonrpc--debug server "Diagnostics received for unvisited %s" uri)))
>+    (cl-loop
>+     with path = (expand-file-name (eglot--uri-to-path uri))
>+     for diag-spec across diagnostics
>+     collect (eglot--dbind ((Diagnostic) message severity source) diag-spec
>+               (setq message (concat source ": " message))
>+               (eglot--make-diag path (cons 1 1) nil ; cons 1 1 bcs lazy...
>+                                 (eglot--diag-type severity)
>+                                 message))
>+     into diags
>+     finally
>+     (setq flymake-list-only-diagnostics
>+           (assoc-delete-all path flymake-list-only-diagnostics #'string=))
>+     (push (cons path diags) flymake-list-only-diagnostics))))
> 
> (cl-defun eglot--register-unregister (server things how)
>   "Helper for `registerCapability'.
>
>
>

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 14.09.2021 11:43, João Távora wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 13.09.2021 23:53, João Távora wrote:
>>
>>>> Anyway, I asked about this because because the use of global variable
>>>> (flymake-list-only-diagnostics) seems like it would make supporting
>>>> multiple projects at the same time more difficult.
>>> I can't guess what you are hinting at, sorry.  You can call M-x
>>> flymake-show-project-diagnostics in two or more projects, of course, and
>>> you'll get separate listings.  I don't know if that counts as
>>> "supporting multiple projects at the same time".
>>
>> If a backend reports diagnostics through
>> flymake-list-only-diagnostics, woudln't one of those values be
>> overridden when two backends offer reports?
> 
> No.  The variable is an alist (could be another type of map), so
> backends only add and remove entries about the files they know about.
> The "and remove" bit is still under study.  For now I've chosen to leave
> the "remove" responsibility to backends, but I've experimented with
> schemes in which flymake.el does the removal strategically.  I'll
> re-study that.  Also, perhaps you're being confused by the variable's
> docstring, which is not as good as its entry in the manual.  I'll try to
> clarify later.

It's a judgment call, but that seems fairly involved for a public API.

That's why I was thinking of a somewhat different shape: where a backend 
reports a full list (corresponding to the default-directory it's called 
from), and then Flymake can either show the list, or discard it at some 
later point.

Then you won't need project.el to filter the full global list, 
delegating the job of compiling the list to the backend. That seems to 
be the most flexible scheme: some authors will prefer Projectile or 
whatever, but it some cases the backend will simply have a different 
view into what a project it (which the user's config might or might not 
reflect). Subtly losing diagnostics in such scenarios seems like a bad 
outcome.

>>> Again, I don't understand your suggestion, but if you can propose it in
>>> the form of code it would be much better, since there would be no
>>> ambiguity.  A word of caution though: making these things work correctly
>>> in tandem with domestic diagnostics, new file visits and buffer killings
>>> was relatively hard.  I tried many different approaches and settled on
>>> these two ("foreign" and "list-only").  Of course if you can clearly
>>> describe a use case where they are unsuitable, a third style may be
>>> invented.  But I would first exhaust the possibilities in these two.
>>
>> I was thinking of a way to get by with only two types: "domestic" and
>> "foreign", without adding a third one.
> 
> Those two already exist.

Maybe I could say that we could have two types of diagnostics (maybe 
even just one: with file-based locations), but they would be 
differentiated by the source them came from. Some would be for the 
current buffer, and some would be for all buffers.

> "foreign" diagnostics belong to a given source buffer's flymake-mode but
> don't target the source, rather other buffers or unvisited files.  Like
> "domestic" ones they are systematically updated as the source is
> updated.
> 
> They are well suited for flymake-cc and strongly typed languages where a
> compilation unit is created from the strongly coupled aggregation of
> many files and checked as a whole.  As you know if you've ever done
> C/C++ the locus of a syntactic mistake is usually spread out through one
> .cpp file and multiple .h files.

The situation where we have a backend which returns errors only for some 
of the "other" files but not all, is not something I considered, but if 
that is indeed an important use case, no argument from me.

UI-wise, though, they could probably work as the "global" source of 
errors anyway, right? Unless the same projects usually also have a 
project-wide list of errors coming from another source.

But in the former case the syntax check would be triggered by e.g. 
editing and saving the current file -- and you should be able to both 
see the list of errors in the current file, and then the list of all 
remaining errors.

'M-x flymake-show-diagnostics-buffer' could have a slightly different 
view in this case: first show the errors for the current buffer, and 
then the errors in other files.

> LSP servers don't systematically provide this info -- as far as I have
> witnessed, so Eglot can't do much with them.  If I'm wrong, then Eglot
> will also use this functionality.  The LSP protocol doesn't forbid it.
> Maybe I'll approach clangd developers with the idea.

IIUC LSP protocol provides some kind of feed where the client can 
subscribe to the updates, and then it sends diagnostics with some 
regularity, where latency probably varies by language server.

But in any case such syntax checks much be triggered by edits and saving 
of project files.

It would be nice if 'M-x flymake-show-diagnostics-buffer' would provide 
basically the same view into that data: it would update with higher 
delays, but otherwise show the same data. Not sure if LSP can send 
reports like "syntax checking in progress" -- if so, perhaps the view 
could also reflect that.

I suppose some users would prefer to be able to show/hide info not 
pertaining to the current buffer, but that's a matter of basic filtering.

> Currently, what _can_ be seen in the logs is an LSP server shooting out
> a once-only batch of diagnostics for the whole project of unvisited
> files when the server is started.  That is better suited to "list-only"
> diagnostics.  Why "list only"?  Because once the actual file is visited,
> it is assumed that flymake-mode will kick in there proper and be able to
> request fresh, domestic, highlightable, diagnostics to override the
> initial "list only" that came in the starting batch.

I suppose the assumption is that the "list only" diagnostics cannot be 
highlightable?

>> No code, sorry.
> 
> If you want a new abnormal hook (as you seemed to propose), you have to
> say, at least in some kind of pseudo-code, _when_ that hook is going to
> be activated.  Maybe by doing that I could start to see the problem that
> it is solving (I also don't see that).

There are options. If the list-only diagnostics will only be displayed 
in the show-diagnostics buffer (and not in the source code buffers), the 
hook could run inside the *Flymake diagnostics* buffer the first time it 
is initiated. Or flymake-global-diagnostic-functions (let's call it 
that) could be called right away when a file is visited.

And maybe it would be called again and again often: whenever the current 
buffer changes, or every second after that. With the assumption that 
calling it too often would not be a problem, would not break/restart 
syntax checking processes (which is the case for LSP, for instance), it 
would just notify each global diagnostics backend whether 
default-directory has changed, and simply ask for updates (with the 
possibility of optimization that the backend would return lists which 
are 'eq' to each other if the list hasn't changed yet).

Alternatively, we could go further than the approach of hooks and make 
the global sources of diagnostics more like subscriptions. In that case 
they would be passed callbacks which are supposed to be called many 
time: every time the diagnostics change. The extra questions are how to 
"unsubscribe" and "relocate" (change default-directory), and whether to 
always "relocate" by "unsubscribing" + "subscribing". Anyway, these 
functions can still reside in a hook, with the general shape of each 
functions like

   (lambda (action &optional multi-callback))

Where ACTION is one of `subscribe', `unsubscribe' or `relocate'. And 
DEFAULT-DIRECTORY serving as an implicit argument, though it can be made 
explicit just as well.

I'm not saying this feature is easy or anything, but it would be nice to 
avoid having multiple sources editing the same alist at once (that logic 
should be part of the framework IMHO, based on the data the sources 
provide), and to let the sources themselves decide which files are part 
of their "project view". They can still use project.el underneath, if 
they find that convenient.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 14.09.2021 11:20, João Távora wrote:
>> Like mentioned previously, I think using project.el for this is
>> probably not ideal (rather, the diagnostic source can decide which
>> diagnostics belong to the current default-directory),
> Yes, it can, and it does (have you read the change?) In fact the
> diagnostic source knows nothing about projects.  But at a certain point,
> someone will have to know about "projects", because the request is not
> for "default-directory-wide diagnostics", it's for project-wide
> diagnostics.  I don't know what better library to use for segregating
> diagnostics into projects other than project.el: it works quite well, to
> be frank.

When I said "belong to default-directory", I didn't mean "reside 
inside". I meant "all diagnostics that, in the opinion of the 
diagnostics source, relate to the specified default-directory".

Basically all diagnostics belonging to the project which 
default-directory belongs to (with implicit assumption that 
default-directory can uniquely identify the project).

 > Indeed, I even thought that flymake-show-project-diagnostics could go
into the 'C-x p' map.  But users can do that easily if they want to, so
I won't argue for that now.

Uh, maybe. I was thinking we'd rather add a new keymap (in the spirit of 
Flycheck's 'C-c !') with Flymake-related commands. Then the "aggregated 
diagnostics" could be on its own button, or they could even be displayed 
by 'M-x flymake-show-diagnostics-buffer'.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It's a judgment call, but that seems fairly involved for a public API.

Certainly no more complicated than what the Flymake API already has,
what with synchronous and asynchronous invocation of callbacks with
specific arguments.  However if something justifies it, though, then
flymake-add/remove-list-only-diagnostics can be created to hide the
implemetation.

> That's why I was thinking of a somewhat different shape: where a
> backend reports a full list (corresponding to the default-directory
> it's called from), and then Flymake can either show the list, or
> discard it at some later point.

When do they report it.  In what moments?  Under what circumstances?
You address this questions, but only vaguely, at the end of your email.

> Then you won't need project.el to filter the full global list,
> delegating the job of compiling the list to the backend. That seems to
> be the most flexible scheme: some authors will prefer Projectile or
> whatever, but it some cases the backend will simply have a different
> view into what a project it (which the user's config might or might
> not reflect). Subtly losing diagnostics in such scenarios seems like a
> bad outcome.

I guess I can provide the full list of diagnostics for a user to plug in
whatever filtering function she wants.  But project filtering is what's
at stake in this request and the project library in Emacs is project.el.
I'd rather use project.el than anything else.  And rather use in the
framework, hidden from the backend's view, than talk to backends about
projects, which they know nothing about.  So it was a very simple
decision there.

> Maybe I could say that we could have two types of diagnostics (maybe
> even just one: with file-based locations), but they would be
> differentiated by the source them came from. Some would be for the
> current buffer, and some would be for all buffers.

This seems reasonably close to what the existing concepts of "foreign"
and "list-only" diagnostics is.

> IIUC LSP protocol provides some kind of feed where the client can
> subscribe to the updates, and then it sends diagnostics with some
> regularity, where latency probably varies by language server.

If I understand you correctly, that's not at all the way the the LSP
protocol handles diagnostics.  Diagnostics are sent by the server when
it feels like it.  But normally, almost 100%, they are sent by the
server very shortly after the client informs about a change in the
"virtual file".  There is no subscription.

> But in any case such syntax checks much be triggered by edits and
> saving of project files.
>
> It would be nice if 'M-x flymake-show-diagnostics-buffer' would
> provide basically the same view into that data: it would update with
> higher delays, but otherwise show the same data. Not sure if LSP can
> send reports like "syntax checking in progress" -- if so, perhaps the
> view could also reflect that.

But I don't understand what the problem with the current solution is.
Both f-s-buffer-diagnostics and f-s-project-diagnostics auto-update with
"edits and saves".

> I suppose some users would prefer to be able to show/hide info not
> pertaining to the current buffer, but that's a matter of basic
> filtering.

Ah! I guess the two things could be merged into the same command,
indeed.  That's a good idea.  Unfortunately there's the fact that when
you narrow to one buffer, the file column becomes useless.  Hiding a
column not easily accomplished in the current tabulated-list-mode
infrastructure.  But that's just a technical hurdle.

>> Currently, what _can_ be seen in the logs is an LSP server shooting out
>> a once-only batch of diagnostics for the whole project of unvisited
>> files when the server is started.  That is better suited to "list-only"
>> diagnostics.  Why "list only"?  Because once the actual file is visited,
>> it is assumed that flymake-mode will kick in there proper and be able to
>> request fresh, domestic, highlightable, diagnostics to override the
>> initial "list only" that came in the starting batch.
>
> I suppose the assumption is that the "list only" diagnostics cannot be
> highlightable?

Ye.... no.  That's their _definition_.  They are only for files which
are not yet visited, so there's nothing to highlight in the moment of
their creation.  The _assumption_ is rather that when their files are
eventually visited by Emacs and flymake-mode, we _assume_ that the
regular ensuing syntax checks deriving from the visit and or from edits
to those already brings in diagnostics.  And that is a good assumption,
if there's a traditional flymake-diagnostic-function capable of handling
that file, which there normally is.  This requires no change to that
existing part of the backend, which was desirable.

>>> No code, sorry.
>> If you want a new abnormal hook (as you seemed to propose), you have
>> to
>> say, at least in some kind of pseudo-code, _when_ that hook is going to
>> be activated.  Maybe by doing that I could start to see the problem that
>> it is solving (I also don't see that).
>
> There are options. If the list-only diagnostics will only be displayed
> in the show-diagnostics buffer (and not in the source code buffers),
> the hook could run inside the *Flymake diagnostics* buffer the first
> time it is initiated. Or flymake-global-diagnostic-functions (let's
> call it that) could be called right away when a file is visited.
>
> And maybe it would be called again and again often: whenever the
> current buffer changes, or every second after that. With the
> assumption that calling it too often would not be a problem, would not
> break/restart syntax checking processes (which is the case for LSP,
> for instance), it would just notify each global diagnostics backend
> whether default-directory has changed, and simply ask for updates
> (with the possibility of optimization that the backend would return
> lists which are 'eq' to each other if the list hasn't changed yet).
>
> Alternatively, we could go further than the approach of hooks and make
> the global sources of diagnostics more like subscriptions. In that
> case they would be passed callbacks which are supposed to be called
> many time: every time the diagnostics change. The extra questions are
> how to "unsubscribe" and "relocate" (change default-directory), and
> whether to always "relocate" by "unsubscribing" +
> "subscribing". Anyway, these functions can still reside in a hook,
> with the general shape of each functions like
>
>   (lambda (action &optional multi-callback))
>
> Where ACTION is one of `subscribe', `unsubscribe' or `relocate'. And
> DEFAULT-DIRECTORY serving as an implicit argument, though it can be
> made explicit just as well.
>
> I'm not saying this feature is easy or anything, 

I have no opinion.  It is certainly interesting.  Subscriptions,
relocations... It spikes my curiosity to read this, but no more than
that.  It tells me that you have ideas and have put in the effort to
write several paragraphs.  But I cannot see clearly the problem that you
are trying to solve.  So I cannot see value in your idea.  Nor can I
really criticize it, to be honest.  Maybe someone else will, of course,
and it's good that you wrote it down.

In contrast, I was able to see Theodor's problem clearly.  Why? Because
he made his own implementation that solves his problem so it's very very
easy to see what he means and to address what he means.  So even if we
have completely discarded that implementation, it was super-useful for
illustration.  That is not the only way to communicate software ideas,
of course, but it is certainly one of the most effective.  It's one
where many "thinkos" that are only natural when imagining vapourware
solutions are already sorted, because reality sorted them for you.  It
happens a lot with me: imagining this and having to discard them once I
"hit the metal".

> but it would be nice to avoid having multiple sources editing the same
> alist at once (that logic should be part of the framework IMHO, based
> on the data the sources provide), and to let the sources themselves
> decide which files are part of their "project view". They can still
> use project.el underneath, if they find that convenient.

Two points:

* I don't see why multiple sources editing a map is a bad thing in
  itself.  Why is it a bad thing?  Just a gut feeling?  Or real danger
  of some mishap?  Earlier you said "judgement call".  Not strong enough
  for me.
  
* I don't think that teaching sources about projects is a good thing.
  Existing sources don't know anything about that now and that doesn't
  stop them from contributing data to the list of
  flymake-show-project-diagnostics.  From the start, sources are defined
  to be concerned with syntax checking _one_ buffer.  99% percent of
  Flymake backends (and probably also Flycheck backends) do this.  I
  don't want to change them.  For the 1% that happen to have easy access
  to some kind of information about other files, there are the new
  options of "foreign" and "list-only" diags, depending on how this
  extra info is acquired.

Best regards,
João


  

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> When I said "belong to default-directory", I didn't mean "reside
> inside". I meant "all diagnostics that, in the opinion of the
> diagnostics source, relate to the specified default-directory".
>
> Basically all diagnostics belonging to the project which
> default-directory belongs to (with implicit assumption that
> default-directory can uniquely identify the project).

I think I answered this is my other email.

>> Indeed, I even thought that flymake-show-project-diagnostics could go
> into the 'C-x p' map.  But users can do that easily if they want to, so
> I won't argue for that now.
>
> Uh, maybe. I was thinking we'd rather add a new keymap (in the spirit
> of Flycheck's 'C-c !') with Flymake-related commands. Then the
> "aggregated diagnostics" could be on its own button, or they could
> even be displayed by 'M-x flymake-show-diagnostics-buffer'.

OK. Could be.  Won't argue that now.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[Dmitry Gutov]

On 17.09.2021 02:36, João Távora wrote:

>> That's why I was thinking of a somewhat different shape: where a
>> backend reports a full list (corresponding to the default-directory
>> it's called from), and then Flymake can either show the list, or
>> discard it at some later point.
> 
> When do they report it.  In what moments?  Under what circumstances?
> You address this questions, but only vaguely, at the end of your email.

Ultimately, those answers depend on the kinds of diagnostics functions 
people are going to write. LSP diagnostics, IIUC, can work with any of 
the options I described.

>> Then you won't need project.el to filter the full global list,
>> delegating the job of compiling the list to the backend. That seems to
>> be the most flexible scheme: some authors will prefer Projectile or
>> whatever, but it some cases the backend will simply have a different
>> view into what a project it (which the user's config might or might
>> not reflect). Subtly losing diagnostics in such scenarios seems like a
>> bad outcome.
> 
> I guess I can provide the full list of diagnostics for a user to plug in
> whatever filtering function she wants.  But project filtering is what's
> at stake in this request and the project library in Emacs is project.el.
> I'd rather use project.el than anything else.  And rather use in the
> framework, hidden from the backend's view, than talk to backends about
> projects, which they know nothing about.  So it was a very simple
> decision there.

But do they? Know nothing about it? Consider where "list diagnostics" 
can come from. If those span not just the current file and maybe a 
couple of files which it include<>-s (in the case of C/C++), they have 
to span a whole set of files, which often corresponds to the notion of 
the project, as understood by some tool or configuration file. So the 
knowledge about the project bounds seems necessary to even be able to 
generate the global diagnostics.

>> Maybe I could say that we could have two types of diagnostics (maybe
>> even just one: with file-based locations), but they would be
>> differentiated by the source them came from. Some would be for the
>> current buffer, and some would be for all buffers.
> 
> This seems reasonably close to what the existing concepts of "foreign"
> and "list-only" diagnostics is.

Yes: I'd rather those use the same container type. Maybe even the same 
reporting mechanism.

>> IIUC LSP protocol provides some kind of feed where the client can
>> subscribe to the updates, and then it sends diagnostics with some
>> regularity, where latency probably varies by language server.
> 
> If I understand you correctly, that's not at all the way the the LSP
> protocol handles diagnostics.  Diagnostics are sent by the server when
> it feels like it.  But normally, almost 100%, they are sent by the
> server very shortly after the client informs about a change in the
> "virtual file".  There is no subscription.

"Server sends" is an event source. It doesn't react to you calling 
diagnostic functions, it only vaguely reacts to user actions, which then 
get translated into notifications for the server, and those, 
asynchronously, result in diagnostic events.

Events one could subscribe to.

>> I suppose some users would prefer to be able to show/hide info not
>> pertaining to the current buffer, but that's a matter of basic
>> filtering.
> 
> Ah! I guess the two things could be merged into the same command,
> indeed.  That's a good idea.  Unfortunately there's the fact that when
> you narrow to one buffer, the file column becomes useless.  Hiding a
> column not easily accomplished in the current tabulated-list-mode
> infrastructure.  But that's just a technical hurdle.

I figured it could be more like a horizontal header. But those might be 
even harder to do, in tabulated-list-mode.

>>> Currently, what _can_ be seen in the logs is an LSP server shooting out
>>> a once-only batch of diagnostics for the whole project of unvisited
>>> files when the server is started.  That is better suited to "list-only"
>>> diagnostics.  Why "list only"?  Because once the actual file is visited,
>>> it is assumed that flymake-mode will kick in there proper and be able to
>>> request fresh, domestic, highlightable, diagnostics to override the
>>> initial "list only" that came in the starting batch.
>>
>> I suppose the assumption is that the "list only" diagnostics cannot be
>> highlightable?
> 
> Ye.... no.  That's their _definition_.  They are only for files which
> are not yet visited, so there's nothing to highlight in the moment of
> their creation.

It seems more like a technical decision rather than definition.

To put it another way: suppose each of 
flymake-global-diagnostic-functions returns a list of values of 
diagnostics. Would those be generally suitable for highlighting files?

> The _assumption_ is rather that when their files are
> eventually visited by Emacs and flymake-mode, we _assume_ that the
> regular ensuing syntax checks deriving from the visit and or from edits
> to those already brings in diagnostics.

If we're talking about LSP-based diagnostics, would those then come from 
some different source? Or would that just be the same information, 
repackaged through the classic hook?

> And that is a good assumption,
> if there's a traditional flymake-diagnostic-function capable of handling
> that file, which there normally is.  This requires no change to that
> existing part of the backend, which was desirable.

I'm thinking that if LSP servers only provide "global" diagnostics, they 
could simply provide all their diagnostics through the new abnormal 
hook. Then changing the "existing part of the backend" might likewise be 
unnecessary.

> In contrast, I was able to see Theodor's problem clearly.  Why? Because
> he made his own implementation that solves his problem so it's very very
> easy to see what he means and to address what he means.  So even if we
> have completely discarded that implementation, it was super-useful for
> illustration.  That is not the only way to communicate software ideas,
> of course, but it is certainly one of the most effective.  It's one
> where many "thinkos" that are only natural when imagining vapourware
> solutions are already sorted, because reality sorted them for you.  It
> happens a lot with me: imagining this and having to discard them once I
> "hit the metal".

That is entirely possible. I just wanted to touch on this discussion 
because it relates to a package I maintain. I'm not currently an LSP 
user, and other things require my attention.

>> but it would be nice to avoid having multiple sources editing the same
>> alist at once (that logic should be part of the framework IMHO, based
>> on the data the sources provide), and to let the sources themselves
>> decide which files are part of their "project view". They can still
>> use project.el underneath, if they find that convenient.
> 
> Two points:
> 
> * I don't see why multiple sources editing a map is a bad thing in
>    itself.  Why is it a bad thing?  Just a gut feeling?  Or real danger
>    of some mishap?  Earlier you said "judgement call".  Not strong enough
>    for me.

It loses the "here is the whole global list of diagnostics related to 
the current file" type of information. I don't like losing information 
in general, but also I can easily imagine (possibly infrequent) cases 
where the underlying tool's understanding of the project will differ 
from the user's configuration of project.el (or the default behavior, 
when there was no configuration).

So imagine: the tool (diagnostics backend) reports a bunch of errors, 
the user pops up the project diagnostics buffer, fixes the errors one by 
one... and then the project still fails to build because some of the 
related errors were filtered out when you used project.el to choose 
which errors to show.

> * I don't think that teaching sources about projects is a good thing.
>    Existing sources don't know anything about that now and that doesn't
>    stop them from contributing data to the list of
>    flymake-show-project-diagnostics.  From the start, sources are defined
>    to be concerned with syntax checking _one_ buffer.  99% percent of
>    Flymake backends (and probably also Flycheck backends) do this.  I
>    don't want to change them.  For the 1% that happen to have easy access
>    to some kind of information about other files, there are the new
>    options of "foreign" and "list-only" diags, depending on how this
>    extra info is acquired.

So we keep the old per-buffer hook for the one-buffer checkers and use 
the new abnormal hook for the newly discovered exceptions. No?

Maybe flymake-global-diagnostic-functions could even be shoehorned for 
the C/C++ case of not-entirely-global checkers: those checks would be 
triggered by an element in flymake-diagnostic-functions, report the 
"local" diagnostics to the local reporter, and then the full list 
through its flymake-global-diagnostic-functions element. The question 
is, which code would add the flymake-global-diagnostic-functions element.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 17.09.2021 02:36, João Távora wrote:
> Ultimately, those answers depend on the kinds of diagnostics functions
> people are going to write. LSP diagnostics, IIUC, can work with any of
> the options I described.

Yes, but currently they work in one specific way.  The new Flymake is
compatible to that way (and all the other backends way).  That's so that
"people" _don't_ have to write new backends or modify the existing ones,
to make at least partial use of new Flymake functionality, M-x
flymake-show-project-diagnostics

> But do they? Know nothing about it?

I think so, yes.  The Flymake backends that live in the Emacs source
tree are examples, and so are many that live in Elpa.  They are all
concerned with the buffer.

> Consider where "list diagnostics" can come from. If those span not
> just the current file and maybe a couple of files which it include<>-s
> (in the case of C/C++), they have to span a whole set of files, which
> often corresponds to the notion of the project, as understood by some
> tool or configuration file. So the knowledge about the project bounds
> seems necessary to even be able to generate the global diagnostics.

Yes, I see what you mean.  Here, again, is an attempt to explain what
how it works.  That's up to the backend: if it knows these things, and
if they are useful to its work, fine.  But they aren't required and
should not be required by Flymake.  The flymake-cc backend uses a
Makefile for example.  Is that "the project"?  Sometimes it is,
sometimes not.  Flymake, the framework, doesn't care.  It cares about
files in a file system.  Backends tell it about problems in the current
buffer and the associated files.  Now they are able to tell it about
other related files, within any criteria of relation they see fit.

> "Server sends" is an event source. It doesn't react to you calling
> diagnostic functions, it only vaguely reacts to user actions, which
> then get translated into notifications for the server, and those,
> asynchronously, result in diagnostic events.
>
> Events one could subscribe to.

I see.  There's nothing in LSP that tells me that model is correct, but
nothing telling me it's wrong either.

> It seems more like a technical decision rather than definition.

It's both.  Making working definitions is technical work.  I define
things so that I can operate on them comfortably and succintly.  I could
have defined "list only diagnostics" it in terms of Aunt Mary's cake,
but it would probably make for a lot more code to write, unfriendly
documentation API, etc.

It's just like you devising the definition of "subscription".  It's a
technical decision.  It's one yet to see light of day, but it's a
decision nonetheless.

> To put it another way: suppose each of
> flymake-global-diagnostic-functions returns a list of values of
> diagnostics. Would those be generally suitable for highlighting files?

I don't know!  I don't know that flymake-global-diagnostic-functions
does, because you haven't written it or described it suitably.  It's
very hard to be your imagination's interpreter.  You should type these
things in Elisp and then use the Elisp interpreter/compiler.

>> The _assumption_ is rather that when their files are
>> eventually visited by Emacs and flymake-mode, we _assume_ that the
>> regular ensuing syntax checks deriving from the visit and or from edits
>> to those already brings in diagnostics.
>
> If we're talking about LSP-based diagnostics, would those then come
> from some different source? Or would that just be the same
> information, repackaged through the classic hook?

If I understand you question correctly: the latter, but only sometimes.
Servers can provide new fresh info when you visit files, so it wouldn't
be the "same information".  But as for the "classic hook" being used for
that: yes.

>> And that is a good assumption,
>> if there's a traditional flymake-diagnostic-function capable of handling
>> that file, which there normally is.  This requires no change to that
>> existing part of the backend, which was desirable.
>
> I'm thinking that if LSP servers only provide "global" diagnostics,
> they could simply provide all their diagnostics through the new
> abnormal hook. Then changing the "existing part of the backend" might
> likewise be unnecessary.

Maybe that works. Maybe not.  To solve what problem? I don't know.  You
don't seem to describe this problem, am I incapable of devining it.  But
if you can see and you can program it, maybe it becomes obvious then.

>> where many "thinkos" that are only natural when imagining vapourware
>> solutions are already sorted, because reality sorted them for you.  It
>> happens a lot with me: imagining this and having to discard them once I
>> "hit the metal".
> That is entirely possible. I just wanted to touch on this discussion
> because it relates to a package I maintain. I'm not currently an LSP
> user, and other things require my attention.

I think, without wanting to overly judge your stance, that that is more
than slightly unproductive.  (1) I solve a problem which uses "your"
project.el library and its existing API (requests no changes to it).
(2) You argue long-windedly that there is a much better solution to
solve the problem, and presumably some other problems.  (3) Finally say
that you don't have direct contact and experience with the base matter,
and that you don't have time to develop your ideas formally.  I don't
even know if you've tried the solution that I've presented and
encoutered some difficulty.

I have to say that it feels a bit unfair for me who has sit down and
spent actual hours of my life working on something real.  This pattern
repeats more or less in many other parts of Emacs that I usually work
on.  I take Eli's frequent advice: "sit down and do the work".  It's
excellent advice.  Then my ideas are more convincing.  Others and I are
more happy and less exhausted to debate them.  I am going to snip the
rest of the conversation, which is repeating the same points.  I have
nothing to add.

Best regards,
João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

Hi João!

(sent this mail some days ago, but the bug was archived, so not sure if
it worked.  Trying again.  If this is only noise, then I'm sorry, but
hopefully ok considering i found a typo in the old patch :))

I finally did get to test this out a little, and I seem to have hit some
small snags I can't really understand.  I'd love some pointers as to how
to continue!

(the patch I currently use is attached at the bottom)

Ok, so what happens is:

- There is no backend set on the list-only-diagnostics as for now, and I
  cannot for the life of me see how to set them, as it doesn't look like
  eglot itself sets it.  Is there some reflection going on here that I
  have missed?

- The function in question is riddled with fallbacks and failure
  checking.  Are we sure we need all that?  I see most of the commits
  relating to the range checking is from 2018, so they may not be
  relevant anymore?

- The diff as supplied appends the list-only-diagnostics just fine to
  the flymake buffer, but I believe since the backend isn't set, eglot
  doesn't report the proper diagnostics when I visit the file.  I have
  to make a bogus edit to trigger the eglot-flymake-backend function.
  (This could just as well be some unrelated eglot/elmls issue...)

- What would be the best way to get the proper line and column?  Now I
  simply use the range, but considering there's a lot of error handling
  there I guess that is a little risky?  I did have a version earlier
  using with-temp-buffer and finding the correct positions, but that
  seemed more complicated than needed.

I'd love to get your thoughts on this :)

Theodor


diff --git a/eglot.el b/eglot.el
index bf9cf25..47c7b74 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1776,9 +1776,14 @@ COMMAND is a symbol naming the command."
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event") ;; noop, use events buffer
 
+(defun eglot--diag-type (severity)
+  (cond ((<= severity 1) 'eglot-error)
+        ((= severity 2)  'eglot-warning)
+        (t          'eglot-note)))
+
 (cl-defmethod eglot-handle-notification
   (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
-          &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
+          &allow-other-keys)        ; FIXME: doesn't respect `eglot-strict-mode'
   "Handle notification publishDiagnostics"
   (if-let ((buffer (find-buffer-visiting (eglot--uri-to-path uri))))
       (with-current-buffer buffer
@@ -1787,9 +1792,7 @@ COMMAND is a symbol naming the command."
          collect (eglot--dbind ((Diagnostic) range message severity source)
                      diag-spec
                    (setq message (concat source ": " message))
-                   (pcase-let
-                       ((sev severity)
-                        (`(,beg . ,end) (eglot--range-region range)))
+                   (pcase-let ((`(,beg . ,end) (eglot--range-region range)))
                      ;; Fallback to `flymake-diag-region' if server
                      ;; botched the range
                      (when (= beg end)
@@ -1807,17 +1810,29 @@ COMMAND is a symbol naming the command."
                           (setq end
                                 (point-at-eol
                                  (1+ (plist-get (plist-get range :end) :line)))))))
-                     (eglot--make-diag (current-buffer) beg end
-                                       (cond ((<= sev 1) 'eglot-error)
-                                             ((= sev 2)  'eglot-warning)
-                                             (t          'eglot-note))
-                                       message `((eglot-lsp-diag . ,diag-spec)))))
+                     (eglot--make-diag
+                      (current-buffer) beg end
+                      (eglot--diag-type severity)
+                      message `((eglot-lsp-diag . ,diag-spec)))))
          into diags
          finally (cond (eglot--current-flymake-report-fn
                         (eglot--report-to-flymake diags))
                        (t
                         (setq eglot--unreported-diagnostics (cons t diags))))))
-    (jsonrpc--debug server "Diagnostics received for unvisited %s" uri)))
+    (cl-loop
+     with path = (expand-file-name (eglot--uri-to-path uri))
+     for diag-spec across diagnostics
+     collect (eglot--dbind ((Diagnostic) range message severity source) diag-spec
+               (setq message (concat source ": " message))
+               (let* ((start (plist-get range :start))
+                      (line (1+ (plist-get start :line)))
+                      (char (1+ (plist-get start :character))))
+                 (eglot--make-diag path (cons line char) nil (eglot--diag-type severity) message)))
+     into diags
+     finally
+     (setq flymake-list-only-diagnostics
+           (assoc-delete-all path flymake-list-only-diagnostics #'string=))
+     (push (cons path diags) flymake-list-only-diagnostics))))
 
 (cl-defun eglot--register-unregister (server things how)
   "Helper for `registerCapability'.

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

[João Távora]

On Sat, Oct 23, 2021 at 8:22 PM Theodor Thornhill <theo@thornhill.no> wrote:
> (sent this mail some days ago, but the bug was archived, so not sure if
> it worked.  Trying again.  If this is only noise, then I'm sorry, but
> hopefully ok considering i found a typo in the old patch :))

No problem, and yes I got the other one.  It's just I've been again
ompletely utterly busy so it'll be a while.  I took the Flymake thing out
of my brain cache and it's going to take a while to get it back.

> I finally did get to test this out a little, and I seem to have hit some
> small snags I can't really understand.  I'd love some pointers as to how
> to continue!
> (the patch I currently use is attached at the bottom)
> Ok, so what happens is:
>
> - There is no backend set on the list-only-diagnostics as for now, and I
>   cannot for the life of me see how to set them, as it doesn't look like
>   eglot itself sets it.  Is there some reflection going on here that I
>   have missed?

No, I don't think so.  Maybe I simply decided that no backend was
necessary?  Is it?  If it is, we might to add some mechanism so
that it is assigned.

> - The function in question is riddled with fallbacks and failure
>   checking.  Are we sure we need all that?  I see most of the commits
>   relating to the range checking is from 2018, so they may not be
>   relevant anymore?

I'm not sure I follow here.  I wouldn't delete any checks until I'm
sure they are doing nothing. 2018 or not, I do remember a lot of
servers providing very strange diagnostics and invalid ranges.

> - The diff as supplied appends the list-only-diagnostics just fine to
>   the flymake buffer, but I believe since the backend isn't set, eglot
>   doesn't report the proper diagnostics when I visit the file.  I have
>   to make a bogus edit to trigger the eglot-flymake-backend function.
>   (This could just as well be some unrelated eglot/elmls issue...)

Ah, this rings a bell. You're talking about visiting a file by clicking a
"list-only  diagnostic" in the list, right?  And then you wnat Flymake to
resume checking and replace them with "real" ones.  Yes, might be
eglot/elmls issue, but usually there's not much we can do: some servers
are like that.

To keep things consistent we could remove the relevant list-only
diagnostics and re-report them via the normal mechanism if the server
is taking too long.

Do you understand this idea? The user wouldn't, in principle, be able to
tell that the freshly annotated diagnostics weren't collected just now
when he visited the buffer, but much much earlier.  The project-wide
listing should, in principle, also remain consistent.

...though probably unsorted.  If I remember correctly consistent sorting
is still a problem.

> - What would be the best way to get the proper line and column?  Now I
>   simply use the range, but considering there's a lot of error handling
>   there I guess that is a little risky?  I did have a version earlier
>   using with-temp-buffer and finding the correct positions, but that
>   seemed more complicated than needed.

Risky for what?  I don't follow

If you used with-temp-buffer, I guess you visited the file.  That would
go against the idea of list-only-diagnostics, which are supposed
to be for unvisited files (if the file were visited you'd have Eglot/Flymake
actually checking).  So using the LSP range and converting it to
flymake-make-diagnostic's protocol seems ok.

> I'd love to get your thoughts on this :)

Hope I could help and sorry if the thoughts were a little sparse.
As I said, this is out of my cache and will be for some time.

Do consider patches/changes to flymake.el as well. This new
mechanism is very new, it's possible it needs some tweaks.

João

bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el

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

João Távora <joaotavora@gmail.com> writes:

> On Sat, Oct 23, 2021 at 8:22 PM Theodor Thornhill <theo@thornhill.no> wrote:
>> (sent this mail some days ago, but the bug was archived, so not sure if
>> it worked.  Trying again.  If this is only noise, then I'm sorry, but
>> hopefully ok considering i found a typo in the old patch :))
>
> No problem, and yes I got the other one.  It's just I've been again
> ompletely utterly busy so it'll be a while.  I took the Flymake thing out
> of my brain cache and it's going to take a while to get it back.
>

Didn't mean to rush things, we do have time :)

>> I finally did get to test this out a little, and I seem to have hit some
>> small snags I can't really understand.  I'd love some pointers as to how
>> to continue!
>> (the patch I currently use is attached at the bottom)
>> Ok, so what happens is:
>>
>> - There is no backend set on the list-only-diagnostics as for now, and I
>>   cannot for the life of me see how to set them, as it doesn't look like
>>   eglot itself sets it.  Is there some reflection going on here that I
>>   have missed?
>
> No, I don't think so.  Maybe I simply decided that no backend was
> necessary?  Is it?  If it is, we might to add some mechanism so
> that it is assigned.
>

I don't think a backend really is necessary anymore.  I've been checking
with more servers lately, and it seems to work better with some than
with others.  And especially elmls seems a little wonky here.

>> - The function in question is riddled with fallbacks and failure
>>   checking.  Are we sure we need all that?  I see most of the commits
>>   relating to the range checking is from 2018, so they may not be
>>   relevant anymore?
>
> I'm not sure I follow here.  I wouldn't delete any checks until I'm
> sure they are doing nothing. 2018 or not, I do remember a lot of
> servers providing very strange diagnostics and invalid ranges.
>

Yeah, I didn't remove any checks yet, and likely will not.  I'm merely
guessing that the lsp scene has settled a bit lately.

>> - The diff as supplied appends the list-only-diagnostics just fine to
>>   the flymake buffer, but I believe since the backend isn't set, eglot
>>   doesn't report the proper diagnostics when I visit the file.  I have
>>   to make a bogus edit to trigger the eglot-flymake-backend function.
>>   (This could just as well be some unrelated eglot/elmls issue...)
>
> Ah, this rings a bell. You're talking about visiting a file by clicking a
> "list-only  diagnostic" in the list, right?  And then you wnat Flymake to
> resume checking and replace them with "real" ones.  Yes, might be
> eglot/elmls issue, but usually there's not much we can do: some servers
> are like that.
>

Yes, you are probably correct here. 

> To keep things consistent we could remove the relevant list-only
> diagnostics and re-report them via the normal mechanism if the server
> is taking too long.
>

I'll look into this, and see if I can find a solution for this.

> Do you understand this idea? The user wouldn't, in principle, be able to
> tell that the freshly annotated diagnostics weren't collected just now
> when he visited the buffer, but much much earlier.  The project-wide
> listing should, in principle, also remain consistent.
>
> ...though probably unsorted.  If I remember correctly consistent sorting
> is still a problem.
>

Yes sorting is a little off, and I'm looking into that as well.  I think
we need to sort on filename, line and col in that order to get it
correct, right?

>> - What would be the best way to get the proper line and column?  Now I
>>   simply use the range, but considering there's a lot of error handling
>>   there I guess that is a little risky?  I did have a version earlier
>>   using with-temp-buffer and finding the correct positions, but that
>>   seemed more complicated than needed.
>
> Risky for what?  I don't follow
>

I'm thinking that if I avoid doing the same error handling as in the
normal case (which requires a live buffer), chances are that the range
may be wrong.  But since I didn't yet see such a case, I'm thinking some
of the error handling is redundant.  I don't have enough proof yet to
remove them, but there may be some problems that I didn't yet discover
with the patch I sent.

> If you used with-temp-buffer, I guess you visited the file.  That would
> go against the idea of list-only-diagnostics, which are supposed
> to be for unvisited files (if the file were visited you'd have Eglot/Flymake
> actually checking).  So using the LSP range and converting it to
> flymake-make-diagnostic's protocol seems ok.
>

Great! Yes, I didn't want to visit the buffer, but that seemed as a good
way to go at the time, considering that it at least won't clutter the
bufferlist with every file in a project on load :)

>> I'd love to get your thoughts on this :)
>
> Hope I could help and sorry if the thoughts were a little sparse.
> As I said, this is out of my cache and will be for some time.
>

No, this is great.  It confirmes some of my hunches, so that is good at
least.  

> Do consider patches/changes to flymake.el as well. This new
> mechanism is very new, it's possible it needs some tweaks.
>

I think most of what _I_ need is covered, in some form or other.  I'll
prepare a patchset for both eglot and maybe flymake sometime soon, and
you can look at it when you have some free time and energy.

Theo