Refactoring flymake.el

Refactoring flymake.el

[João Távora]

Hello all,

I've been away from this list and away from hacking for a while now, but
I thought I'd return with a modest proposal for refactoring flymake.el
and making it more useful to users and also third-party extensions.

Specifically, I'm thinking of separating its UI and from its
diagnostics-generating backends. Currently the latter rely purely on
lanching external processes and examining their output, but recent
experimentations with the LSP (Language Server Protocol) showed that to
not always be the case. Another example is Elisp itself, which is
syntax-checkable without an external tool.

If you are thinking: "Hasn't flycheck.el done all those things
already", the answer is probably yes, but flycheck has been around for
some time and isn't in Emacs, whereas flymake.el is. Anyway, that is a
whole different topic. I believe flymake.el's flaws can be fixed and it
can be made as good as, if not better than, flycheck.el.

I'm also committed to maintaining backward compatibility with the many
flymake.el configurations out in the wild.

I've already done some work, but before I invest much more, i'd
like to know if there's any interest in these efforts. 

So here's what I have so far. In a newly pushed git branch:

  scratch/flymake-refactor

there are just two commits right now:

  eb34f7f5a2 Split flymake.el into flymake-proc.el and flymake-ui.el
  13993c46a2 Add flymake-backends defcustom

