all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Two issues with the new Flymake
@ 2017-11-03  9:50 Lele Gaifax
  2017-11-03 12:33 ` Stefan Monnier
  2017-11-03 20:17 ` Three Flymake backends Was " João Távora
  0 siblings, 2 replies; 19+ messages in thread
From: Lele Gaifax @ 2017-11-03  9:50 UTC (permalink / raw)
  To: emacs-devel

Hi all,

I'm happily using the new Flymake implementation coupled with the Python
backend (that I'm still hoping it could be merged into Emacs 26, see bug
#28808).

I have two "minor" issues with it, one I think should be corrected in Flymake,
the other by any chance related to my configuration, but was not able to
identify its source yet.

The first problem is something that happens when I add new code at the end of
the source: if I write something and then quickly enough partially delete it
with `delete-backward-char' and replace it with something else, at times
Flymake reports an error message about unexpected report from the backend; I
think it's due to the external checker reporting a problem in a region of the
buffer that isn't there anymore at the time Flymake receives/processes that
annotation. This is at worst a little annoyance, because at that time Emacs
"hangs" for a noticeable interval of time, enough to disrupt my (re)writing.

The second one is a strange interaction with the `python-shell-send-region' (a
feature that I use very seldom): when I execute that, I get the following
error message

   Error in post-command-hook (#[0
   "\304\305\303\242\306#\210r\302q\210\307\310\311\301\"\300\")\207" [nil
   (post-command on-display) #<killed buffer> (#0) remove-hook
   post-command-hook nil flymake-start remove post-command] 4]): (error
   "Selecting deleted buffer")

This does not happen in a `emacs -Q' session, so I bet it must be something
related to the activation of flymake-mode and/or with the python-flymake
backend, but as said despite my efforts I was not able to better understand
what's going on (toggle-debug-on-error didn't help). Do you have any advice on
how to investigate further?

Thanks in advance for any hint,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Two issues with the new Flymake
  2017-11-03  9:50 Two issues with the new Flymake Lele Gaifax
@ 2017-11-03 12:33 ` Stefan Monnier
  2017-11-03 14:07   ` Lele Gaifax
  2017-11-03 20:17 ` Three Flymake backends Was " João Távora
  1 sibling, 1 reply; 19+ messages in thread
From: Stefan Monnier @ 2017-11-03 12:33 UTC (permalink / raw)
  To: emacs-devel

[ Please report such problems via M-x report-emacs-bug, so they get
  a bug-number.  ]

> The first problem is something that happens when I add new code at the end of
> the source: if I write something and then quickly enough partially delete it
> with `delete-backward-char' and replace it with something else, at times
> Flymake reports an error message about unexpected report from the backend; I
> think it's due to the external checker reporting a problem in a region of the
> buffer that isn't there anymore at the time Flymake receives/processes that
> annotation. This is at worst a little annoyance, because at that time Emacs
> "hangs" for a noticeable interval of time, enough to disrupt my (re)writing.

What is the exact message you get?

> The second one is a strange interaction with the `python-shell-send-region' (a
> feature that I use very seldom): when I execute that, I get the following
> error message

>    Error in post-command-hook (#[0
>    "\304\305\303\242\306#\210r\302q\210\307\310\311\301\"\300\")\207" [nil
>    (post-command on-display) #<killed buffer> (#0) remove-hook
>    post-command-hook nil flymake-start remove post-command] 4]): (error
>    "Selecting deleted buffer")

