Flymake refactored

classic Classic list List threaded Threaded
79 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Flymake refactored

João Távora
Hi all,

I think my Flymake refactoring/rewriting effort is nearing
completion. Obviously people to take a look at it before it is
merged to master.

Here are the main changes:

* Flymake now annotates arbitrary buffer regions, not just lines.

* Flymake supports arbitrary diagnostic types, not just errors and
  warnings. See variable flymake-diagnostic-types-alist.

* There is a clean separation between the UI showing the diagnostics
  and the sources of these diagnostics, called backends.

* Flymake supports multiple simultaneous backends, meaning that you can
  check your buffer from different perspectives.

* The "legacy" regexp-based backend still works nicely in my (limited)
  testing. It is enabled by default but lives in a separate file
  lisp/progmodes/flymake-proc.el

* Two backends for Emacs-lisp are provided in
  lisp/progmodes/flymake-elisp.el. Enabling flymake-mode in an elisp
  buffer should activate them.

* Backends are just functions kept in a buffer-local variable. See
  flymake-diagnostic-functions.

* Some colorful fanciness in the mode-line. Still far from flycheck's
  (which is very good) but should be easy to get there.

* In a separate scrap-able commit I bind M-n and M-p in flymake-mode to
  navigate errors. I like it and the keys were unused, but it's probably
  against the rules.

* I started rewriting the manual, but won't go much further until the
  implementation is stabilized.

Regarding backward compatibility:

* I have provided obsolete aliases for most variables and functions,
  at least the ones I though were "public". Probably too many, but
  possibly missed one.

* Steve Purcell's flymake-easy.el adaptor
  (https://github.com/purcell/flymake-easy) seems to work, which is a
  nice surprise. It hooks onto the legacy backend via the obsolete
  aliases.

* Got rid of flymake-display-err-menu-for-current-line, since it wasn't
  doing anything useful anyway.

The code is in the scratch/flymake-refactor branch on Savannah. There
are some 40 proper Changelog-style commits with profuse comments
explaining the changes. You can follow that story or review the final
patch, but keep M-x vc-region-history handy.

Here are a couple of screenshots taken from clean emacs -Q runs after
just M-x flymake-mode.

flymake-1.png (57K) Download Attachment
flymake-2.png (58K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Stefan Monnier
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


Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Dmitry Gutov
In reply to this post by Stefan Monnier
On 9/28/17 9:52 PM, Stefan Monnier wrote:

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

I think it would be very silly to use next-error for grep, but not for
flymake. Flycheck uses it, so flymake can make do somehow, too.

It should also encourage us to sort out that mess.

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Ted Zlatanov
On Fri, 29 Sep 2017 14:51:59 +0200 Dmitry Gutov <[hidden email]> wrote:

DG> On 9/28/17 9:52 PM, Stefan Monnier wrote:
>> 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...).

DG> I think it would be very silly to use next-error for grep, but not for flymake.
DG> Flycheck uses it, so flymake can make do somehow, too.

DG> It should also encourage us to sort out that mess.

Right now the logic is based on buffer visibility, I think. next-error
supposed to be DWIM so the complexity is on the software side (and
messy) to keep the user experience simple. For me, it Just Works
currently (I use Occur and Flycheck a lot, compile and grep/git grep
less) so I've been pretty happy with it. I'm sure it can be improved :)

Ted


Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Dmitry Gutov
On 9/29/17 4:55 PM, Ted Zlatanov wrote:

> Right now the logic is based on buffer visibility, I think. next-error
> supposed to be DWIM so the complexity is on the software side (and
> messy) to keep the user experience simple. For me, it Just Works
> currently (I use Occur and Flycheck a lot, compile and grep/git grep
> less) so I've been pretty happy with it. I'm sure it can be improved :)

See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489 for a recent
discussion.

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Ted Zlatanov
On Fri, 29 Sep 2017 17:03:52 +0200 Dmitry Gutov <[hidden email]> wrote:

DG> On 9/29/17 4:55 PM, Ted Zlatanov wrote:
>> Right now the logic is based on buffer visibility, I think. next-error
>> supposed to be DWIM so the complexity is on the software side (and
>> messy) to keep the user experience simple. For me, it Just Works
>> currently (I use Occur and Flycheck a lot, compile and grep/git grep
>> less) so I've been pretty happy with it. I'm sure it can be improved :)

DG> See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489 for a recent discussion.

I was specifically talking about how Flymake should integrate with
next-error. Sorry I didn't make that clearer.

