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

[Dmitry Gutov <dgutov@yandex.ru>]

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.