The first commit makes flymake.el into just a stub that `require's
flymake-ui.el and flymake-proc.el. flymake-ui.el contains the flymake
UI: the minor-mode, functions dealing with overlays, generic error
containers, etc... flymake-proc.el has all the functions dealing with
external processes, Makefiles, temporary mocks, and all that junk. This
segregation was done very roughly, it may need some fine-tuning.

The second commit adds a very simple indirection variable, a
flymake-backends defcustom, which is currently only used by flymake-proc
to add its backend.

The only testing I've given it is interactive and the Emacs suite test
case.

Going forward I'm thinking of:

1. Segregating functions flymake-proc.el into their own namespace
generously establishing obsoletion aliases for user-visible functions.
2. Adding an experimental emacs-lisp backend based on bytecomp.el
3. ...
4. Checking in again to see if it there's still interest :-)


João

Re: Refactoring flymake.el

[John Wiegley]

>>>>> "JT" == João Távora <joaotavora@gmail.com> writes:

JT> 4. Checking in again to see if it there's still interest :-)

I'd be interested to see what you come up with.

I also use flycheck, but that doesn't mean there isn't room for another
attempt. In particular, flycheck started out somewhat averse to customization,
which it later eased up on. This means the core design did not consider this
much at the beginning: which may have led to design choices that flymake can
now improve upon.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Refactoring flymake.el

[Sam Steingold]

Hi João,

One thing about flymake that inconveniences me is that I cannot disable
it for some specific files.
I have a bunch of "scratch.py" files that I use as poor man's notebooks.
To disable flymake on them, I made this chanfe to flymake.el today:

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index ed34d9aaa5..edb87c92b3 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -133,6 +133,13 @@ flymake-master-file-count-limit
   :group 'flymake
   :type 'integer)
 
+(defcustom flymake-disallowed-file-name-regexp nil
+  "File names matching this regexp will not be checked.
+This overrides `flymake-allowed-file-name-masks'."
+  :group 'flymake
+  :type 'string
+  :version "26.1")
+
 (defcustom flymake-allowed-file-name-masks
   '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" flymake-simple-make-init)
     ("\\.xml\\'" flymake-xml-init)
@@ -228,14 +235,16 @@ flymake-get-file-name-mode-and-masks
   "Return the corresponding entry from `flymake-allowed-file-name-masks'."
   (unless (stringp file-name)
     (error "Invalid file-name"))
-  (let ((fnm flymake-allowed-file-name-masks)
-	(mode-and-masks nil))
-    (while (and (not mode-and-masks) fnm)
-      (if (string-match (car (car fnm)) file-name)
-	  (setq mode-and-masks (cdr (car fnm))))
-      (setq fnm (cdr fnm)))
-    (flymake-log 3 "file %s, init=%s" file-name (car mode-and-masks))
-    mode-and-masks))
+  (unless (and flymake-disallowed-file-name-regexp
+               (string-match flymake-disallowed-file-name-regexp file-name))
+    (let ((fnm flymake-allowed-file-name-masks)
+	  (mode-and-masks nil))
+      (while (and (not mode-and-masks) fnm)
+        (let ((item (pop fnm)))
+          (when (string-match (car item) file-name)
+	    (setq mode-and-masks (cdr item)))))
+      (flymake-log 3 "file %s, init=%s" file-name (car mode-and-masks))
+      mode-and-masks)))
 
 (defun flymake-can-syntax-check-file (file-name)
   "Determine whether we can syntax check FILE-NAME.
--8<---------------cut here---------------end--------------->8---

and set `flymake-disallowed-file-name-regexp` to "scratch".
it seems to be working, so I was thinking about pushing it.
Do you think there is a better approach?

Thanks.

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net
http://islamexposedonline.com http://iris.org.il http://no2bds.org
In the race between idiot-proof software and idiots, the idiots are winning.

Re: Refactoring flymake.el

[Stefan Monnier]

> One thing about flymake that inconveniences me is that I cannot disable
> it for some specific files.
> I have a bunch of "scratch.py" files that I use as poor man's notebooks.

Can you add some file-local settings in them?

> +(defcustom flymake-disallowed-file-name-regexp nil
> +  "File names matching this regexp will not be checked.
> +This overrides `flymake-allowed-file-name-masks'."
> +  :group 'flymake
> +  :type 'string
> +  :version "26.1")

Using file-local or dir-local variables settings is often more convenient.


        Stefan

Re: Refactoring flymake.el

[Dmitry Gutov]

Hey João,

On 8/17/17 5:40 PM, João Távora wrote:

> Specifically, I'm thinking of separating its UI and from its
> diagnostics-generating backends. Currently the latter rely purely on
> lanching external processes and examining their output, but recent
> experimentations with the LSP (Language Server Protocol) showed that to
> not always be the case. Another example is Elisp itself, which is
> syntax-checkable without an external tool.

*thumbs up*

> If you are thinking: "Hasn't flycheck.el done all those things
> already", the answer is probably yes, but flycheck has been around for
> some time and isn't in Emacs, whereas flymake.el is. Anyway, that is a
> whole different topic. I believe flymake.el's flaws can be fixed and it
> can be made as good as, if not better than, flycheck.el.
> 
> I'm also committed to maintaining backward compatibility with the many
> flymake.el configurations out in the wild.

That's great, but ultimately not so important, IMO. We can drop it in 
some future release, with the usual deprecation period.

> 4. Checking in again to see if it there's still interest :-)

Since Flycheck's author basically made sure we can't use it in Emacs or 
GNU ELPA, we do need an alternative.

Very happy someone is working on it, especially someone with your Elisp 
experience.

Re: Refactoring flymake.el

[Sam Steingold]

> * Stefan Monnier <zbaavre@veb.hzbagerny.pn> [2017-08-17 18:32:48 -0400]:
>
>> One thing about flymake that inconveniences me is that I cannot disable
>> it for some specific files.
>> I have a bunch of "scratch.py" files that I use as poor man's notebooks.
>
> Can you add some file-local settings in them?

elpy (https://github.com/jorgenschaefer/elpy) ignores them.
I just tried

--8<---------------cut here---------------start------------->8---
# Local Variables:
# flymake-mode: nil
# End:
--8<---------------cut here---------------end--------------->8---

and flymake is still enabled in scratch.
So -- no, file-local settings do not solve my problem.

>> +(defcustom flymake-disallowed-file-name-regexp nil
>> +  "File names matching this regexp will not be checked.
>> +This overrides `flymake-allowed-file-name-masks'."
>> +  :group 'flymake
>> +  :type 'string
>> +  :version "26.1")
>
> Using file-local or dir-local variables settings is often more convenient.

Nope, because I have to deal with the "safe local variable dialogue"
every time I visit such a file.
And again when I restart emacs and all those scratch files are reloaded
by desktop.

I understand the security implications, so I am reluctant to disable the
check, but this is a major inconvenience, so would use this approach
only as a last resort.

So, are you really _that_ opposed to `flymake-disallowed-file-name-regexp'?

Thanks.

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net http://honestreporting.com
http://thereligionofpeace.com http://islamexposedonline.com http://no2bds.org
Beauty is only a light switch away.

Re: Refactoring flymake.el

[Dmitry Gutov]

On 8/18/17 5:51 PM, Sam Steingold wrote:

>> Can you add some file-local settings in them?
> 
> elpy (https://github.com/jorgenschaefer/elpy) ignores them.
> I just tried

That's something your can discuss with Jorgen.

Maybe Elpy has a different variable that governs whether flymake-mode 
should be enabled in any or all of its buffers.

>> Using file-local or dir-local variables settings is often more convenient.
> 
> Nope, because I have to deal with the "safe local variable dialogue"
> every time I visit such a file.

Only if such variable is not safe. If it's a boolean, for instance, 
adding ":safe 'booleanp" to its defcustom should be easy.

Re: Refactoring flymake.el

[Sam Steingold]

> * Dmitry Gutov <qthgbi@lnaqrk.eh> [2017-08-18 18:11:13 +0300]:
>
> On 8/18/17 5:51 PM, Sam Steingold wrote:
>
>>> Can you add some file-local settings in them?
>>
>> elpy (https://github.com/jorgenschaefer/elpy) ignores them.
>> I just tried
>
> That's something your can discuss with Jorgen.

Alas, not an option.

> Maybe Elpy has a different variable that governs whether flymake-mode
> should be enabled in any or all of its buffers.

It does not.

>>> Using file-local or dir-local variables settings is often more convenient.
>>
>> Nope, because I have to deal with the "safe local variable dialogue"
>> every time I visit such a file.
>
> Only if such variable is not safe. If it's a boolean, for instance,
> adding ":safe 'booleanp" to its defcustom should be easy.

Of course. However, this variable is defined by a local
`define-minor-mode', not `defcustom'.

Thanks.

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net http://think-israel.org
http://camera.org http://www.memritv.org http://thereligionofpeace.com
The difference between theory and practice is that in theory there isn't any.

Re: Refactoring flymake.el

[Dmitry Gutov]

On 8/18/17 7:13 PM, Sam Steingold wrote:

>> That's something your can discuss with Jorgen.
> 
> Alas, not an option.

I mean by filing an issue. But you probably know better.

>> Only if such variable is not safe. If it's a boolean, for instance,
>> adding ":safe 'booleanp" to its defcustom should be easy.
> 
> Of course. However, this variable is defined by a local
> `define-minor-mode', not `defcustom'.

You can also mark it safe using `put' later. Like:

(put 'abbrev-mode 'safe-local-variable 'booleanp)

Re: Refactoring flymake.el

[Clément Pit-Claudel]

On 2017-08-18 18:13, Sam Steingold wrote:
> Of course. However, this variable is defined by a local
> `define-minor-mode', not `defcustom'.

I think thst shouldn't be an issue: for example, dired-x has this:

  (put 'dired-omit-mode 'safe-local-variable 'booleanp)

Same for abbrev-mode:

  (put 'abbrev-mode 'safe-local-variable 'booleanp)

Re: Refactoring flymake.el

[Sam Steingold]

> * Dmitry Gutov <qthgbi@lnaqrk.eh> [2017-08-18 19:25:16 +0300]:
>
>>> Only if such variable is not safe. If it's a boolean, for instance,
>>> adding ":safe 'booleanp" to its defcustom should be easy.
>>
>> Of course. However, this variable is defined by a local
>> `define-minor-mode', not `defcustom'.
>
> You can also mark it safe using `put' later. Like:
>
> (put 'abbrev-mode 'safe-local-variable 'booleanp)

Yes, but this _is_ a security issue.
flymake-mode actually _may_ run an OS command.
I think it would be safe to _disable_ the mode, e.g.,

(put 'flymake-mode 'safe-local-variable 'null)

Objections?

PS. Do people really hate the `flymake-disallowed-file-name-regexp' patch?
Why?

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net http://think-israel.org
http://americancensorship.org https://jihadwatch.org
Please express your antipathy in the suicidal form.

Re: Refactoring flymake.el

[Noam Postavsky]

On Fri, Aug 18, 2017 at 1:59 PM, Sam Steingold <sds@gnu.org> wrote:
>
> PS. Do people really hate the `flymake-disallowed-file-name-regexp' patch?
> Why?

I don't see that anybody "hates" it, but rather there are questions
about existing alternatives, because we want to avoid adding
yet-another-option that might turn out to be redundant.

Speaking of, it looks to me that (push (list "scratch[.]py\\'" nil)
flymake-allowed-file-name-masks) would give the effect you want?

Re: Refactoring flymake.el

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> That's great, but ultimately not so important, IMO. We can drop it in
> some future release, with the usual deprecation period.

Yes, I meant *some* backward compatibility. I was thinking of making a
big-list-o-define-obsoletes and eventually making it smaller. At the
beginning I would at least ensure that the flymake-easy.el
(github.com/purcell/flymake-easy) middleware keeps working, because that
seems to be used between flymake.el and third-party flymake-for-langx.el
libraries.

> Very happy someone is working on it

Thanks for the thumbs up, and I hope you can spare a little time later
on to review my efforts.

João

Re: Refactoring flymake.el

[Sam Steingold]

> * Noam Postavsky <acbfgnif@hfref.fbheprsbetr.arg> [2017-08-18 14:52:58 -0400]:
>
> On Fri, Aug 18, 2017 at 1:59 PM, Sam Steingold <sds@gnu.org> wrote:
>>
>> PS. Do people really hate the `flymake-disallowed-file-name-regexp' patch?
>> Why?
>
> I don't see that anybody "hates" it, but rather there are questions
> about existing alternatives, because we want to avoid adding
> yet-another-option that might turn out to be redundant.
>
> Speaking of, it looks to me that (push (list "scratch[.]py\\'" nil)
> flymake-allowed-file-name-masks) would give the effect you want?

It doesn't now, but I can easily fix that.
Thanks for the idea.

-- 
Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504
http://steingoldpsychology.com http://www.childpsy.net
http://jij.org http://mideasttruth.com http://think-israel.org
((lambda (x) (list x (list 'quote x))) '(lambda (x) (list x (list 'quote x))))

Re: Refactoring flymake.el

[Stefan Monnier]

>> Can you add some file-local settings in them?
> elpy (https://github.com/jorgenschaefer/elpy) ignores them.
> I just tried

> --8<---------------cut here---------------start------------->8---
> # Local Variables:
> # flymake-mode: nil
> # End:
> --8<---------------cut here---------------end--------------->8---

> and flymake is still enabled in scratch.
> So -- no, file-local settings do not solve my problem.

We can add code to provide a new var, instead of adding the code you
added.  Then the behavior can be configured via file-local or
dir-local settings.

So my question was whether in your use case it's practical to add
file-local settings.

>> Using file-local or dir-local variables settings is often more convenient.
> Nope, because I have to deal with the "safe local variable dialogue"
> every time I visit such a file.

Such questions should only show up if the var doesn't have a proper
safe-local-variable property set.


        Stefan

Re: Refactoring flymake.el

[Leo Liu]

Hi João,

On 2017-08-17 15:40 +0100, João Távora wrote:
> I've been away from this list and away from hacking for a while now, but
> I thought I'd return with a modest proposal for refactoring flymake.el
> and making it more useful to users and also third-party extensions.
>
> Specifically, I'm thinking of separating its UI and from its
> diagnostics-generating backends. Currently the latter rely purely on
> lanching external processes and examining their output, but recent
> experimentations with the LSP (Language Server Protocol) showed that to
> not always be the case. Another example is Elisp itself, which is
> syntax-checkable without an external tool.

Thank you for working on this.

I originally asked to maintain flymake.el because I wanted to make it
work better with https://github.com/massemanet/distel (which is slime
for Erlang) for syntax checking. But I have done little to maintain
flymake :( There are a few comments on desirable changes to flymake on
this thread
https://lists.gnu.org/archive/html/emacs-devel/2013-12/msg00215.html.

Cheers,
Leo

Re: Refactoring flymake.el - jumped the gun

[João Távora]

joaotavora@gmail.com (João Távora) writes:

> So here's what I have so far. In a newly pushed git branch:
>
>   scratch/flymake-refactor
>
> there are just two commits right now:
>
>   eb34f7f5a2 Split flymake.el into flymake-proc.el and flymake-ui.el
>   13993c46a2 Add flymake-backends defcustom

Unfortunately, I seemed to have jumped the gun and inadvertently pushed
these commits into master instead of keeping them in the
scratch/flymake-refactor branch until I got some reviews, which
naturally was my intention. So sorry about that.

As I explained they are a very simple innocuous and backward compatible
refactoring, which is probably why they didn't break any tests (and
nobody apparently noticed).

However, since they were pushed in error, I can revert them if someone
feels that's necessary.

Best,
João

Re: Refactoring flymake.el - jumped the gun

[Stefan Monnier]

> However, since they were pushed in error, I can revert them if someone
> feels that's necessary.

I haven't had time to look at them, really, but what I oversaw seemed to
go in the right direction, so we may need further patches but I think
it's OK to build on what you pushed, without having to revert first.


        Stefan