Re: Flymake refactored

[Stefan Monnier <monnier@iro.umontreal.ca>]

Hi João,

This looks really good, thank you very much.  I think this kind of
functionality is very important and we should strive to polish it such
that it can be enabled by default globally, via a global-flymake-mode
(although I'm quite aware that we're not there yet and I don't think we
need to get there before your changes are merged).

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

Note: Just because some are nitpicks about the specific working of
a specific chunk of code doesn't mean that I've read all the code
carefully (it's *really* not the case).  More to the point, I'm sure
I missed a lot of things.

> +line, respectively. When @code{flymake-mode} is active, they are
> +mapped to @kbd{M-n} and @kbd{M-p}, respectively, and by default.

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
with it, but I'm not even sure I'd like it for flymake-mode because it's
a mode that will be "always" enabled, contrary to smerge-mode which
I only enable while there are conflicts to resolve).

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

> +;; Copyright (C) 2017  João Távora

Of course, this should say FSF instead.

> +(defun flymake-elisp--checkdoc-1 ()
> +  "Do actual work for `flymake-elisp-checkdoc'."
> +  (let (collected)
> +    (cl-letf (((symbol-function 'checkdoc-create-error)
> +               (lambda (text start end &optional unfixable)
> +                 (push (list text start end unfixable) collected)
> +                 nil)))

We should change checkdoc.el directly to avoid such hackish surgery.

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

> +(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)))

Similarly, we should change directly byte-compile-log-warning such that
this kind of surgery is not needed.

> +  (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)

> +(add-hook 'emacs-lisp-mode-hook
> +          'flymake-elisp-setup-backends)

You should change elisp-mode.el directly instead.
Actually I'd argue that the contents of flymake-elisp.el should move
to elisp-mode.el.  This also means remove the (require 'flymake-ui)
and instead mark `flymake-make-diagnostic` as autoloaded.

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

> +(add-to-list 'flymake-diagnostic-functions
> +             'flymake-proc-legacy-flymake)

Should also be

    (add-hook 'flymake-diagnostic-functions #'flymake-proc-legacy-flymake)

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

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.

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

Also some parts of flymake-proc seem to make sense for "all" backends
rather than only for flymake-proc.

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

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

> +  (define-obsolete-variable-alias 'flymake-allowed-file-name-masks
> +    'flymake-proc-allowed-file-name-masks "26.1"

Since I argued above that flymake-proc-allowed-file-name-masks should
be obsolete, there's not much point renaming
flymake-allowed-file-name-masks to flymake-proc-allowed-file-name-masks.

> +  :group 'flymake

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

> +(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
a diagnostic object which will/may cause the corresponding region to be
marked at some later time.

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

> +  (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).

> +          (or (not filter)
> +              (cond ((functionp filter) (funcall filter ov))
> +                    ((symbolp filter) (overlay-get ov filter))))))
> +   (save-restriction
> +     (widen)
> +     (let ((ovs (if (and beg (null end))
> +                    (overlays-at beg t)
> +                  (overlays-in (or beg (point-min))
> +                               (or end (point-max))))))
> +       (if compare
> +           (cl-sort ovs compare :key (or key
> +                                         #'identity))
> +         ovs)))))

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.

> +* A (possibly empty) list of objects created with
> +  `flymake-make-diagnostic', causing flymake to annotate the
> +  buffer with this information and consider the backend has
> +  having finished its check normally.

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.

> +* 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?

> +* The symbol `:panic', signalling that the backend has
> +  encountered an exceptional situation and should be disabled.
> +
> +In the latter cases, it is also possible to provide REPORT-FN
> +with a string as the keyword argument `:explanation'. The string
> +should give human-readable details of the situation.")

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

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

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.

> +(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))
         ...))

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.

> +        (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))

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

> -;;;###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)).

> +        (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 ;-)  ]

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

Yuck!

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

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

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.


        Stefan