Re: Flymake refactored

[João Távora]

Hi Stefan,

> This looks really good, thank you very much.  I think this kind of
> functionality is very important and we should strive to polish it

Thanks, and thanks very much for such a speedy review. Before I start
replying to it, I just wanted to say that I'll have to switch context
again away from hacking very soon, so I wanted to get the most out of
these early review cycles

At the end of this mail, I've summarized a list of the changes you
suggested, from less to more controversial. If you see something trivial
that you can fix right away, or something less trivial but
uncontroversial, just push it to the branch.

> I'm sorry I didn't give you feedback earlier, so here's my impressions
> on a fairly quick look at the overall diff.

No problem. I've only picked it up the last week or so. It was sitting
pretty quiet in the scratch branch in a state that didn't really merit
reviewing. From my perspective, 6 hours between asking and getting a
decent review is pretty good.

> Right, as you guessed I don't think this is acceptable for the default
> (I use M-n/M-p bindings in my smerge-mode config and am pretty happy

Well, in your case I would like to use M-n/M-p for flymake and overriden
vwhen smerge-mode kicks in, which is a much more serious kind of
conflict. When smerge-mode turns itself off again (it doesn't lately,
but it used to), you get flymake again.

OTOH, perhaps it's a good thing they are empty. They should be reserved
for the user IMO.

> Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p`
> for it, but I'm not sure if it will work well (the problem with the
> `next-error` thingy is that there can be many different "kinds" or
> error sources, so flymake would end up competing with compile.el, grep,
> etc...).

Agree. Let's not do anything :-). I didn't mention it, but you can
navigate through errors with the mouse wheel on the little red/yellow
indicators.

>> +;; Copyright (C) 2017  João Távora
>
> Of course, this should say FSF instead.

:-)

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

>> +(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. IDK, is it a good idea to admit
that (lazy) users place everything in the global hook? Inn which case it
should actually error, which is a synchronous form of panic that causes
the UI to disable it for the buffer.

>> +(defun flymake-elisp--batch-byte-compile (&optional file)
> [...]
>> +    (unwind-protect
>> +        (cl-letf (((symbol-function 'byte-compile-log-warning)
>> +                   (lambda (string &optional fill level)
>> +                     (push (list string byte-compile-last-position fill 
>> level)
>> +                           collected)
>> +                     t)))
>

>
>> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
>> +               'flymake-elisp-checkdoc t)
>> +  (add-to-list (make-local-variable 'flymake-diagnostic-functions)
>> +               'flymake-elisp-byte-compile t))
>
> `flymake-diagnostic-functions` should be a(n abnormal) hook which we
> manipulate with `add-hook`:
>
>     (add-hook 'flymake-diagnostic-functions #'flymake-elisp-checkdoc nil t)
>     (add-hook 'flymake-diagnostic-functions
>     #'flymake-elisp-byte-compile nil t)

No problem. I actually wanted a hook, but no run-hook-* function had the the
proper semantics. And I didn't want to write the logic for the 't'
and everything. But now I notice run-hook-wrapped should do a nice job, right?

>> +(add-hook 'emacs-lisp-mode-hook
>> +          'flymake-elisp-setup-backends)
> You should change elisp-mode.el directly instead.

OK.

> Actually I'd argue that the contents of flymake-elisp.el should move
> to elisp-mode.el.

OK.

> 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. But isn't having flymake-make-diagnostic and the hook already
"knowing" about flymake)? Isn't (eval-when-compile (require
'flymake-ui)) better?  But maybe I don't read you correctly and I'm just
confused.

>> +(defcustom flymake-proc-allowed-file-name-masks
>> +  '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'"
>> +     flymake-proc-simple-make-init
>> +     nil
>> +     flymake-proc-real-file-name-considering-includes)
>> +    ("\\.xml\\'" flymake-proc-xml-init)
>> +    ("\\.html?\\'" flymake-proc-xml-init)
>> +    ("\\.cs\\'" flymake-proc-simple-make-init)
>> +    ("\\.p[ml]\\'" flymake-proc-perl-init)
>> +    ("\\.php[345]?\\'" flymake-proc-php-init)
>> +    ("\\.h\\'" flymake-proc-master-make-header-init 
>> flymake-proc-master-cleanup)
>> +    ("\\.java\\'" flymake-proc-simple-make-java-init 
>> flymake-proc-simple-java-cleanup)
>> +    ("[0-9]+\\.tex\\'" flymake-proc-master-tex-init 
>> flymake-proc-master-cleanup)
>> +    ("\\.tex\\'" flymake-proc-simple-tex-init)
>> +    ("\\.idl\\'" flymake-proc-simple-make-init)
>>      ;; ("\\.cpp\\'" 1)
>>      ;; ("\\.java\\'" 3)
>>      ;; ("\\.h\\'" 2 ("\\.cpp\\'" "\\.c\\'")
>
> 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.

>> +(progn
>> +  (define-obsolete-variable-alias
> [...]
>
> There are too many obsolete aliases in there, IMO.
> We should think a bit more about each one of them.

Oh no :-)
>
> Many of them alias `flymake-foo` to `flymake-proc--bar`, which doesn't
> make much sense: if `flymake-proc--bar` is used outside of flymake-proc
> (via some old name), then it probably shouldn't have "--" in its name.

Makes a lot of sense, I removed them.

When I started I too obsessed with compatibility so you might have
guessed I started by making one alias for every symbol indiscriminately,
marked it internal for good measure. Then I hand picked some that looked
like they could be used outside.

Now I consider flymake-easy.el working minimally to be sufficient backward
compatibility.

> 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.

> 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.
And fixed: the "master" mechanism (good old "master" naming eh), for
example, is broken. It cannot understand that C++ .tpp files are
included from .hpp included from .cpp.

Don't get me wrong, legacy flymake has been immensely useful for me, but
again I think it should stay put until we have a better alternative. I
think we could handle a bit of duplication here.

> 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.

>> +  (define-obsolete-variable-alias 'flymake-xml-program
>> +    'flymake-proc-xml-program "26.1"
>> +    "Program to use for XML validation.")
>
> In the long run, this should likely move to some xml major mode, so
> `flymake-proc-xml-program` is probably not the "right" name for it.
> So I think we may as well keep using `flymake-xml-program` for now,
> until it's moved to some other package.

No, let's write a good backend for xml-mode/sgml-modes instead. There
are probably super cool xml linters out there, some perhaps in elisp
(nxml-mode of James Clark fame for one)

>> +  :group 'flymake
>
> Inside flymake(-ui).el, those `:group 'flymake` are redundant.

A nitpick indeed :-). Fixed.