I'm not sure if you want to follow up on that bug or here in a new
thread, but if you want to make a branch with the changes you think
would improve the next-error DWIM experience, I'll be glad to take a
look and test it with my workflows. That would probably be the most
effective way.

Ted


Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Dmitry Gutov
On 9/29/17 6:26 PM, Ted Zlatanov wrote:

> I was specifically talking about how Flymake should integrate with
> next-error. Sorry I didn't make that clearer.

It's still not clear to me, then. Should Flymake integration be "messy"?

> I'm not sure if you want to follow up on that bug or here in a new
> thread, but if you want to make a branch with the changes you think
> would improve the next-error DWIM experience, I'll be glad to take a
> look and test it with my workflows. That would probably be the most
> effective way.

Hopefully, someday. Right now, I see how it shouldn't work, but I don't
exactly see how it should.

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Ted Zlatanov
On Fri, 29 Sep 2017 19:35:38 +0200 Dmitry Gutov <[hidden email]> wrote:

DG> On 9/29/17 6:26 PM, Ted Zlatanov wrote:
>> I was specifically talking about how Flymake should integrate with
>> next-error. Sorry I didn't make that clearer.

DG> It's still not clear to me, then. Should Flymake integration be "messy"?

I'd go for "unsurprising" :)

>> I'm not sure if you want to follow up on that bug or here in a new
>> thread, but if you want to make a branch with the changes you think
>> would improve the next-error DWIM experience, I'll be glad to take a
>> look and test it with my workflows. That would probably be the most
>> effective way.

DG> Hopefully, someday. Right now, I see how it shouldn't work, but I don't exactly
DG> see how it should.

It might be productive to gather use cases and workflows. Those were
missing in the bug discussion, and will probably make it clearer.

For instance, my typical Flycheck interaction while writing CFEngine
code is to keep 1-2 cfengine-mode buffers open (next-error just
navigates between syntax errors in the visible buffer) and to sometimes
pop up an Occur buffer (in which case I want to navigate ocurrences for
as long as the Occur buffer is visible). I generally don't compile,
grep, or git grep in that workflow.

HTH
Ted


Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Marcin Borkowski-3
In reply to this post by João Távora
Hi,

I didn't (yet) look at it, but thanks for working on this.  I use
Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
whether to switch to flymake at some point in time.  Just being curious:
does Flycheck support running more linters on the same file at the same
time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
would one switch to Flymake for JS?

(Note: I understand that your time is limited, João, so if answer to any
of these questions requires more than 30 seconds, just tell me to rtfm,
which I'll be happy to do once I'm at home.)

Best,

--
Marcin Borkowski

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Dmitry Gutov
In reply to this post by Ted Zlatanov
On 9/29/17 7:56 PM, Ted Zlatanov wrote:

> DG> It's still not clear to me, then. Should Flymake integration be "messy"?
>
> I'd go for "unsurprising" :)

Then the concerns about M-x next-error should still apply.

> It might be productive to gather use cases and workflows. Those were
> missing in the bug discussion, and will probably make it clearer.
>
> For instance, my typical Flycheck interaction while writing CFEngine
> code is to keep 1-2 cfengine-mode buffers open (next-error just
> navigates between syntax errors in the visible buffer) and to sometimes
> pop up an Occur buffer (in which case I want to navigate ocurrences for
> as long as the Occur buffer is visible). I generally don't compile,
> grep, or git grep in that workflow.

I wouldn't say I have a specific workflow, but I do have a habit of
killing or quitting windows or otherwise hiding buffers which I'm not
looking at directly.

But there are some scenarios mentioned in the bug discussion, e.g. in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#74, earlier in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#44, and earlier
again in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#32.

Please send any further replies to this to the bug address.

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

João Távora
In reply to this post by Marcin Borkowski-3
Marcin Borkowski <[hidden email]> writes:

> I didn't (yet) look at it, but thanks for working on this.  I use
> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
> whether to switch to flymake at some point in time.  Just being curious:
> does Flycheck support running more linters on the same file at the same

I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
then the answer is yes.

> time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
> would one switch to Flymake for JS?

One possibility would be: wait that it is (hopefully) present in the
next official emacs release, Emacs 26.1 and then wait that someone
writes a decent JS backend (what you call "linter" and Flycheck calls
"checkers") for it.

Another possibility would be: check out the most recent code and write
the backend yourself :-)

> (Note: I understand that your time is limited, João, so if answer to any
> of these questions requires more than 30 seconds, just tell me to rtfm,
> which I'll be happy to do once I'm at home.)

