Re: Flymake refactored

[Stefan Monnier]

>> We should change checkdoc.el directly to avoid such hackish surgery.
>> ...
>> Similarly, we should change directly byte-compile-log-warning such that
>> tnhis kind of surgery is not needed.
> Sure, no problem, i was in a hurry and didn't want to touch more
> files.

Yes, of course I understand that.

> (...but didn't you use to be the advocate for add-function
> everywhere, as in add-function versus hooks? Isn't this slightly
> better?)

A `foo-function` variable (manipulated with add-function) or
a `foo-functions` (manipulated with add-hook) is a completely different
issue than (cl-letf (((symbol-function 'foo) ...)) ...).

The first two mean that the code was designed specifically to allow such
customization and e use the advertized API for it, whereas the second
means that we opportunistically take advantage of some accidental
details of the code.

Also the first two can be modified buffer-locally contrary to the second
which naturally has a global effect.

>>> +(defun flymake-elisp-checkdoc (report-fn)
>>> +  "A flymake backend for `checkdoc'.
>>> +Calls REPORT-FN directly."
>>> +  (when (derived-mode-p 'emacs-lisp-mode)
>> This test should not be needed.
> ..if flymake-elisp-checkdoc is always placed in the correct buffer-local
> value of flymake-diagnostic-functions.

Indeed.  It will be installed there by elisp-mode.  I see no reason to
assume that hordes of users are going to come and install it on the
global part of the hook.  And if they do, they get what they ask for.

>> This also means remove the (require 'flymake-ui)
>> and instead mark `flymake-make-diagnostic` as autoloaded.
> Hmmm. If I read your reasoning correctly, you want to avoid
> elisp-mode.el knowing about flymake and also avoiding a compilation
> warning.

Not really: I just don't want to preload flymake-ui into the dumped Emacs.

> But isn't having flymake-make-diagnostic and the hook already
> "knowing" about flymake)?

I think flymake should become part of the global infrastructure which
major modes will usually want to support.  Like imenu, font-lock,
indent-line-function, eldoc, prettify-symbols, etc...

For that, they will have to know something about flymake, of course (at
the very least they'll need to write a backend function for it).
That doesn't necessarily mean requiring flymake, tho, since the user may
or may not use flymake (again, like imenu, font-lock, eldoc,
prettify-symbols, ...).

> Isn't (eval-when-compile (require
> 'flymake-ui)) better?

Could be, but the byte-compiler will then still complain that you're
calling flymake-make-diagnostic while it doesn't know that it will be
available at runtime.

>> This smells of legacy: just as we do for Elisp, it should be the major
>> mode which specifies how to get the diagnostics, so we don't need to
>> care about filenames.
>> So this should be marked as obsolete.
> Sure, but you're assuming a correspondence between the cars of those and
> major modes. And to be honest that really sounds like boring work. My
> plan was to leave flymake-proc.el quiet in a dark corner and
> flymake-proc-legacy-flymake in the global hook until we write proper
> backends for a sufficient number of modes.

We're in violent agreement.

>> If flymake-proc.el is considered legacy that should disappear in the
>> long run, then I'm not sure all that renaming is justified (we can keep
>> using the old names until we get rid of them altogether).
> I don't agree, I like the -proc prefix to remind me not to touch it.

With all the "flymake-proc--" aliases out of the way, maybe there are
sufficiently few remaining aliases that I can agree (I haven't looked
at the result yet).

>> Also some parts of flymake-proc seem to make sense for "all" backends
>> rather than only for flymake-proc.
> Sure, they should be extracted into some flymake-util.el or
> flymake-common.el or just flymake.el. But they could just be rewritten.

Let's try and move the ones that were sufficiently well designed that we
can keep using them in flymake.el without regret.  For the others, they
can definitely stay in flymake-proc.el.

>> Some concrete examples:
>>
>>> +  (define-obsolete-variable-alias 
>>> 'flymake-compilation-prevents-syntax-check
>>> +    'flymake-proc-compilation-prevents-syntax-check "26.1"
>>> +    "If non-nil, don't start syntax check if compilation is running.")
>>
>> This doesn't look specific to the specific flymake-proc backend, so
>> maybe it should stay in flymake(-ui).el
> IDK, there's no real conflict AFAIK, it's just a convenience. And M-x
> compile is for external processes just like flymake-proc.el. But OK.

IIUC you're saying it's just not useful enough to keep it in flymake.el?

>> (:constructor flymake-make-diagnostic (buffer beg end type
>> text))
> Also that, but actually I kept that indirection to remind me that I
> probably want to create different objects for different types of objects
> in the future. But then didn't go that way.

That makes sense as well.  I guess the main thing for me is to try not
to go through the default constructor of cl-defstruct (the one with all
the key arguments), since it expands to a fairly large function, and
calls to it need a somewhat costly compiler-macro to turn them into
efficient code.  The inefficiency is nothing to worry about, but if we
don't make use of the feature, it's just pure waste.

>>> +* The symbol `:progress', signalling that the backend is still
>>> +  working and will call REPORT-FN again in the future.
>> This description leaves me wondering why a backend would ever want to
>> do that.  What kind of effect does it have on the UI?
> Nothing for the moment. But consider in the future that the UI wants to
> know if it should wait for a backend that is taking too long to
> report. This could be its keepalive. Or maybe your idea of calling
> REPORT-FN multiple times takes care of it.

I see.  Indeed, then, a value of nil would play the same role, I guess.

>> I don't understand this last paragraph.  AFAICT from this docstring,
>> REPORT-FN will be a function which takes a single argument, so I don't
>> know what "provide REPORT-FN with a string as the keyword argument
>> `:explanation'" means.  Does it mean something like
>> (funcall REPORT-FN `(:explanation ,str))
> No it means
> (funcall REPORT-FN (nth (random 2)'(:panic :progress)) :explanation "this") 

Ah, I see.  I guess it should be rephrased to make it more clear, then.

>> Regarding flymake-diagnostic-functions I've been wondering about its
>> behavior for very large buffers.  More specifically I've been wondering
>> whether it would make sense to:
>> A- provide to the backend function the BEG/END of the region changed
>> since the last time we called it.
>> B- make it possible to focus the work on the currently displayed part of
>> the buffer.
>> But I guess it's not worth the effort: most/all backends won't be able
>> to make any use of that information anyway.

> This makes sense.  The backend is called in the buffer so it has B.

Well, it doesn't really have B: it could try and compute it with
get-buffer-window-list and such, but it wouldn't easily be able to take
into account which parts were already covered by previous calls, etc...

> I was also very much thinking of making A, by collecting regions in
> flymake-after-change-function, and storing them in a buffer-local
> variable, like flymake-check-start-time or flymake-last-change-time.
> Or perhaps dynamically binding it around the backend call.  Or just
> passing them as arguments.

But which kind of backend would be able to make good use of such info?
There might be a few (mostly the rare few implemented in Elisp,
I guess), but I'm not sure it's worth the trouble (it implies a lot of
work both on flymake side and on the backend side).

For several checkers a change to line L2 might cause changes to the
diagnostics on lines before L2, so for many backends the only way to
work is "globally".

If we want to link something like nxml's checker into flymake in a good
way, we'll probably just need a completely different hook than
flymake-diagnostic-functions.

> How would I mixin existing stuff for a supposed :my-error that is
> just like :error, but overrides some properties.

Not sure I understand the question: how would you do it with the
flymake-diagnostic-types-alist you currently have?

>> `flymake-ui.el` should be renamed flymake.el (which will solve the
>> above yuckiness).  Admittedly, this will cause problems with mutual
>> dependencies between flymake.el and flymake-proc.el, so it'll take some
>> effort to solve these issues.
> Perhaps solved by making the function flymake-proc-legacy-flymake, the
> variable flymake-proc-err-line-patterns, and some others autoloaded?

Could be, I haven't looked at them (and don't have access to the code
right now).

> Already fixed:
> * Scrap the M-n, M-p commit.
> * Delete many obsolete aliases.
> * :group in defcustom
> * flymake-make-diagnostic docstring
> * overlay property is just 'flymake
> * cl-remove-if-not and cl-sort

Great.

> Will fix ASAP:
>
> * Move flymake-compilation-prevents-syntax-check to flymake-ui.el
> * flymake.el autoload yuckiness.
> * Make a flymake-category.
> * Make flymake-diagnostic-functions a hook. Use run-hook-wrapped.
> * Obsolete alias for flymake-mode-on/off
> * Don't use hash-table-keys, unless I really need it.

Sounds good.

> Will fix, but harder:
> * proper checkdoc.el interface
> * proper byte-compile.el interface
> * Move flymake-elisp.el contents to elisp-mode.el

I can help with that.

> * Allow REPORT-FN to be called multiple times from backends

There's no hurry for that.

> * Passing recent changes to backends

Maybe we should keep that for later, to avoid overengineering the API.

> * Rename flymake-ui.el to flymake.el and make some autoloads.

That'd be good.

> Have doubts:
> * remove starting test in flymake-elisp-checkdoc
> * Use defstruct constructor for flymake-make-diagnostic
> * A replacement for &key in REPORT-FN's lambda list.

None of those are really important, I was just voicing my preference.

> * mark flymake-proc-err-line-patterns obsolete and add to
>   some other variable from across emacs progmodes.

I only meant to mark it as obsolete.  But if the whole of flymake-proc
is considered obsolete (or close enough), then it's not even worth it.

> * remove the -proc prefix from defs in the proc backend.

Only when it avoids obsolete aliases.  And again it's just my own
preference, nothing really important.

> * reconsider parts from flymake-proc into an util package.

If that makes sense, yes.


        Stefan