This #[...] thingy above is the

        ((start-post-command
          ()
          (remove-hook 'post-command-hook #'start-post-command
                       nil)
          (with-current-buffer buffer
            (flymake-start (remove 'post-command deferred) force)))

function from flymake-start and the error comes from with-current-buffer
because `buffer` is not live any more when that code is run.

Not sure why that would happen, but here's my guess:

python-shell-send-region calls python-shell-buffer-substring
which does:

    (with-temp-buffer
      (python-mode)
      [...]

so it generates a temp buffer, puts it in python-mode, which in turns
runs python-mode-hook and hence enables flymake-mode which then
registers itself on post-command-hook.  But of course, by the time
post-command-hook is run, that temp buffer has been killed.

So I think the patch below should fix this.  I pushed it to `emacs-26`.


        Stefan


diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 1048bc5065..649e954893 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -742,8 +742,9 @@ flymake-start
           ()
           (remove-hook 'post-command-hook #'start-post-command
                        nil)
-          (with-current-buffer buffer
-            (flymake-start (remove 'post-command deferred) force)))
+          (when (buffer-live-p buffer)
+            (with-current-buffer buffer
+              (flymake-start (remove 'post-command deferred) force))))
          (start-on-display
           ()
           (remove-hook 'window-configuration-change-hook #'start-on-display




^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: Two issues with the new Flymake
  2017-11-03 12:33 ` Stefan Monnier
@ 2017-11-03 14:07   ` Lele Gaifax
  2017-11-03 16:59     ` João Távora
  0 siblings, 1 reply; 19+ messages in thread
From: Lele Gaifax @ 2017-11-03 14:07 UTC (permalink / raw)
  To: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> [ Please report such problems via M-x report-emacs-bug, so they get
>   a bug-number.  ]

My bad, sorry.

>> The first problem is something that happens when I add new code at the end of
>> the source: if I write something and then quickly enough partially delete it
>> with `delete-backward-char' and replace it with something else, at times
>> Flymake reports an error message about unexpected report from the backend
>
> What is the exact message you get?

Luckily it was simple to get one (previous attempts did not succeed, most
likely because I was focused on writing the code and resisted the temptation
to do a context switch :-).

In a python-mode buffer I appended "def pippo(@):" at the end of the buffer,
and quickly deleted the last three characters (that is, "@):"): the "@"
triggered an error in the checker tool (I'm using flake8 at the moment, but I
think that's irrelevant), and as soon as Flymake got that it emitted

  error in process sentinel: flymake-error: [Flymake] Invalid region line=417 col=13
  error in process sentinel: [Flymake] Invalid region line=417 col=13

obviously because line 417 shrinked to 10 characters.

>
>> The second one is a strange interaction with the `python-shell-send-region' (a
>> feature that I use very seldom): when I execute that, I get the following
>> error message
>
>>    Error in post-command-hook (#[0
>>    "\304\305\303\242\306#\210r\302q\210\307\310\311\301\"\300\")\207" [nil
>>    (post-command on-display) #<killed buffer> (#0) remove-hook
>>    post-command-hook nil flymake-start remove post-command] 4]): (error
>>    "Selecting deleted buffer")
>
> This #[...] thingy above is the
>
>         ((start-post-command
>           ()
>           (remove-hook 'post-command-hook #'start-post-command
>                        nil)
>           (with-current-buffer buffer
>             (flymake-start (remove 'post-command deferred) force)))
>
> function from flymake-start and the error comes from with-current-buffer
> because `buffer` is not live any more when that code is run.

Thanks a lot for the analysis and even more for the explanation!

> ...
>
> So I think the patch below should fix this.  I pushed it to `emacs-26`.

Great, will try that as soon as I can upgrade my installation and will report
back.

Thanks again,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Two issues with the new Flymake
  2017-11-03 14:07   ` Lele Gaifax
@ 2017-11-03 16:59     ` João Távora
  2017-11-03 17:15       ` Stefan Monnier
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-03 16:59 UTC (permalink / raw)
  To: Lele Gaifax; +Cc: emacs-devel

Lele Gaifax <lele@metapensiero.it> writes:

> In a python-mode buffer I appended "def pippo(@):" at the end of the buffer,
> and quickly deleted the last three characters (that is, "@):"): the "@"
> triggered an error in the checker tool (I'm using flake8 at the moment, but I
> think that's irrelevant), and as soon as Flymake got that it emitted
>
>   error in process sentinel: flymake-error: [Flymake] Invalid region line=417 col=13
>   error in process sentinel: [Flymake] Invalid region line=417 col=13
>
> obviously because line 417 shrinked to 10 characters.
>

Thanks for explaining. I think flymake-diag-region is too alarmist in
this case, which is reasonably normal, so I demoted the handling of this
error to a warning to the *Flymake log* buffer.

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index c2349d8c7c..e13d79770e 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -342,7 +342,7 @@ flymake-diag-region
                 (let* ((beg (fallback-bol))
                        (end (fallback-eol beg)))
                   (cons beg end)))))))
-    (error (flymake-error "Invalid region line=%s col=%s" line col))))
+    (error (flymake-log :warning "Invalid region line=%s col=%s" line col))))
 
 (defvar flymake-diagnostic-functions nil
   "Special hook of Flymake backends that check a buffer.

Regarding your pending contribution for this python backend I will post
separately very soon.

João





^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: Two issues with the new Flymake
  2017-11-03 16:59     ` João Távora
@ 2017-11-03 17:15       ` Stefan Monnier
  0 siblings, 0 replies; 19+ messages in thread
From: Stefan Monnier @ 2017-11-03 17:15 UTC (permalink / raw)
  To: emacs-devel

> -    (error (flymake-error "Invalid region line=%s col=%s" line col))))
> +    (error (flymake-log :warning "Invalid region line=%s col=%s" line col))))

BTW, for the `master` branch, I think flymake should try to keep track
of which pats of the buffer have changed since the process was launched,
so that it can distinguish between spurious errors due to out-of-date
information (as in this case), and real errors.

At the very least it could keep enough info to know if *some* change
took place or not (e.g. remember the value of buffer-text-modified-tick
when the backend was launched).

Again, this is not for the `emacs-26` branch.


        Stefan




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-03  9:50 Two issues with the new Flymake Lele Gaifax
  2017-11-03 12:33 ` Stefan Monnier
@ 2017-11-03 20:17 ` João Távora
  2017-11-04 15:30   ` Stefan Monnier
  2017-11-05 12:50   ` Dmitry Gutov
  1 sibling, 2 replies; 19+ messages in thread
From: João Távora @ 2017-11-03 20:17 UTC (permalink / raw)
  To: Lele Gaifax; +Cc: Mark Oteiza, Stefan Monnier, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 6811 bytes --]

Lele Gaifax <lele@metapensiero.it> writes:

> Hi all,
>
> I'm happily using the new Flymake implementation coupled with the Python
> backend (that I'm still hoping it could be merged into Emacs 26, see bug
> #28808).

Hi,

I attach patches for Lele's Python backend, which would close 28808 and
I'm also throwing in backends for Ruby and Perl.

I think this type of Flymake backends was given a green flag by the
maintainers while ago, *for emacs 26*, but I will wait a couple of days
for objections.

A second point that I wish to raise pertains to what to do in master,
going forward.  As I had predicted, these backends look ridiculously
alike, minus very small differences. They all pipe buffer contents into
checker tools, regexp-search the output, and emit flymake diagnostics.

Obviously, this makes maintenance hard if small fixes are made to the
common structure. The tex-chktex backend recently contributed by Mark is
already an example of that in that it is missing a fix that this type of
backend requires (I pushed a fix in the meantime).

So I'm thinking that, for master (_not_ emacs-26) we could use a
declarative flymake-define-simple-backend macro.

The problem with these macros it that its easy to make them either too
flexible or not flexible enough (or both in the common case :-)).
Anyway, here's my idea for that macro preceded by a examples of how it
simplifies the backends that I propose.

   (flymake-define-simple-backend
    ruby-flymake
    ruby-flymake-command
    '("^\\(?:.*.rb\\|-\\):\\([0-9]+\\): \\(.*\\)$" 1 nil nil 2))
    
   (flymake-define-simple-backend
    perl-flymake
    perl-flymake-command
    '("^\\(.+\\) at - line \\([0-9]+\\)" 2 nil nil 1)
    (lambda (msg)
      (if (string-match
           "\\(Scalar value\\|Useless use\\|Unquoted string\\)"
           msg)
          :warning
        :error)))
    
   (flymake-define-simple-backend
    python-flymake
    python-flymake-command
    python-flymake-command-output-pattern
    (lambda (text)
      (assoc-default
       text
       python-flymake-msg-alist
       #'string-match)))

And now the macro.

  (defvar-local flymake--simple-backend-procs
    (make-hash-table))
   
  (defmacro flymake-define-simple-backend (name command pattern &optional type-predicate)
    "Define a simple Flymake backend under NAME.
  This backend runs the COMMAND syntax tool, passes the current
  buffer contents to its standard input, and uses PATTERN to
  examine the output and look for diagnostic messages.
   
  PATTERN must evaluate to a list of the form (REGEXP LINE COLUMN
  TYPE MESSAGE): if REGEXP matches, the LINE'th subexpression gives
  the line number, the COLUMN'th subexpression gives the column
  number on that line, the TYPE'th subexpression gives the type of
  the message and the MESSAGE'th gives the message text itself.
   
  If COLUMN or TYPE are nil or that index didn't match, that
  information is not present on the matched line and a default will
  be used."
    (let ((name-once name))
      `(defun ,name-once (report-fn &rest _args)
         "A Flymake backend defined with
  `flymake-define-simple-backend'."
         (let* ((command-once ,command)
                (pattern-once ,pattern)
                (pred-once (function ,type-predicate))
                (process (gethash ',name-once flymake--simple-backend-procs))
                (source (current-buffer)))
           (unless (executable-find (car command-once))
             (error "Cannot find a suitable checker"))
           (when (process-live-p process)
             (kill-process process))
           (save-restriction
             (widen)
             (setq
              process
              (puthash
               ',name-once
               (make-process
                :name (symbol-name ',name-once) :noquery t :connection-type 'pipe
                :buffer (generate-new-buffer
                         (format " *simple backend %s*" ',name-once))
                :command command-once
                :sentinel
                (lambda (proc _event)
                  (when (eq 'exit (process-status proc))
                    (unwind-protect
                        (if (with-current-buffer source
                              (eq proc
                                  (gethash ',name-once flymake--simple-backend-procs)))
                            (with-current-buffer (process-buffer proc)
                              (goto-char (point-min))
                              (cl-loop
                               while (search-forward-regexp
                                      (nth 0 pattern-once)
                                      nil t)
                               for msg = (match-string
                                          (nth 4 pattern-once))
                               for (beg . end) = (flymake-diag-region
                                                  source
                                                  (string-to-number
                                                   (match-string
                                                    (nth 1 pattern-once)))
                                                  (if (nth 2 pattern-once)
                                                      (string-to-number
                                                       (match-string
                                                        (nth 3 pattern-once)))))
                               for match-target = (if (nth 3 pattern-once)
                                                      (match-string
                                                       (nth 3 pattern-once))
                                                    msg)
                               for type = (if pred-once
                                              (funcall pred-once match-target)
                                            (if (string-match "warning" match-target)
                                                :warning
                                              :error))
                               collect (flymake-make-diagnostic source
                                                                beg
                                                                end
                                                                type
                                                                msg)
                               into diags
                               finally (funcall report-fn diags)))
                          (flymake-log :debug "Canceling obsolete check %s"
                                       proc))
                      (kill-buffer (process-buffer proc))))))
               flymake--simple-backend-procs))
             (process-send-region process (point-min) (point-max))
             (process-send-eof process))))))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-Flymake-backend-for-Python.patch --]
[-- Type: text/x-diff, Size: 7109 bytes --]

From fd800a9e16493872ff3c8244a2e30e2d9e61fca4 Mon Sep 17 00:00:00 2001
From: Lele Gaifax <lele@metapensiero.it>
Date: Fri, 3 Nov 2017 12:20:36 +0000
Subject: [PATCH 1/3] Add a Flymake backend for Python

Implement new Flymake backend with related customizable settings.

* lisp/progmodes/python.el (python-flymake-command)
(python-flymake-command-output-pattern)
(python-flymake-msg-alist): New defcustom.
(python--flymake-parse-output): New function, able to parse
python-flymake-command output accordingly to
python-flymake-command-output-pattern.
(python-flymake): New function implementing the backend
interface using python--flymake-parse-output for the real
work.
(python-mode): Add python-flymake to flymake-diagnostic-functions.
---
 lisp/progmodes/python.el | 136 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 135 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 895117b9ee..b7902fb978 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -5142,6 +5142,138 @@ python-util-valid-regexp-p
   (ignore-errors (string-match regexp "") t))
 
 \f
+;;; Flymake integration
+
+(defgroup python-flymake nil
+  "Integration between Python and Flymake."
+  :group 'python
+  :link '(custom-group-link :tag "Flymake" flymake)
+  :version "26.1")
+
+(defcustom python-flymake-command '("pyflakes")
+  "The external tool that will be used to perform the syntax check.
+This is a non empty list of strings, the checker tool possibly followed by
+required arguments.  Once launched it will receive the Python source to be
+checked as its standard input.
+To use `flake8' you would set this to (\"flake8\" \"-\")."
+  :group 'python-flymake
+  :type '(repeat string))
+
+;; The default regexp accomodates for older pyflakes, which did not
+;; report the column number, and at the same time it's compatible with
+;; flake8 output, although it may be redefined to explicitly match the
+;; TYPE
+(defcustom python-flymake-command-output-pattern
+  (list
+   "^\\(?:<?stdin>?\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
+   1 2 nil 3)
+  "Specify how to parse the output of `python-flymake-command'.
+The value has the form (REGEXP LINE COLUMN TYPE MESSAGE): if
+REGEXP matches, the LINE'th subexpression gives the line number,
+the COLUMN'th subexpression gives the column number on that line,
+the TYPE'th subexpression gives the type of the message and the
+MESSAGE'th gives the message text itself.
+
+If COLUMN or TYPE are nil or that index didn't match, that
+information is not present on the matched line and a default will
+be used."
+  :group 'python-flymake
+  :type '(list regexp
+               (integer :tag "Line's index")
+               (choice
+                (const :tag "No column" nil)
+                (integer :tag "Column's index"))
+               (choice
+                (const :tag "No type" nil)
+                (integer :tag "Type's index"))
+               (integer :tag "Message's index")))
+
+(defcustom python-flymake-msg-alist
+  '(("\\(^redefinition\\|.*unused.*\\|used$\\)" . :warning))
+  "Alist used to associate messages to their types.
+Each element should be a cons-cell (REGEXP . TYPE), where TYPE must be
+one defined in the variable `flymake-diagnostic-types-alist'.
+For example, when using `flake8' a possible configuration could be:
+
+  ((\"\\(^redefinition\\|.*unused.*\\|used$\\)\" . :warning)
+   (\"^E999\" . :error)
+   (\"^[EW][0-9]+\" . :note))
+
+By default messages are considered errors."
+  :group 'python-flymake
+  :type `(alist :key-type (regexp)
+                :value-type (symbol)))
+
+(defvar-local python--flymake-proc nil)
+
+(defun python--flymake-parse-output (source proc report-fn)
+  "Collect diagnostics parsing checker tool's output line by line."
+  (let ((rx (nth 0 python-flymake-command-output-pattern))
+        (lineidx (nth 1 python-flymake-command-output-pattern))
+        (colidx (nth 2 python-flymake-command-output-pattern))
+        (typeidx (nth 3 python-flymake-command-output-pattern))
+        (msgidx (nth 4 python-flymake-command-output-pattern)))
+    (with-current-buffer (process-buffer proc)
+      (goto-char (point-min))
+      (cl-loop
+       while (search-forward-regexp rx nil t)
+       for msg = (match-string msgidx)
+       for (beg . end) = (flymake-diag-region
+                          source
+                          (string-to-number
+                           (match-string lineidx))
+                          (and colidx
+                               (match-string colidx)
+                               (string-to-number
+                                (match-string colidx))))
+       for type = (or (and typeidx
+                           (match-string typeidx)
+                           (assoc-default
+                            (match-string typeidx)
+                            python-flymake-msg-alist
+                            #'string-match))
+                      (assoc-default msg
+                                     python-flymake-msg-alist
+                                     #'string-match)
+                      :error)
+       collect (flymake-make-diagnostic
+                source beg end type msg)
+       into diags
+       finally (funcall report-fn diags)))))
+
+(defun python-flymake (report-fn &rest _args)
+  "Flymake backend for Python.
+This backend uses `python-flymake-command' (which see) to launch a process
+that is passed the current buffer's content via stdin.
+REPORT-FN is Flymake's callback function."
+  (unless (executable-find (car python-flymake-command))
+    (error "Cannot find a suitable checker"))
+
+  (when (process-live-p python--flymake-proc)
+    (kill-process python--flymake-proc))
+
+  (let ((source (current-buffer)))
+    (save-restriction
+      (widen)
+      (setq python--flymake-proc
+            (make-process
+             :name "python-flymake"
+             :noquery t
+             :connection-type 'pipe
+             :buffer (generate-new-buffer " *python-flymake*")
+             :command python-flymake-command
+             :sentinel
+             (lambda (proc _event)
+               (when (eq 'exit (process-status proc))
+                 (unwind-protect
+                     (when (with-current-buffer source
+                             (eq proc python--flymake-proc))
+                       (python--flymake-parse-output source proc report-fn))
+                   (kill-buffer (process-buffer proc)))))))
+      (process-send-region python--flymake-proc (point-min) (point-max))
+      (process-send-eof python--flymake-proc))))
+
+\f
 (defun python-electric-pair-string-delimiter ()
   (when (and electric-pair-mode
              (memq last-command-event '(?\" ?\'))
@@ -5255,7 +5387,9 @@ python-mode
   (make-local-variable 'python-shell-internal-buffer)
 
   (when python-indent-guess-indent-offset
-    (python-indent-guess-indent-offset)))
+    (python-indent-guess-indent-offset))
+
+  (add-hook 'flymake-diagnostic-functions #'python-flymake nil t))
 
 
 (provide 'python)
-- 
2.14.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-a-Flymake-backend-for-Perl.patch --]
[-- Type: text/x-diff, Size: 5877 bytes --]

From 88440e31f48cd2fb44fef4d6b4ecaaf017246002 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Fri, 3 Nov 2017 16:05:39 +0000
Subject: [PATCH 2/3] Add a Flymake backend for Perl

Define a simple backend in perl-mode.el, which cperl-mode.el also
uses.

* lisp/progmodes/cperl-mode.el (cperl-mode): Add to
flymake-diagnostic-functions.

* lisp/progmodes/flymake-proc.el
(flymake-proc-allowed-file-name-masks): Disable legacy backend
for perl files.

* lisp/progmodes/perl-mode.el (perl-flymake-command): New
defcustom.
(perl--flymake-proc): New buffer-local variable.
(perl-flymake): New function.
(perl-mode): Add to flymake-diagnostic-functions.
---
 lisp/progmodes/cperl-mode.el   |  4 ++-
 lisp/progmodes/flymake-proc.el |  2 +-
 lisp/progmodes/perl-mode.el    | 71 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 853604d1d7..4c63ec2fb4 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -1896,7 +1896,9 @@ cperl-mode
   (if cperl-pod-here-scan
       (or cperl-syntaxify-by-font-lock
        (progn (or cperl-faces-init (cperl-init-faces-weak))
-	      (cperl-find-pods-heres)))))
+	      (cperl-find-pods-heres))))
+  ;; Setup Flymake
+  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
 \f
 ;; Fix for perldb - make default reasonable
 (defun cperl-db ()
diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index 5c4d451d63..2e98b2afd1 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -73,7 +73,7 @@ flymake-proc-allowed-file-name-masks
     ("\\.xml\\'" flymake-proc-xml-init)
     ("\\.html?\\'" flymake-proc-xml-init)
     ("\\.cs\\'" flymake-proc-simple-make-init)
-    ("\\.p[ml]\\'" flymake-proc-perl-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)
diff --git a/lisp/progmodes/perl-mode.el b/lisp/progmodes/perl-mode.el
index 24b934ce6c..0f1088920a 100644
--- a/lisp/progmodes/perl-mode.el
+++ b/lisp/progmodes/perl-mode.el
@@ -581,6 +581,73 @@ perl-current-defun-name
 	(match-string-no-properties 1))))
 
 \f
+;;; Flymake support
+(defcustom perl-flymake-command '("perl" "-w" "-c")
+  "External tool used to check Perl source code.
+This is a non empty list of strings, the checker tool possibly
+followed by required arguments.  Once launched it will receive
+the Perl source to be checked as its standard input."
+  :group 'perl
+  :type '(repeat string))
+
+(defvar-local perl--flymake-proc nil)
+
+;;;###autoload
+(defun perl-flymake (report-fn &rest _args)
+  "Perl backend for Flymake.  Launches
+`perl-flymake-command' (which see) and passes to its standard
+input the contents of the current buffer.  The output of this
+command is analysed for error and warning messages."
+  (unless (executable-find (car perl-flymake-command))
+    (error "Cannot find a suitable checker"))
+
+  (when (process-live-p perl--flymake-proc)
+    (kill-process perl--flymake-proc))
+
+  (let ((source (current-buffer)))
+    (save-restriction
+      (widen)
+      (setq
+       perl--flymake-proc
+       (make-process
+        :name "perl-flymake" :noquery t :connection-type 'pipe
+        :buffer (generate-new-buffer " *perl-flymake*")
+        :command perl-flymake-command
+        :sentinel
+        (lambda (proc _event)
+          (when (eq 'exit (process-status proc))
+            (unwind-protect
+                (if (with-current-buffer source (eq proc perl--flymake-proc))
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (cl-loop
+                       while (search-forward-regexp
+                              "^\\(.+\\) at - line \\([0-9]+\\)"
+                              nil t)
+                       for msg = (match-string 1)
+                       for (beg . end) = (flymake-diag-region
+                                          source
+                                          (string-to-number (match-string 2)))
+                       for type =
+                       (if (string-match
+                            "\\(Scalar value\\|Useless use\\|Unquoted string\\)"
+                            msg)
+                           :warning
+                         :error)
+                       collect (flymake-make-diagnostic source
+                                                        beg
+                                                        end
+                                                        type
+                                                        msg)
+                       into diags
+                       finally (funcall report-fn diags)))
+                  (flymake-log :debug "Canceling obsolete check %s"
+                               proc))
+              (display-buffer (process-buffer proc)))))))
+      (process-send-region perl--flymake-proc (point-min) (point-max))
+      (process-send-eof perl--flymake-proc))))
+
+\f
 (defvar perl-mode-hook nil
   "Normal hook to run when entering Perl mode.")
 
@@ -665,7 +732,9 @@ perl-mode
   ;; Setup outline-minor-mode.
   (setq-local outline-regexp perl-outline-regexp)
   (setq-local outline-level 'perl-outline-level)
-  (setq-local add-log-current-defun-function #'perl-current-defun-name))
+  (setq-local add-log-current-defun-function #'perl-current-defun-name)
+  ;; Setup Flymake
+  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
 \f
 ;; This is used by indent-for-comment
 ;; to decide how much to indent a comment in Perl code
-- 
2.14.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Add-a-Flymake-backend-for-Perl.patch --]
[-- Type: text/x-diff, Size: 5877 bytes --]

From 88440e31f48cd2fb44fef4d6b4ecaaf017246002 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Fri, 3 Nov 2017 16:05:39 +0000
Subject: [PATCH 3/3] Add a Flymake backend for Perl

Define a simple backend in perl-mode.el, which cperl-mode.el also
uses.

* lisp/progmodes/cperl-mode.el (cperl-mode): Add to
flymake-diagnostic-functions.

* lisp/progmodes/flymake-proc.el
(flymake-proc-allowed-file-name-masks): Disable legacy backend
for perl files.

* lisp/progmodes/perl-mode.el (perl-flymake-command): New
defcustom.
(perl--flymake-proc): New buffer-local variable.
(perl-flymake): New function.
(perl-mode): Add to flymake-diagnostic-functions.
---
 lisp/progmodes/cperl-mode.el   |  4 ++-
 lisp/progmodes/flymake-proc.el |  2 +-
 lisp/progmodes/perl-mode.el    | 71 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 853604d1d7..4c63ec2fb4 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -1896,7 +1896,9 @@ cperl-mode
   (if cperl-pod-here-scan
       (or cperl-syntaxify-by-font-lock
        (progn (or cperl-faces-init (cperl-init-faces-weak))
-	      (cperl-find-pods-heres)))))
+	      (cperl-find-pods-heres))))
+  ;; Setup Flymake
+  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
 \f
 ;; Fix for perldb - make default reasonable
 (defun cperl-db ()
diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index 5c4d451d63..2e98b2afd1 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -73,7 +73,7 @@ flymake-proc-allowed-file-name-masks
     ("\\.xml\\'" flymake-proc-xml-init)
     ("\\.html?\\'" flymake-proc-xml-init)
     ("\\.cs\\'" flymake-proc-simple-make-init)
-    ("\\.p[ml]\\'" flymake-proc-perl-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)
diff --git a/lisp/progmodes/perl-mode.el b/lisp/progmodes/perl-mode.el
index 24b934ce6c..0f1088920a 100644
--- a/lisp/progmodes/perl-mode.el
+++ b/lisp/progmodes/perl-mode.el
@@ -581,6 +581,73 @@ perl-current-defun-name
 	(match-string-no-properties 1))))
 
 \f
+;;; Flymake support
+(defcustom perl-flymake-command '("perl" "-w" "-c")
+  "External tool used to check Perl source code.
+This is a non empty list of strings, the checker tool possibly
+followed by required arguments.  Once launched it will receive
+the Perl source to be checked as its standard input."
+  :group 'perl
+  :type '(repeat string))
+
+(defvar-local perl--flymake-proc nil)
+
+;;;###autoload
+(defun perl-flymake (report-fn &rest _args)
+  "Perl backend for Flymake.  Launches
+`perl-flymake-command' (which see) and passes to its standard
+input the contents of the current buffer.  The output of this
+command is analysed for error and warning messages."
+  (unless (executable-find (car perl-flymake-command))
+    (error "Cannot find a suitable checker"))
+
+  (when (process-live-p perl--flymake-proc)
+    (kill-process perl--flymake-proc))
+
+  (let ((source (current-buffer)))
+    (save-restriction
+      (widen)
+      (setq
+       perl--flymake-proc
+       (make-process
+        :name "perl-flymake" :noquery t :connection-type 'pipe
+        :buffer (generate-new-buffer " *perl-flymake*")
+        :command perl-flymake-command
+        :sentinel
+        (lambda (proc _event)
+          (when (eq 'exit (process-status proc))
+            (unwind-protect
+                (if (with-current-buffer source (eq proc perl--flymake-proc))
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (cl-loop
+                       while (search-forward-regexp
+                              "^\\(.+\\) at - line \\([0-9]+\\)"
+                              nil t)
+                       for msg = (match-string 1)
+                       for (beg . end) = (flymake-diag-region
+                                          source
+                                          (string-to-number (match-string 2)))
+                       for type =
+                       (if (string-match
+                            "\\(Scalar value\\|Useless use\\|Unquoted string\\)"
+                            msg)
+                           :warning
+                         :error)
+                       collect (flymake-make-diagnostic source
+                                                        beg
+                                                        end
+                                                        type
+                                                        msg)
+                       into diags
+                       finally (funcall report-fn diags)))
+                  (flymake-log :debug "Canceling obsolete check %s"
+                               proc))
+              (display-buffer (process-buffer proc)))))))
+      (process-send-region perl--flymake-proc (point-min) (point-max))
+      (process-send-eof perl--flymake-proc))))
+
+\f
 (defvar perl-mode-hook nil
   "Normal hook to run when entering Perl mode.")
 
@@ -665,7 +732,9 @@ perl-mode
   ;; Setup outline-minor-mode.
   (setq-local outline-regexp perl-outline-regexp)
   (setq-local outline-level 'perl-outline-level)
-  (setq-local add-log-current-defun-function #'perl-current-defun-name))
+  (setq-local add-log-current-defun-function #'perl-current-defun-name)
+  ;; Setup Flymake
+  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
 \f
 ;; This is used by indent-for-comment
 ;; to decide how much to indent a comment in Perl code
-- 
2.14.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-03 20:17 ` Three Flymake backends Was " João Távora
@ 2017-11-04 15:30   ` Stefan Monnier
  2017-11-04 23:17     ` João Távora
  2017-11-05 12:50   ` Dmitry Gutov
  1 sibling, 1 reply; 19+ messages in thread
From: Stefan Monnier @ 2017-11-04 15:30 UTC (permalink / raw)
  To: emacs-devel

FWIW, I think we can install those backends into emacs-26, and later
consolidate them in `master`.

> Obviously, this makes maintenance hard if small fixes are made to the
> common structure. The tex-chktex backend recently contributed by Mark is
> already an example of that in that it is missing a fix that this type of
> backend requires (I pushed a fix in the meantime).
>
> So I'm thinking that, for master (_not_ emacs-26) we could use a
> declarative flymake-define-simple-backend macro.

FWIW, I think that "makes maintenance hard if small fixes are made to the
common structure" is also an argument against the use of a macro (or
at least against putting the common structure in the macro's expansion),
since it would imply that you need to recompile the backend in order to
benefit from fixes in the common structure.

Also using a macro like you did makes it difficult to debug/edebug the
common code.  Better move that common code into a function.

See additional comments about the code below,


        Stefan


>   (defvar-local flymake--simple-backend-procs
>     (make-hash-table))

You never `setq` this var, so it will behave like a global var.
More specifically, a single hash-table is used for all buffers, which
I believe is not what you intended.

>   (defmacro flymake-define-simple-backend (name command pattern &optional type-predicate)
>     "Define a simple Flymake backend under NAME.

I had to look at the code to understand what you meant by "under NAME".
I'd have written it more like "Define Flymake backend function named
NAME", maybe.

>   PATTERN must evaluate to a list of the form (REGEXP LINE COLUMN
>   TYPE MESSAGE): if REGEXP matches, the LINE'th subexpression gives
>   the line number, the COLUMN'th subexpression gives the column
>   number on that line, the TYPE'th subexpression gives the type of
>   the message and the MESSAGE'th gives the message text itself.

Line numbers are reasonably standardized, but column numbers aren't.
Some tools start counting from 0 others from 1, some count bytes, others
count chars, yet others count actual "visual" columns.  `compile.el` has
added some vars to control this, but it's rather messy.  So we should
make it possible to provide code which does the conversion to whatever
we use.

Maybe it would also make sense to try and support message which include
both the BEG and the END.

Rather than `type-predicate`, I'd rather just allow `type` to be a function.
Also it'd probably make sense to allow the type returned by that
function to be nil in which case we should just skip the match
(i.e. not create a diagnostic for it).

So I guess I'm looking at something that look like
a `flymake-simple-backend` with signature:

   (flymake-simple-backend COMMAND REGEXP MSG TYPE START &optional END)

where MSG and TYPE are normally integers, and START and END can be
either an integer (line-number submatch) or a list of 2 integers (LINE
COL), and all 4 of those could be functions instead.

>     (let ((name-once name))

I must say I don't get what this <foo>-once business is about.
What is `-once` supposed to mean?  What did you need it for?

> +  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
                                             ^^
                                             #'

-- Stefan




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-04 15:30   ` Stefan Monnier
@ 2017-11-04 23:17     ` João Távora
  0 siblings, 0 replies; 19+ messages in thread
From: João Távora @ 2017-11-04 23:17 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> FWIW, I think we can install those backends into emacs-26, and later
> consolidate them in `master`.

That's my idea too.

> Also using a macro like you did makes it difficult to debug/edebug the
> common code.  Better move that common code into a function.

Sure, of course, what I proposed was just a quick draft. Sorry if I
passed the idea that I wanted a full review. I mostly wanted how
something like this could simplify the backends.

>>   (defvar-local flymake--simple-backend-procs
>>     (make-hash-table))
>
> You never `setq` this var, so it will behave like a global var.
> More specifically, a single hash-table is used for all buffers, which
> I believe is not what you intended.

Yep, that's a bug. Thanks.

>>   (defmacro flymake-define-simple-backend (name command pattern &optional type-predicate)
>>     "Define a simple Flymake backend under NAME.
>
> I had to look at the code to understand what you meant by "under NAME".
>
Then it wasn't such a bad idea haha :-)

>>   PATTERN must evaluate to a list of the form (REGEXP LINE COLUMN
>>   TYPE MESSAGE): if REGEXP matches, the LINE'th subexpression gives
>>   the line number, the COLUMN'th subexpression gives the column
>>   number on that line, the TYPE'th subexpression gives the type of
>>   the message and the MESSAGE'th gives the message text itself.
>
> Line numbers are reasonably standardized, but column numbers aren't.
> Some tools start counting from 0 others from 1, some count bytes, others
> count chars, yet others count actual "visual" columns.  `compile.el` has
> added some vars to control this, but it's rather messy.  So we should
> make it possible to provide code which does the conversion to whatever
> we use.
>
> Maybe it would also make sense to try and support message which include
> both the BEG and the END.
>
> Rather than `type-predicate`, I'd rather just allow `type` to be a function.
> Also it'd probably make sense to allow the type returned by that
> function to be nil in which case we should just skip the match
> (i.e. not create a diagnostic for it).

All this makes sense, but what could also be done is just use
compile.el. All the code we're talking about has been written before. We
should just let compile.el do the parsing. I have a prototype of this
with a few changes to compile.el that I attach as a prelimiary
patch. I'm still using a lot of compile.el internals that would have to
be exported somehow.

Obviously, the advantage is that writing a backend for a language that
compile.el already supports would become as easy as:

  (flymake-define-simple-backend
   language-flymake
   language-tool-command-that-supports-stdin
   '(language)
   "stdin")

It *almost* works. It might be slow, didn't measure. And for the moment
is particularly naive when looking for the compilation message text and
perl, for example, fools it because the message text comes before the
link. But the big problem that I haven't been able to to debug, is that
when invoked in this barebones manner, compile.el leaves all the
compilation messages at same type, so everything's an error in C code,
for example.

>>     (let ((name-once name))
>
> I must say I don't get what this <foo>-once business is about.
> What is `-once` supposed to mean?  What did you need it for?

name-once was probably a brainf*rt but the others are so that do don't
evaluate the form passed to the macro more than once, because it can be
something with side-effects. Nevermind, it goes away once you use the
function anyway.

>> +  (add-hook 'flymake-diagnostic-functions 'perl-flymake nil t))
>                                            #'

I did this for consistency of style with the rest of Emacs because I'd
never seen #' in add-hooks before, but now I see a few, though the vast
majority doesn't have it. Also I have a (probably misguided) tendency to
think of function objects as stuff that isn't quite as `eq' as symbols
interned somewhere, so not so good for lists you might want to `delq'
later..

João

So the patch here. Notice that in flymake.el I take the opportunity to
tidy up and move some functions around.

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 0794830fcb..ca5b27acb4 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -533,7 +533,7 @@ compilation-error-regexp-alist-alist
   "Alist of values for `compilation-error-regexp-alist'.")
 
 (defcustom compilation-error-regexp-alist
-  (mapcar 'car compilation-error-regexp-alist-alist)
+  (mapcar 'car compilation-error-regexp-alist-alist))
   "Alist that specifies how to match errors in compiler output.
 On GNU and Unix, any string is a valid filename, so these
 matchers must make some common sense assumptions, which catch
@@ -1228,34 +1228,34 @@ compilation-internal-error-properties
                (if (local-variable-p 'compilation-first-column)
                    compilation-first-column first-column)))
           (save-excursion
-	  (save-restriction
-	    (widen)
-	    (goto-char (marker-position marker))
-	    ;; Set end-marker if appropriate and go to line.
-	    (if (not (or end-col end-line))
-		(compilation-beginning-of-line (- line marker-line -1))
-	      (compilation-beginning-of-line (- (or end-line line)
-                                                marker-line -1))
-	      (if (or (null end-col) (< end-col 0))
-		  (end-of-line)
-		(compilation-move-to-column end-col screen-columns))
-	      (setq end-marker (point-marker))
-	      (when end-line
-                (compilation-beginning-of-line (- line end-line -1))))
-	    (if col
-		(compilation-move-to-column col screen-columns)
-	      (forward-to-indentation 0))
-	    (setq marker (point-marker)))))))
+	    (save-restriction
+	      (widen)
+	      (goto-char (marker-position marker))
+	      ;; Set end-marker if appropriate and go to line.
+	      (if (not (or end-col end-line))
+		  (compilation-beginning-of-line (- line marker-line -1))
+	        (compilation-beginning-of-line (- (or end-line line)
+                                                  marker-line -1))
+	        (if (or (null end-col) (< end-col 0))
+		    (end-of-line)
+		  (compilation-move-to-column end-col screen-columns))
+	        (setq end-marker (point-marker))
+	        (when end-line
+                  (compilation-beginning-of-line (- line end-line -1))))
+	      (if col
+		  (compilation-move-to-column col screen-columns)
+	        (forward-to-indentation 0))
+	      (setq marker (point-marker)))))))
 
     (setq loc (compilation-assq line (compilation--file-struct->loc-tree
                                       file-struct)))
     (setq end-loc
-    (if end-line
+          (if end-line
               (compilation-assq
                end-col (compilation-assq
                         end-line (compilation--file-struct->loc-tree
                                   file-struct)))
-      (if end-col			; use same line element
+            (if end-col			; use same line element
                 (compilation-assq end-col loc))))
     (setq loc (compilation-assq col loc))
     ;; If they are new, make the loc(s) reference the file they point to.
@@ -1275,17 +1275,16 @@ compilation-internal-error-properties
 	    (setcdr end-loc
                     (compilation--make-cdrloc (or end-line line) file-struct
                                               end-marker))))
-
     ;; Must start with face
     `(font-lock-face ,compilation-message-face
-      compilation-message ,(compilation--make-message loc type end-loc)
-      help-echo ,(if col
-                     "mouse-2: visit this file, line and column"
-                   (if line
-                       "mouse-2: visit this file and line"
-                     "mouse-2: visit this file"))
-      keymap compilation-button-map
-      mouse-face highlight)))
+                     compilation-message ,(compilation--make-message loc type end-loc)
+                     help-echo ,(if col
+                                    "mouse-2: visit this file, line and column"
+                                  (if line
+                                      "mouse-2: visit this file and line"
+                                    "mouse-2: visit this file"))
+                     keymap compilation-button-map
+                     mouse-face highlight)))
 
 (defun compilation--put-prop (matchnum prop val)
   (when (and (integerp matchnum) (match-beginning matchnum))
@@ -2480,6 +2479,38 @@ compilation-find-buffer
       (current-buffer)
     (next-error-find-buffer avoid-current 'compilation-buffer-internal-p)))
 
+;;;###autoload
+(defun compilation-compute-loc (loc)
+  "Compute the actual marker for LOC once in its target buffer.
+LOC represents a location of the error message, and is an object
+of the kind extracted with `compilation--message->loc' or
+`compilation--message->end-loc'."
+  (let ((last 1))
+    (save-excursion
+      (save-restriction
+        (widen)
+        (goto-char (point-min))
+        ;; Treat file's found lines in forward order, 1 by 1.
+        (dolist (line (reverse (cddr (compilation--loc->file-struct loc))))
+          (when (car line)		; else this is a filename w/o a line#
+            (compilation-beginning-of-line (- (car line) last -1))
+            (setq last (car line)))
+          ;; Treat line's found columns and store/update a marker for each.
+          (dolist (col (cdr line))
+            (if (compilation--loc->col col)
+                (if (eq (compilation--loc->col col) -1)
+                    ;; Special case for range end.
+                    (end-of-line)
+                  (compilation-move-to-column (compilation--loc->col col)
+                                              compilation-error-screen-columns))
+              (beginning-of-line)
+              (skip-chars-forward " \t"))
+            (if (compilation--loc->marker col)
+                (set-marker (compilation--loc->marker col) (point))
+              (setf (compilation--loc->marker col) (point-marker)))
+            ;; (setf (compilation--loc->timestamp col) timestamp)
+            ))))))
+
 ;;;###autoload
 (defun compilation-next-error-function (n &optional reset)
   "Advance to the next error message and visit the file where the error was.
@@ -2489,7 +2520,6 @@ compilation-next-error-function
     (setq compilation-current-error nil))
   (let* ((screen-columns compilation-error-screen-columns)
 	 (first-column compilation-first-column)
-	 (last 1)
 	 (msg (compilation-next-error (or n 1) nil
 				      (or compilation-current-error
 					  compilation-messages-start
@@ -2499,9 +2529,9 @@ compilation-next-error-function
 	 (marker (point-marker)))
     (setq compilation-current-error (point-marker)
 	  overlay-arrow-position
-	    (if (bolp)
-		compilation-current-error
-	      (copy-marker (line-beginning-position))))
+	  (if (bolp)
+	      compilation-current-error
+	    (copy-marker (line-beginning-position))))
     ;; If loc contains no marker, no error in that file has been visited.
     ;; If the marker is invalid the buffer has been killed.
     ;; So, recalculate all markers for that file.
@@ -2525,37 +2555,18 @@ compilation-next-error-function
                  (cadr (car (compilation--loc->file-struct loc)))
                  (compilation--file-struct->formats
                   (compilation--loc->file-struct loc)))
-        (let ((screen-columns
+        (let ((compilation-error-screen-columns
                ;; Obey the compilation-error-screen-columns of the target
                ;; buffer if its major mode set it buffer-locally.
+               ;;
+               ;; Why is this necessary, shouldn't just accessing the
+               ;; variable do the same indirection? -- jt@2017-10-24
                (if (local-variable-p 'compilation-error-screen-columns)
                    compilation-error-screen-columns screen-columns))
               (compilation-first-column
                (if (local-variable-p 'compilation-first-column)
                    compilation-first-column first-column)))
-          (save-restriction
-            (widen)
-            (goto-char (point-min))
-            ;; Treat file's found lines in forward order, 1 by 1.
-            (dolist (line (reverse (cddr (compilation--loc->file-struct loc))))
-              (when (car line)		; else this is a filename w/o a line#
-                (compilation-beginning-of-line (- (car line) last -1))
-                (setq last (car line)))
-              ;; Treat line's found columns and store/update a marker for each.
-              (dolist (col (cdr line))
-                (if (compilation--loc->col col)
-                    (if (eq (compilation--loc->col col) -1)
-                        ;; Special case for range end.
-                        (end-of-line)
-                      (compilation-move-to-column (compilation--loc->col col)
-                                                  screen-columns))
-                  (beginning-of-line)
-                  (skip-chars-forward " \t"))
-                (if (compilation--loc->marker col)
-                    (set-marker (compilation--loc->marker col) (point))
-                  (setf (compilation--loc->marker col) (point-marker)))
-                ;; (setf (compilation--loc->timestamp col) timestamp)
-                ))))))
+          (compilation-compute-loc loc))))
     (compilation-goto-locus marker (compilation--loc->marker loc)
                             (compilation--loc->marker end-loc))
     (setf (compilation--loc->visited loc) t)))
diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 1048bc5065..23a935481c 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -218,27 +218,6 @@ flymake-error
                (:constructor flymake--diag-make))
   buffer beg end type text backend)
 
-;;;###autoload
-(defun flymake-make-diagnostic (buffer
-                                beg
-                                end
-                                type
-                                text)
-  "Make a Flymake diagnostic for BUFFER's region from BEG to END.
-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))
-
-;;;###autoload
-(defun flymake-diagnostics (&optional beg end)
-  "Get Flymake diagnostics in region determined by BEG and END.
-
-If neither BEG or END is supplied, use the whole buffer,
-otherwise if BEG is non-nil and END is nil, consider only
-diagnostics at BEG."
-  (mapcar (lambda (ov) (overlay-get ov 'flymake-diagnostic))
-          (flymake--overlays :beg beg :end end)))
-
 (defmacro flymake--diag-accessor (public internal thing)
   "Make PUBLIC an alias for INTERNAL, add doc using THING."
   `(defsubst ,public (diag)
@@ -305,45 +284,6 @@ flymake-note
 (define-obsolete-face-alias 'flymake-warnline 'flymake-warning "26.1")
 (define-obsolete-face-alias 'flymake-errline 'flymake-error "26.1")
 
-;;;###autoload
-(defun flymake-diag-region (buffer line &optional col)
-  "Compute BUFFER's region (BEG . END) corresponding to LINE and COL.
-If COL is nil, return a region just for LINE.  Return nil if the
-region is invalid."
-  (condition-case-unless-debug _err
-      (with-current-buffer buffer
-        (let ((line (min (max line 1)
-                         (line-number-at-pos (point-max) 'absolute))))
-          (save-excursion
-            (goto-char (point-min))
-            (forward-line (1- line))
-            (cl-flet ((fallback-bol
-                       () (progn (back-to-indentation) (point)))
-                      (fallback-eol
-                       (beg)
-                       (progn
-                         (end-of-line)
-                         (skip-chars-backward " \t\f\t\n" beg)
-                         (if (eq (point) beg)
-                             (line-beginning-position 2)
-                           (point)))))
-              (if (and col (cl-plusp col))
-                  (let* ((beg (progn (forward-char (1- col))
-                                     (point)))
-                         (sexp-end (ignore-errors (end-of-thing 'sexp)))
-                         (end (or (and sexp-end
-                                       (not (= sexp-end beg))
-                                       sexp-end)
-                                  (ignore-errors (goto-char (1+ beg)))))
-                         (safe-end (or end
-                                       (fallback-eol beg))))
-                    (cons (if end beg (fallback-bol))
-                          safe-end))
-                (let* ((beg (fallback-bol))
-                       (end (fallback-eol beg)))
-                  (cons beg end)))))))
-    (error (flymake-error "Invalid region line=%s col=%s" line col))))
-
 (defvar flymake-diagnostic-functions nil
   "Special hook of Flymake backends that check a buffer.
 
@@ -1193,6 +1133,213 @@ flymake-show-diagnostics-buffer
       (revert-buffer)
       (display-buffer (current-buffer)))))
 
+\f
+;;; Utility functions (API)
+;;;
+;;;###autoload
+(defun flymake-diag-region (buffer line &optional col)
+  "Compute BUFFER's region (BEG . END) corresponding to LINE and COL.
+If COL is nil, return a region just for LINE.  Return nil if the
+region is invalid."
+  (condition-case-unless-debug _err
+      (with-current-buffer buffer
+        (let ((line (min (max line 1)
+                         (line-number-at-pos (point-max) 'absolute))))
+          (save-excursion
+            (goto-char (point-min))
+            (forward-line (1- line))
+            (cl-flet ((fallback-bol
+                       () (progn (back-to-indentation) (point)))
+                      (fallback-eol
+                       (beg)
+                       (progn
+                         (end-of-line)
+                         (skip-chars-backward " \t\f\t\n" beg)
+                         (if (eq (point) beg)
+                             (line-beginning-position 2)
+                           (point)))))
+              (if (and col (cl-plusp col))
+                  (let* ((beg (progn (forward-char (1- col))
+                                     (point)))
+                         (sexp-end (ignore-errors (end-of-thing 'sexp)))
+                         (end (or (and sexp-end
+                                       (not (= sexp-end beg))
+                                       sexp-end)
+                                  (ignore-errors (goto-char (1+ beg)))))
+                         (safe-end (or end
+                                       (fallback-eol beg))))
+                    (cons (if end beg (fallback-bol))
+                          safe-end))
+                (let* ((beg (fallback-bol))
+                       (end (fallback-eol beg)))
+                  (cons beg end)))))))
+    (error (flymake-error "Invalid region line=%s col=%s" line col))))
+
+;;;###autoload
+(defun flymake-make-diagnostic (buffer
+                                beg
+                                end
+                                type
+                                text)
+  "Make a Flymake diagnostic for BUFFER's region from BEG to END.
+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))
+
+;;;###autoload
+(defun flymake-diagnostics (&optional beg end)
+  "Get Flymake diagnostics in region determined by BEG and END.
+
+If neither BEG or END is supplied, use the whole buffer,
+otherwise if BEG is non-nil and END is nil, consider only
+diagnostics at BEG."
+  (mapcar (lambda (ov) (overlay-get ov 'flymake-diagnostic))
+          (flymake--overlays :beg beg :end end)))
+
+;;;###autoload
+(defun flymake-compilation-to-diagnostics (source-buffer
+                                           &optional
+                                           compilation-alist-symbols
+                                           buffer-filename-alias)
+  "Search current buffer for compiler diagnostics for SOURCE-BUFFER.
+BUFFER-FILENAME-ALIAS is a regular expression matching the
+filename component of those compilation messages that pertain
+exclusively to SOURCE-BUFFER.  If nil, the string \"<stdin>\" is
+used, since it is a relatively of compilers to output this string
+when checking content passed to their standard inputs.
+
+COMPILATION-ALIST-SYMBOLS is a list of symbols used to index
+`compilation-error-regexp-alist-alist' (which see) and thus
+specifying the error patterns to look for. If nil, a large (and
+slow) collection of symbols is used."
+  (set (make-local-variable 'compilation-locs)
+       (make-hash-table :test 'equal :weakness 'value))
+  (apply #'compilation-parse-errors (point-min) (point-max)
+         (or compilation-alist-symbols
+             (cl-remove-if-not #'symbolp
+                               compilation-error-regexp-alist)))
+  (let ((next (point-min))
+        (regexp (or buffer-filename-alias "<stdin>"))
+        retval)
+
+    ;; HACK the case where property starts right away and we don't
+    ;; want to skip it.
+    (goto-char (point-min)) (insert "\n")
+    (while (setq next
+                 (next-single-property-change next 'compilation-message))
+      (let ((message (get-text-property next 'compilation-message)))
+        (when message
+          (let* (;; This wont work with perl, the
+                 ;; message appears before the link. 
+                 (message-text (buffer-substring
+                                (goto-char
+                                 (setq
+                                  next
+                                  (next-single-property-change
+                                   next 'compilation-message)))
+                                (line-end-position)))
+                 (beg-loc (compilation--message->loc message))
+                 (end-loc (compilation--message->end-loc message))
+                 (file-spec
+                  (compilation--file-struct->file-spec
+                   (compilation--loc->file-struct beg-loc))))
+            (when (string-match
+                   regexp
+                   (car file-spec))
+              (with-current-buffer source-buffer
+                (compilation-compute-loc beg-loc)
+                (when end-loc
+                  (compilation-compute-loc end-loc))
+                (push
+                 (flymake-make-diagnostic source-buffer
+                                          (compilation--loc->marker beg-loc)
+                                          (if end-loc
+                                              (compilation--loc->marker end-loc)
+                                            (1+ (compilation--loc->marker beg-loc)))
+                                          (assoc-default
+                                           (compilation--message->type message)
+                                           '((0 . :note)
+                                             (1 . :warning)
+                                             (2 . :error)))
+                                          message-text)
+                 retval))))
+          )
+        ))
+    retval))
+
+;;;###autoload
+(defmacro flymake-define-simple-backend (name
+                                         command
+                                         compilation-alist-symbols
+                                         buffer-filename-alias)
+  "Define a simple Flymake backend named NAME.
+This backend runs the COMMAND syntax tool, passes the current
+buffer contents to its standard input, and uses library
+compile.el to examine the output and look for diagnostic
+messages.
+
+BUFFER-FILENAME-ALIAS is a regular expression matching the
+filename component of those compilation messages that pertain
+exclusively to SOURCE-BUFFER.  If nil, the string \"<stdin>\" is
+used, since it is a relatively of compilers to output this string
+when checking content passed to their standard inputs.
+
+COMPILATION-ALIST-SYMBOLS is a list of symbols used to index
+`compilation-error-regexp-alist-alist' (which see) and thus
+specifying the error patterns to look for. If nil, a large (and
+slow) collection of symbols is used. It can also be a pattern"
+  `(defun ,name (report-fn &rest _args)
+     "A Flymake backend defined with
+  `flymake-define-simple-backend'."
+     (flymake--simple-backend-1 ',name
+                                ,command
+                                ,compilation-alist-symbols
+                                ,buffer-filename-alias
+                                report-fn)))
+
+(defun flymake--simple-backend-1 (name command
+                                       compilation-alist-symbols
+                                       buffer-filename-alias
+                                       report-fn)
+  "Help `flymake-define-simple-backend'."
+  (let* ((process (gethash name flymake--simple-backend-procs))
+         (source (current-buffer)))
+    (unless (executable-find (car command))
+      (error "Cannot find a suitable checker"))
+    (when (process-live-p process)
+      (kill-process process))
+    (save-restriction
+      (widen)
+      (setq
+       process
+       (puthash
+        name
+        (make-process
+         :name (symbol-name name) :noquery t :connection-type 'pipe
+         :buffer (generate-new-buffer
+                  (format " *simple backend %s*" name))
+         :command command
+         :sentinel
+         (lambda (proc _event)
+           (when (eq 'exit (process-status proc))
+             (unwind-protect
+                 (if (with-current-buffer source
+                       (eq proc
+                           (gethash name flymake--simple-backend-procs)))
+                     (with-current-buffer (process-buffer proc)
+                       (funcall
+                        report-fn
+                        (flymake-compilation-to-diagnostics
+                         source
+                         compilation-alist-symbols
+                         buffer-filename-alias)))
+                   (flymake-log :debug "Canceling obsolete check %s"
+                                proc))
+               (display-buffer (process-buffer proc))))))
+        flymake--simple-backend-procs))
+      (process-send-region process (point-min) (point-max))
+      (process-send-eof process))))
+
 (provide 'flymake)
 
 (require 'flymake-proc)



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-03 20:17 ` Three Flymake backends Was " João Távora
  2017-11-04 15:30   ` Stefan Monnier
@ 2017-11-05 12:50   ` Dmitry Gutov
  2017-11-05 12:59     ` João Távora
  1 sibling, 1 reply; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-05 12:50 UTC (permalink / raw)
  To: João Távora, Lele Gaifax
  Cc: Mark Oteiza, Stefan Monnier, emacs-devel

On 11/3/17 10:17 PM, João Távora wrote:

> I attach patches for Lele's Python backend, which would close 28808 and
> I'm also throwing in backends for Ruby and Perl.

I think you've attached two patches for Perl, and none for Ruby.

> So I'm thinking that, for master (_not_ emacs-26) we could use a
> declarative flymake-define-simple-backend macro.

Why not make it a function? From what I can see, the usage will just 
have to quote the first two arguments.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 12:50   ` Dmitry Gutov
@ 2017-11-05 12:59     ` João Távora
  2017-11-05 13:04       ` Dmitry Gutov
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-05 12:59 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 11/3/17 10:17 PM, João Távora wrote:
>
>> I attach patches for Lele's Python backend, which would close 28808 and
>> I'm also throwing in backends for Ruby and Perl.
>
> I think you've attached two patches for Perl, and none for Ruby.

Oops. But I've just pushed the correct set emacs-26 if you want to have
a look now.

>> So I'm thinking that, for master (_not_ emacs-26) we could use a
>> declarative flymake-define-simple-backend macro.
> Why not make it a function? From what I can see, the usage will just
> have to quote the first two arguments.

Because the macro defines a new function.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 12:59     ` João Távora
@ 2017-11-05 13:04       ` Dmitry Gutov
  2017-11-05 13:22         ` João Távora
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-05 13:04 UTC (permalink / raw)
  To: João Távora
  Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

On 11/5/17 2:59 PM, João Távora wrote:

> Oops. But I've just pushed the correct set emacs-26 if you want to have
> a look now.

Thanks! Now we have a backend for 'ruby -w', that's great, but Rubocop 
has become a pretty essential tool in Ruby world. For our team, at least.

So sooner or later we'll need a backend for it too, for parity with 
Flycheck.

>>> So I'm thinking that, for master (_not_ emacs-26) we could use a
>>> declarative flymake-define-simple-backend macro.
>> Why not make it a function? From what I can see, the usage will just
>> have to quote the first two arguments.
> 
> Because the macro defines a new function.

Ah, yes. It would have to use 'eval' in that case.

Although the other option, I think, is for the said function to return a 
lambda. The return value could be used with `defalias' or with 
`add-hook' directly.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 13:04       ` Dmitry Gutov
@ 2017-11-05 13:22         ` João Távora
  2017-11-05 20:14           ` Dmitry Gutov
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-05 13:22 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 11/5/17 2:59 PM, João Távora wrote:
>
>> Oops. But I've just pushed the correct set emacs-26 if you want to have
>> a look now.
>
> Thanks! Now we have a backend for 'ruby -w', that's great, but Rubocop
> has become a pretty essential tool in Ruby world. For our team, at
> least.
>
> So sooner or later we'll need a backend for it too, for parity with
> Flycheck.

I think the doors are still open for the "sooner" case :-) I don't have
rubucop, but defining new backends of the
"feed-to-stdin-then-parse-with-regexp" type is really easy (and of
really verbose, but we're trying to fix that)

>>>> So I'm thinking that, for master (_not_ emacs-26) we could use a
>>>> declarative flymake-define-simple-backend macro.
>>> Why not make it a function? From what I can see, the usage will just
>>> have to quote the first two arguments.
>>
>> Because the macro defines a new function.
>
> Ah, yes. It would have to use 'eval' in that case.
>
> Although the other option, I think, is for the said function to return
> a lambda. The return value could be used with `defalias' or with
> `add-hook' directly.

All of this is more verbose than the relatively standard way of using
"define" macros.

And add-hook for lambda's is not very nice imo because there isn't a
easy way to remove-hook afterwards.  In the future, remove-hook with a
cl-like KEY arg and a working pair of function-get/function-put could
change that though...





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 13:22         ` João Távora
@ 2017-11-05 20:14           ` Dmitry Gutov
  2017-11-05 21:05             ` João Távora
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-05 20:14 UTC (permalink / raw)
  To: João Távora
  Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

On 11/5/17 3:22 PM, João Távora wrote:

> I think the doors are still open for the "sooner" case :-) I don't have
> rubucop, but defining new backends of the
> "feed-to-stdin-then-parse-with-regexp" type is really easy (and of
> really verbose, but we're trying to fix that)

OK, I'll keep that in mind. :)

>> Although the other option, I think, is for the said function to return
>> a lambda. The return value could be used with `defalias' or with
>> `add-hook' directly.
> 
> All of this is more verbose than the relatively standard way of using
> "define" macros.
> 
> And add-hook for lambda's is not very nice imo because there isn't a
> easy way to remove-hook afterwards.

The defalias way should be all right with remove-hook, though.

I'm in the "don't create new macros unless it's _really_ beneficial" 
camp, though it might be in the minority here.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 20:14           ` Dmitry Gutov
@ 2017-11-05 21:05             ` João Távora
  2017-11-05 23:56               ` Dmitry Gutov
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-05 21:05 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 11/5/17 3:22 PM, João Távora wrote:
>
>> I think the doors are still open for the "sooner" case :-) I don't have
>> rubucop, but defining new backends of the
>> "feed-to-stdin-then-parse-with-regexp" type is really easy (and of
>> really verbose, but we're trying to fix that)
>
> OK, I'll keep that in mind. :)

FWIW I installed it and quickly hacked up this. It's the same
boilerplate again, with the tiny exception that rubocop needs the file
to be also saved for some reason (pure stdin doesn't work for some
reason).

(defvar-local rubocop--flymake-proc nil)

(defun rubocop-flymake (report-fn &rest _args)
  "Rubocop backend"
  (unless (executable-find "rubocop")
    (error "Cannot find a suitable checker"))

  (when (process-live-p rubocop--flymake-proc)
    (kill-process rubocop--flymake-proc))

  (let ((source (current-buffer)))
    (when buffer-file-name
      (save-restriction
        (widen)
        (setq
         rubocop--flymake-proc
         (make-process
          :name "rubocop-flymake" :noquery t :connection-type 'pipe
          :buffer (generate-new-buffer " *rubocop-flymake*")
          :command `("rubocop" "--stdin" ,buffer-file-name "--format" "emacs")
          :sentinel
          (lambda (proc _event)
            (when (eq 'exit (process-status proc))
              (unwind-protect
                  (if (with-current-buffer source (eq proc rubocop--flymake-proc))
                      (with-current-buffer (process-buffer proc)
                        (goto-char (point-min))
                        (cl-loop
                         while (search-forward-regexp
                                "^\\(?:.*.rb\\|-\\):\\([0-9]+\\):\\([0-9]+\\): \\(.*\\)$"
                                nil t)
                         for msg = (match-string 3)
                         for (beg . end) = (flymake-diag-region
                                            source
                                            (string-to-number (match-string 1))
                                            (string-to-number (match-string 2)))
                         for type = (if (string-match "^E: " msg)
                                        :error
                                      :note)
                         collect (flymake-make-diagnostic source
                                                          beg
                                                          end
                                                          type
                                                          msg)
                         into diags
                         finally (funcall report-fn diags)))
                    (flymake-log :debug "Canceling obsolete check %s"
                                 proc))
                (kill-buffer (process-buffer proc)))))))
        (process-send-region rubocop--flymake-proc (point-min) (point-max))
        (process-send-eof rubocop--flymake-proc)))))


> The defalias way should be all right with remove-hook, though.

Yes, and exposing the function that returns the lambda is also OK.

> I'm in the "don't create new macros unless it's _really_ beneficial"
> camp, though it might be in the minority here.

I'm with you on that camp. However macros that create functions or
defmethod, if done in the standard fashion, almost always escape that
razor.




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 21:05             ` João Távora
@ 2017-11-05 23:56               ` Dmitry Gutov
  2017-11-06  9:48                 ` João Távora
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-05 23:56 UTC (permalink / raw)
  To: João Távora
  Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

On 11/5/17 11:05 PM, João Távora wrote:

> FWIW I installed it and quickly hacked up this. It's the same
> boilerplate again, with the tiny exception that rubocop needs the file
> to be also saved for some reason (pure stdin doesn't work for some
> reason).

Thanks. Is there a place in the code where you are saving the file? I 
don't see it.

The backend seems to work fine for me in this regard, but that might be 
dependent on the version, see 
https://github.com/bbatsov/rubocop/issues/2576.

That aside, it seems like it didn't pick up the project configuration 
file. Here's what worked for me:

(defvar-local rubocop--flymake-proc nil)

(defvar rubocop-config-file ".rubocop.yml")

(defun rubocop-flymake (report-fn &rest _args)
   "Rubocop backend"
   (unless (executable-find "rubocop")
     (error "Cannot find a suitable checker"))

   (when (process-live-p rubocop--flymake-proc)
     (kill-process rubocop--flymake-proc))

   (let ((source (current-buffer))
         (command (list "rubocop" "--stdin" buffer-file-name "--format" 
"emacs"))
         config-dir)
     (when buffer-file-name
       (setq config-dir (locate-dominating-file buffer-file-name 
rubocop-config-file))
       (when config-dir
         (setq command (append command (list "--config" 
(expand-file-name rubocop-config-file
 
  config-dir)))))
       (save-restriction
         (widen)
         (setq
          rubocop--flymake-proc
          (make-process
           :name "rubocop-flymake" :noquery t :connection-type 'pipe
           :buffer (generate-new-buffer " *rubocop-flymake*")
           :command command
           :sentinel
           (lambda (proc _event)
             (when (eq 'exit (process-status proc))
               (unwind-protect
                   (if (with-current-buffer source (eq proc 
rubocop--flymake-proc))
                       (with-current-buffer (process-buffer proc)
                         (goto-char (point-min))
                         (cl-loop
                          while (search-forward-regexp
 
"^\\(?:.*.rb\\|-\\):\\([0-9]+\\):\\([0-9]+\\): \\(.*\\)$"
                                 nil t)
                          for msg = (match-string 3)
                          for (beg . end) = (flymake-diag-region
                                             source
                                             (string-to-number 
(match-string 1))
                                             (string-to-number 
(match-string 2)))
                          for type = (if (string-match "^E: " msg)
                                         :error
                                       :note)
                          collect (flymake-make-diagnostic source
                                                           beg
                                                           end
                                                           type
                                                           msg)
                          into diags
                          finally (funcall report-fn diags)))
                     (flymake-log :debug "Canceling obsolete check %s"
                                  proc))
                 (kill-buffer (process-buffer proc)))))))
         (process-send-region rubocop--flymake-proc (point-min) (point-max))
         (process-send-eof rubocop--flymake-proc)))))



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-05 23:56               ` Dmitry Gutov
@ 2017-11-06  9:48                 ` João Távora
  2017-11-06 10:35                   ` Dmitry Gutov
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-06  9:48 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 11/5/17 11:05 PM, João Távora wrote:
>
>> FWIW I installed it and quickly hacked up this. It's the same
>> boilerplate again, with the tiny exception that rubocop needs the file
>> to be also saved for some reason (pure stdin doesn't work for some
>> reason).
>
> Thanks. Is there a place in the code where you are saving the file? I
> don't see it.

No, I misexplained. Rubocop needs the buffer to be sustained by a file,
but then (mostly?) reads from stdin like it doesn't need it. It baffles
me, and possibly also flycheck.el where I learned about it. The flymake
backend just gives up if there is no buffer-file-name

> The backend seems to work fine for me in this regard, but that might
> be dependent on the version, see
> https://github.com/bbatsov/rubocop/issues/2576.

Hmm, then maybe I have an old one, I used apt install on debian.

>
> That aside, it seems like it didn't pick up the project configuration
> file. Here's what worked for me:

Hey I didn't spend more than 5 minutes with it :-)





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-06  9:48                 ` João Távora
@ 2017-11-06 10:35                   ` Dmitry Gutov
  2017-11-06 11:08                     ` João Távora
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-06 10:35 UTC (permalink / raw)
  To: João Távora
  Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

On 11/6/17 11:48 AM, João Távora wrote:

> No, I misexplained. Rubocop needs the buffer to be sustained by a file,
> but then (mostly?) reads from stdin like it doesn't need it. It baffles
> me, and possibly also flycheck.el where I learned about it. The flymake
> backend just gives up if there is no buffer-file-name

Ah yes, I've read that too.

>> The backend seems to work fine for me in this regard, but that might
>> be dependent on the version, see
>> https://github.com/bbatsov/rubocop/issues/2576.
> 
> Hmm, then maybe I have an old one, I used apt install on debian.

Probably not. Anyway, I didn't see any difference with '--cache false'.

>> That aside, it seems like it didn't pick up the project configuration
>> file. Here's what worked for me:
> 
> Hey I didn't spend more than 5 minutes with it :-)

However long, I'm glad you did. Now, the version I've sent is pretty 
much working.

Will you push it to emacs-26? Where will it live? How/when will Flymake 
choose between rubocop-flymake and ruby-flymake?



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-06 10:35                   ` Dmitry Gutov
@ 2017-11-06 11:08                     ` João Távora
  2017-11-13  0:23                       ` Dmitry Gutov
  0 siblings, 1 reply; 19+ messages in thread
From: João Távora @ 2017-11-06 11:08 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

Dmitry Gutov <dgutov@yandex.ru> writes:

> Probably not. Anyway, I didn't see any difference with '--cache false'.
>
Then probably a good idea to do what flycheck does because gremlins.

>> Hey I didn't spend more than 5 minutes with it :-)
>
> However long, I'm glad you did. Now, the version I've sent is pretty
> much working.
>
> Will you push it to emacs-26?

I think you should test it a tiny bit more and then you push it :-)

> Where will it live?

ruby-mode.el so it at least locally shares some code with ruby-flymake.

> How/when will Flymake choose between rubocop-flymake and ruby-flymake?

Flymake can use both at the same time. Just

   (add-hook 'flymake-diagnostic-functions 'ruby-flymake nil t)
   (add-hook 'flymake-diagnostic-functions 'rubocop-flymake nil t)

The user can remove-hook if he wants to.

If rubocop does everything "ruby -w" does and more, then maybe a single
backend. One that uses a ruby-flymake-use-rubocop-if-available
defcustom, and then checks for (executable-find "rubocop").



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Three Flymake backends Was Re: Two issues with the new Flymake
  2017-11-06 11:08                     ` João Távora
@ 2017-11-13  0:23                       ` Dmitry Gutov
  0 siblings, 0 replies; 19+ messages in thread
From: Dmitry Gutov @ 2017-11-13  0:23 UTC (permalink / raw)
  To: João Távora
  Cc: Mark Oteiza, Lele Gaifax, Stefan Monnier, emacs-devel

On 11/6/17 1:08 PM, João Távora wrote:

> Then probably a good idea to do what flycheck does because gremlins.

Sure.

>>> Hey I didn't spend more than 5 minutes with it :-)
>>
>> However long, I'm glad you did. Now, the version I've sent is pretty
>> much working.
>>
>> Will you push it to emacs-26?
> 
> I think you should test it a tiny bit more and then you push it :-)

It seems to be working well, but I'm getting bogged down by minor details.

Do we add defcustoms for the program name and the config file name? Do 
we really need the ruby-flymake-command defcustom? I doubt there is an 
alternative program that gives the same output as 'ruby -wc'.

>> Where will it live?
> 
> ruby-mode.el so it at least locally shares some code with ruby-flymake.

OK, code sharing will be step two.

>> How/when will Flymake choose between rubocop-flymake and ruby-flymake?
> 
> Flymake can use both at the same time. Just
> 
>     (add-hook 'flymake-diagnostic-functions 'ruby-flymake nil t)
>     (add-hook 'flymake-diagnostic-functions 'rubocop-flymake nil t)
> 
> The user can remove-hook if he wants to.
> 
> If rubocop does everything "ruby -w" does and more,

It does.

> then maybe a single
> backend. One that uses a ruby-flymake-use-rubocop-if-available
> defcustom, and then checks for (executable-find "rubocop").

Do we check for (executable-find "rubocop") once inside the major mode 
function, or every time the checker is called?

In the latter case we'll have a function ruby-flymake-auto calling one 
or the other.



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-11-13  0:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-03  9:50 Two issues with the new Flymake Lele Gaifax
2017-11-03 12:33 ` Stefan Monnier
2017-11-03 14:07   ` Lele Gaifax
2017-11-03 16:59     ` João Távora
2017-11-03 17:15       ` Stefan Monnier
2017-11-03 20:17 ` Three Flymake backends Was " João Távora
2017-11-04 15:30   ` Stefan Monnier
2017-11-04 23:17     ` João Távora
2017-11-05 12:50   ` Dmitry Gutov
2017-11-05 12:59     ` João Távora
2017-11-05 13:04       ` Dmitry Gutov
2017-11-05 13:22         ` João Távora
2017-11-05 20:14           ` Dmitry Gutov
2017-11-05 21:05             ` João Távora
2017-11-05 23:56               ` Dmitry Gutov
2017-11-06  9:48                 ` João Távora
2017-11-06 10:35                   ` Dmitry Gutov
2017-11-06 11:08                     ` João Távora
2017-11-13  0:23                       ` Dmitry Gutov

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.