:-)

It took about 60 seconds but I'm not going to charge for the extra 30.

Also there is yet no m to rtf. Working on it.

João

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Marcin Borkowski-3

On 2017-10-01, at 01:43, João Távora <[hidden email]> wrote:

> Marcin Borkowski <[hidden email]> writes:
>
>> I didn't (yet) look at it, but thanks for working on this.  I use
>> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
>> whether to switch to flymake at some point in time.  Just being curious:
>> does Flycheck support running more linters on the same file at the same
>
> I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
> then the answer is yes.

I know that.  I want to know whether Flycheck can do that, too.  If not,
then we have a strong advantage of Flymake.

>> time?  (From a cursory glance at the docs, I'm not sure.)  Also, how
>> would one switch to Flymake for JS?
>
> One possibility would be: wait that it is (hopefully) present in the
> next official emacs release, Emacs 26.1 and then wait that someone
> writes a decent JS backend (what you call "linter" and Flycheck calls
> "checkers") for it.

By "linter" I mean the outside program doing the checking (like
ESlint).  By "checker", Flymake/Flycheck mean the Elisp interface
between the linter and fly.*.

> Another possibility would be: check out the most recent code and write
> the backend yourself :-)

That's a cool idea.  I am quite tempted to do that.  As usual, time is
the main problem.

>> (Note: I understand that your time is limited, João, so if answer to any
>> of these questions requires more than 30 seconds, just tell me to rtfm,
>> which I'll be happy to do once I'm at home.)
>
> :-)
>
> It took about 60 seconds but I'm not going to charge for the extra 30.

Oh, thanks!

> Also there is yet no m to rtf. Working on it.

I'd be happy to proofread it when the time comes.

Best,

--
Marcin Borkowski

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Mark Oteiza
Marcin Borkowski <[hidden email]> writes:

> On 2017-10-01, at 01:43, João Távora <[hidden email]> wrote:
>
>> Marcin Borkowski <[hidden email]> writes:
>>
>>> I didn't (yet) look at it, but thanks for working on this.  I use
>>> Flycheck currently (maily for JavaScript/ESlint), but I'm wondering
>>> whether to switch to flymake at some point in time.  Just being curious:
>>> does Flycheck support running more linters on the same file at the same
>>
>> I don't know (did you mean Flycheck or Flymake?). If you meant Flymake,
>> then the answer is yes.
>
> I know that.  I want to know whether Flycheck can do that, too.  If not,
> then we have a strong advantage of Flymake.

It can, but only does so if a particular checker specifies it with the
:next-checkers keyword.  For instance, the 'emacs-lisp' checker
indicates that the 'emacs-lisp-checkdoc' checker be run next.

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

João Távora
In reply to this post by Stefan Monnier

[In the meantime I see you've been checking out emacs-diffs. Just a
heads up that deleted the scratch/flymake-refactor and repushed it again
with some different commits (but most of original tree is still
identical hash-wise)

Sorry for the git no-no. I did nothing special, just a bit obsessed with
clean Git history I guess]

So here's my reply to your email before that.

Stefan Monnier <[hidden email]> writes:

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

Sorry, I mistook advice-add for add-function. I meant to say that
cl-letf is better than advice-add for dynamic localized overrides, but
you're right, it's not, as it binds the symbol globally. I fixed this.

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

Of course. The probability is small, but the damage or confusion might
be large, like elisp-byte-compiling an c-buffer. I made these functions
error, as good practice.

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

I see, because elisp-mode.el is preloaded (I was missing that bit).

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

Right, I agree. Perhaps I use autoloads sparingly (vs require) because
in third-party extensions you can't generally count on autoloads across
Emacs versions.

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

I can only think of porting flymake-proc-stop-all-syntax-checks, but:

* That is hard to do generically (requires more API)
* I don't see why it's particularly useful

I'd rather fix bugs in flymake-proc.el like the one that leaves those
@#$% *_flymake.c files behind (I think this happens because the cleanup
functions are local to a buffer that is outlived by the process, BTW)

>>>> +    'flymake-proc-compilation-prevents-syntax-check "26.1"
> IIUC you're saying it's just not useful enough to keep it in
> flymake.el?

Yes, what are we really preventing? I can only think of protecting
against a Makefile being confused by, say, files of _flymake.c suffix,
in the make directory. But this is a flymake-proc.el specific
problem. (but this one is simpler to port anyway).

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

I see. Let's leave it for later.

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