>> +(defun flymake-make-diagnostic (buffer
>> +                                beg
>> +                                end
>> +                                type
>> +                                text)
>> +  "Mark BUFFER's region from BEG to END with a flymake diagnostic.
>
> AFAIK this function does not really "mark".  It just creates

It used to. Fixed the docstring.

>
>> +TYPE is a key to `flymake-diagnostic-types-alist' and TEXT is a
>> +description of the problem detected in this region."
>> +  (flymake--diag-make :buffer buffer :beg beg :end end :type type :text 
>> text))
>
> You could get almost the same function (and completely skip the
> definition of flymake--diag-make) with:
>
>          (:constructor flymake-make-diagnostic (buffer beg end type
>          text))
> Maybe we should extend cl-defstruct so we can give it docstrings
> for :constructors, since I suspect that the desire to provide a proper
> docstring was the main motivation for not using a :constructor here.

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. 

>> +  (cl-remove-if-not
>> +   (lambda (ov)
>> +     (and (overlay-get ov 'flymake-overlay)
>
> `ov` is by definition an overlay, so using property name
> `flymake-overlay` sounds redundant: just `flymake` would do just fine.
> (actually it's a bit worse: the property name `flymake-overlay` makes it
> sound like its value will be an overlay, whereas it's just t).

Not just "a bit worse", Stefan: a truly horrible crime. Fixed.

> You probably should `cl-remove-if-not` before doing to `cl-sort`.
> Both for performance reasons (fewer overlays to sort) and so that your
> `compare` and `key` functions don't have to deal with
> non-flymake overlays.

Well spotted. Fixed.

> The docstring should say if it's OK for the backend to call REPORT-FN
> several times with such diagnostics (so it can start a background
> process and call the REPORT-FN every time it gets a chunk of reports
> from the background process), or whether it should instead wait until it
> has all the diagnostics before calling REPORT-FN.

Indeed. I think it should be OK to do so, but currently it
isn't. Something to think about. Since each REPORT-FN is a different
lambda we could use that fact to track if a calling it "too much".

>> +* 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 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") 

> Maybe just say that the third option is to pass to REPORT-FN a value of
> the form `(:panic . EXPLANATION)` where EXPLANATION is a string.
> [ Note: I don't like using &key for functions, so I recommend not to
>   use it for API interfaces such as flymake-diagnostic-functions,
>   although I don't object to using them internally in flymake if you so
>   prefer.  ]

The first argument to REPORT-FN can be a list or a non-nil symbol. In
the latter case keywords are accepted. Actually :explanation is always
accepted, as is :force. I just forgot to describe it. Probably other
keywords will come in handy. Keywords are good, they're free
destructuring, better than pcase'ing funky cons (much as I respect pcase
:-).

> 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. 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.

>> +(defvar flymake-diagnostic-types-alist
>> +  `((:error
>> +     . ((category . flymake-error)
>> +        (mode-line-face . compilation-error)))
>> +    (:warning
>> +     . ((category . flymake-warning)
>> +        (mode-line-face . compilation-warning)))
>> +    (:note
>> +     . ((category . flymake-note)
>> +        (mode-line-face . compilation-info))))
>
> This mapping from diag-type to properties seems redundant with the one
> from category symbol to properties.  I'm no friend of the `category`
> property, so I'd recommend you use
>
>     (defvar flymake-diagnostic-types-alist
>       `((:error (face . flymake-error)
>                 (bitmap . flymake-error-bitmap)
>                 (severity . ,(warning-numeric-level :error))
>                 (mode-line-face . compilation-error))
>          ...))

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

> but if you prefer using `category`, you can just use
> (intern (format "flymake-category-%s" diag-type)) and then put all the
> properties on those symbols.

OK. Will do.

>> +        (dolist (backend flymake-diagnostic-functions)
>
> In order to properly handle flymake-diagnostic-functions as a hook
> (e.g. handle the t case), you probably want to use:
>
>     (run-hook-wrapped 'flymake-diagnostic-functions
>       (lambda (backend)
>         (cond ((memq backend flymake--running-backends)
>                (flymake-log 2 "Backend %s still running, not restarting"
>                             backend))
>               ((memq backend flymake--disabled-backends)
>                (flymake-log 2 "Backend %s is disabled, not starting"
>                             backend))
>               (t
>                (flymake--run-backend backend)))
>         nil))

That's it (I had seen run-hook-wrapped in the meantime). Thanks!

>>  (defun flymake-mode-on ()
>>    "Turn flymake mode on."
>>    (flymake-mode 1)
>> -  (flymake-log 1 "flymake mode turned ON for buffer %s" (buffer-name)))
>> +  (flymake-log 1 "flymake mode turned ON"))
>
> We should make this an obsolete alias of `flymake-mode`.

OK.

>> -;;;###autoload
>>  (defun flymake-mode-off ()
>>    "Turn flymake mode off."
>>    (flymake-mode 0)
>> -  (flymake-log 1 "flymake mode turned OFF for buffer %s" (buffer-name)))
>> +  (flymake-log 1 "flymake mode turned OFF"))
>
> I'd make this obsolete (for those rare cases where it's needed, you can
> just as well call (flymake-mode -1)).

OK.

>> +        (reported (hash-table-keys flymake--diagnostics-table)))
>
> AFAICT you only use `reported` as a boolean, so it would be more
> efficient to use
>
>            (reported (> (hash-table-count flymake--diagnostics-table) 0))
>
> [ I noticed this because of `hash-table-keys`: almost every time it's
>   used, it's an inefficient way to solve the problem, in my
>   experience ;-)  ]

Actually, I was thinking of making cl-set-difference assertions between
that and other sets in the future. hash-table-keys seems a nice way.

>> +;;; Dummy autoloads ensure that this file gets autoloaded, not just
>> +;;; flymake-ui.el where they actually live.
>>
>> +;;;###autoload
>> +(define-minor-mode flymake-mode nil)
>
> Yuck!

Yes, kinda horrible. I was in a hurry.

>> +(require 'flymake-ui)
>> +(require 'flymake-proc)
>> +(require 'flymake-elisp)
>
> `flymake-elisp.el` should not be loaded as soon as you enable
> flymake-mode, since we may never use Elisp in that session: the
> dependency goes in the wrong direction.

OK.

> `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?

> I think the best solution is to make it so flymake.el doesn't (require
> 'flymake-proc).  It will require moving some stuff from flymake-proc.el
> to flymake.el (e.g. all the backward compatibility APIs), but I think
> the result may be even cleaner.

I think that's my idea from the previous paragraph too. OK.

To summarize:

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

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.

Will fix, but harder:

* proper checkdoc.el interface
* proper byte-compile.el interface
* Move flymake-elisp.el contents to elisp-mode.el
* Allow REPORT-FN to be called multiple times from backends
* Passing recent changes to backends
* Rename flymake-ui.el to flymake.el and make some autoloads.

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.

Don't like:
* removing category in flymake-diagnostic-types-alist
* mark flymake-proc-err-line-patterns obsolete and add to
  some other variable from across emacs progmodes.
* remove the -proc prefix from defs in the proc backend.
* reconsider parts from flymake-proc into an util package.

No effect:

* Copyright FSF in the header

Thanks,
João