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

[Theodor Thornhill]

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