It does now, but before that only one call to REPORT-FN are allowed. See
the docstring again and the commit where I significantly redid the
API. Multiple calls to REPORT-FN are now allowed.

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

Hopefully I did.  But it probably needs your nitpicking.

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

Really? That's disappointing... Even with the new version of
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?

See the new docstring of flymake-diagnostic-types-alist:

  `flymake-category', a symbol whose property list is considered
  as a default for missing values of any other properties.  This
  is useful to backend authors when creating new diagnostic types
  that differ from an existing type by only a few properties.

So if elisp-flymake-checkdoc needs to make something very similar to
notes but with no text face (just the fringe bitmap), it can do

   ...
   (flymake-make-diagnostic
                     (current-buffer)
                     start end :checkdoc-note text)
   ...

And then

   (add-to-list 'flymake-diagnostic-types-alist
                '(:checkdoc-note . ((flymake-category . flymake-note)
                                    (face . nil))))

And then the user can override it.

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

Actually, I just (require 'flymake-proc) *after* (provide 'flymake) in
flymake.el. It looks pretty clean to me, does you see any drawbacks?

>> * Move flymake-compilation-prevents-syntax-check to flymake-ui.el

Not done, not convinced of the usefulness yet.

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

Done

>> * Don't use hash-table-keys, unless I really need it.

Actually, I think I need hash-table-keys.
>
>> Will fix, but harder:
>> * proper checkdoc.el interface
>> * proper byte-compile.el interface
>> * Move flymake-elisp.el contents to elisp-mode.el

No need, I think. I did these too.
>
>> * Allow REPORT-FN to be called multiple times from backends

No, API changes big as this one are important to get right sooner rather
than later. In the mantime, I noticed that Flycheck's API is very
similar this one (though not exactly the same).
>
>> * Passing recent changes to backends
>
> Maybe we should keep that for later, to avoid overengineering the API.

But, this one I agree to save for later. Probably binding a dynamic
variable. (but we could also pass kwargs to the backends)

>> * Rename flymake-ui.el to flymake.el and make some autoloads.
>
> That'd be good.

Did that.

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

Good, because I kept them

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

Didn't do this too. If we mark it obsolete, what's the "use instead"
message?

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

See if you like it now.

>> * reconsider parts from flymake-proc into an util package.
>
> If that makes sense, yes.

Nothing strikes me as really essential in this regard, perhaps later.

João

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Stefan Monnier
> [In the meantime I see you've been checking out emacs-diffs. Just a
> heads up that deleted the scratch/flymake-refactor and repushed it again
> with some different commits (but most of original tree is still
> identical hash-wise)
> Sorry for the git no-no. I did nothing special, just a bit obsessed with
> clean Git history I guess]

That's OK.  We want to allow this in scratch branches.

> Sorry, I mistook advice-add for add-function. I meant to say that
> cl-letf is better than advice-add for dynamic localized overrides, but
> you're right, it's not, as it binds the symbol globally. I fixed this.

I saw your recent introduction of proper hooks for checkdoc
and bytecomp.  I intend to take a closer look at them, but haven't had
time for it yet.

>> 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.
> I can only think of porting flymake-proc-stop-all-syntax-checks, but:
> * That is hard to do generically (requires more API)
> * I don't see why it's particularly useful
> I'd rather fix bugs in flymake-proc.el like the one that leaves those
> @#$% *_flymake.c files behind (I think this happens because the cleanup
> functions are local to a buffer that is outlived by the process, BTW)

"Hard to do generically + not particularly useful" qualifies as
"with regret" I think ;-)

>> 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.
> Really? That's disappointing... Even with the new version of
> flymake-diagnostic-functions?

I don't actually know.  But since nxml's checker currently doesn't go
though flymake.el its design was special tailored to take advantage of
the info available in Emacs (e.g. knowledge of what was changed, to
avoid repeating previous checks, knowledge of what's displayed, so as
to do all the checks "instantly" yet lazily, ...).  I can't remember
enough of the details to be sure, but my gut feeling tells me that it'll
be hard to preserve the desirable properties while interfacing
through flymake (since it's targeted at a very different use case).

> Actually, I just (require 'flymake-proc) *after* (provide 'flymake) in
> flymake.el. It looks pretty clean to me, does you see any drawbacks?

Yes, I saw that.  It's indeed simple, and should work fine especially
for a file which we plan on marking obsolete.

>> 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.
> Didn't do this too.  If we mark it obsolete, what's the "use instead"
> message?

Don't know.  flymake-diagnostic-functions?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

João Távora
Stefan Monnier <[hidden email]> writes:

> That's OK.  We want to allow this in scratch branches.

Yeah, but boy does it spam emacs-diffs. Can we enable push -f in just
the scratch branches. I don't know if its possible but Google says it's
a pretty commonly requested feature.

> "Hard to do generically + not particularly useful" qualifies as
> "with regret" I think ;-)

Au contraire, with delight, it's not everyday that a hard thing to do is
useless :-)

> enough of the details to be sure, but my gut feeling tells me that it'll
> be hard to preserve the desirable properties while interfacing
> through flymake (since it's targeted at a very different use case).

I don't know (I also haven't looked at the code) but none of those
things sound like showstoppers.  For a start, I'd be happy just
preserving the fact that it isn't based on regexps.  The only thing a
backend has to do is tell flymake where some problems exist. Even a naive
solution like running nmxl in a subprocess emacs and prin1'int a list of
collected errors, like we do for elisp-flymake-byte-compile now,
shouldn't be horrible.

>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>> message?
> Don't know.  flymake-diagnostic-functions?

Yeah, but right now that's saying "this bit is obsolete, go write a
replacement and then use that instead. good luck "

Regarding the merge to emacs-26, do you see anything else we need to
iron out before it?

João


Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

Stefan Monnier
>> enough of the details to be sure, but my gut feeling tells me that it'll
>> be hard to preserve the desirable properties while interfacing
>> through flymake (since it's targeted at a very different use case).

> I don't know (I also haven't looked at the code) but none of those
> things sound like showstoppers.  For a start, I'd be happy just
> preserving the fact that it isn't based on regexps.  The only thing a
> backend has to do is tell flymake where some problems exist. Even a naive
> solution like running nmxl in a subprocess emacs and prin1'int a list of
> collected errors, like we do for elisp-flymake-byte-compile now,
> shouldn't be horrible.

Oh, I don't forsee any major difficulty in writing an nxml backend for
flymake, *IF* we limit ourselves to the goal of making it work.  But if
we want it to work about as well as nxml-mode itself, it'll be harder.

>>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>>> message?
>> Don't know.  flymake-diagnostic-functions?
> Yeah, but right now that's saying "this bit is obsolete, go write a
> replacement and then use that instead. good luck "

I thought you were the one saying that flymake-proc is all legacy and
will be replaced.  I don't think anyone has claimed that flymake-proc is
*currently* obsolete, just that the plan is to retire it at some point
(this point being presumably after a replacement is written).

> Regarding the merge to emacs-26, do you see anything else we need to
> iron out before it?

Maybe just this issue of letting backends tell flymake.el whey they're
done (and letting flymake tell backends to abort the currently running
check)?


        Stefan

Reply | Threaded
Open this post in threaded view
|

Re: Flymake refactored

João Távora
Stefan Monnier <[hidden email]> writes:

> Oh, I don't forsee any major difficulty in writing an nxml backend for
> flymake, *IF* we limit ourselves to the goal of making it work.  But if
> we want it to work about as well as nxml-mode itself, it'll be harder.

Maybe you're right for the bit where (from what I gather from your
comments) nxml-mode concentrates on the visible part of the buffer. Not
unrelated, that's where nxml-mode and Flymake differ in their
promise. Flymake's is to check the whole buffer, letting you know in the
mode-line how many errors you have, and quickly letting you navigate
between them. I think that, by design, that part could never be "as well
as nxml-mode". On the other hand, perhaps it would be useful to ask
backends to concentrate on this region first, then elsewhere.

>>>> Didn't do this too.  If we mark it obsolete, what's the "use instead"
>>>> message?
>>> Don't know.  flymake-diagnostic-functions?
>> Yeah, but right now that's saying "this bit is obsolete, go write a
>> replacement and then use that instead. good luck "
>
> I thought you were the one saying that flymake-proc is all legacy and
> will be replaced.  I don't think anyone has claimed that flymake-proc is
> *currently* obsolete, just that the plan is to retire it at some point
> (this point being presumably after a replacement is written).

OK then. I guess I misunderstand the graveness of a make-obsolete.

>> Regarding the merge to emacs-26, do you see anything else we need to
>> iron out before it?
>
> Maybe just this issue of letting backends tell flymake.el whey they're
> done (and letting flymake tell backends to abort the currently running
> check)?

Done, and rebuilt a prettier history in the branch
scratch/flymake-refactor-cleaner-for-emacs-26.

Will still fix some more longstanding bugs and then merge to
emacs-26. Bugs and finishing touches can still be fixed there.

João

1234