From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Flymake refactored Date: Thu, 28 Sep 2017 15:52:26 -0400 Message-ID: References: <87h8vmj3tr.fsf@lolita> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1506628402 13245 195.159.176.226 (28 Sep 2017 19:53:22 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 28 Sep 2017 19:53:22 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Sep 28 21:53:18 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dxery-0002u1-Mv for ged-emacs-devel@m.gmane.org; Thu, 28 Sep 2017 21:53:15 +0200 Original-Received: from localhost ([::1]:60612 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxes6-0006CA-4f for ged-emacs-devel@m.gmane.org; Thu, 28 Sep 2017 15:53:22 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:42175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxerV-0006Bq-KA for emacs-devel@gnu.org; Thu, 28 Sep 2017 15:52:47 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dxerS-0007An-EN for emacs-devel@gnu.org; Thu, 28 Sep 2017 15:52:45 -0400 Original-Received: from [195.159.176.226] (port=41194 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dxerS-00079x-2i for emacs-devel@gnu.org; Thu, 28 Sep 2017 15:52:42 -0400 Original-Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1dxerD-0007nq-TV for emacs-devel@gnu.org; Thu, 28 Sep 2017 21:52:27 +0200 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 367 Original-X-Complaints-To: usenet@blaine.gmane.org Cancel-Lock: sha1:+3vXZIgVvfDep00kaDfhf7BBvG4= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 195.159.176.226 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:218859 Archived-At: Hi Joćo, This looks really good, thank you very much. I think this kind of functionality is very important and we should strive to polish it such that it can be enabled by default globally, via a global-flymake-mode (although I'm quite aware that we're not there yet and I don't think we need to get there before your changes are merged). I'm sorry I didn't give you feedback earlier, so here's my impressions on a fairly quick look at the overall diff. Note: Just because some are nitpicks about the specific working of a specific chunk of code doesn't mean that I've read all the code carefully (it's *really* not the case). More to the point, I'm sure I missed a lot of things. > +line, respectively. When @code{flymake-mode} is active, they are > +mapped to @kbd{M-n} and @kbd{M-p}, respectively, and by default. Right, as you guessed I don't think this is acceptable for the default (I use M-n/M-p bindings in my smerge-mode config and am pretty happy with it, but I'm not even sure I'd like it for flymake-mode because it's a mode that will be "always" enabled, contrary to smerge-mode which I only enable while there are conflicts to resolve). Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p` for it, but I'm not sure if it will work well (the problem with the `next-error` thingy is that there can be many different "kinds" or error sources, so flymake would end up competing with compile.el, grep, etc...). > +;; Copyright (C) 2017 Joćo Tįvora Of course, this should say FSF instead. > +(defun flymake-elisp--checkdoc-1 () > + "Do actual work for `flymake-elisp-checkdoc'." > + (let (collected) > + (cl-letf (((symbol-function 'checkdoc-create-error) > + (lambda (text start end &optional unfixable) > + (push (list text start end unfixable) collected) > + nil))) We should change checkdoc.el directly to avoid such hackish surgery. > +(defun flymake-elisp-checkdoc (report-fn) > + "A flymake backend for `checkdoc'. > +Calls REPORT-FN directly." > + (when (derived-mode-p 'emacs-lisp-mode) This test should not be needed. > +(defun flymake-elisp--batch-byte-compile (&optional file) [...] > + (unwind-protect > + (cl-letf (((symbol-function 'byte-compile-log-warning) > + (lambda (string &optional fill level) > + (push (list string byte-compile-last-position fill level) > + collected) > + t))) Similarly, we should change directly byte-compile-log-warning such that this kind of surgery is not needed. > + (add-to-list (make-local-variable 'flymake-diagnostic-functions) > + 'flymake-elisp-checkdoc t) > + (add-to-list (make-local-variable 'flymake-diagnostic-functions) > + 'flymake-elisp-byte-compile t)) `flymake-diagnostic-functions` should be a(n abnormal) hook which we manipulate with `add-hook`: (add-hook 'flymake-diagnostic-functions #'flymake-elisp-checkdoc nil t) (add-hook 'flymake-diagnostic-functions #'flymake-elisp-byte-compile nil t) > +(add-hook 'emacs-lisp-mode-hook > + 'flymake-elisp-setup-backends) You should change elisp-mode.el directly instead. Actually I'd argue that the contents of flymake-elisp.el should move to elisp-mode.el. This also means remove the (require 'flymake-ui) and instead mark `flymake-make-diagnostic` as autoloaded. > +(defcustom flymake-proc-allowed-file-name-masks > + '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" > + flymake-proc-simple-make-init > + nil > + flymake-proc-real-file-name-considering-includes) > + ("\\.xml\\'" flymake-proc-xml-init) > + ("\\.html?\\'" flymake-proc-xml-init) > + ("\\.cs\\'" flymake-proc-simple-make-init) > + ("\\.p[ml]\\'" flymake-proc-perl-init) > + ("\\.php[345]?\\'" flymake-proc-php-init) > + ("\\.h\\'" flymake-proc-master-make-header-init flymake-proc-master-cleanup) > + ("\\.java\\'" flymake-proc-simple-make-java-init flymake-proc-simple-java-cleanup) > + ("[0-9]+\\.tex\\'" flymake-proc-master-tex-init flymake-proc-master-cleanup) > + ("\\.tex\\'" flymake-proc-simple-tex-init) > + ("\\.idl\\'" flymake-proc-simple-make-init) > ;; ("\\.cpp\\'" 1) > ;; ("\\.java\\'" 3) > ;; ("\\.h\\'" 2 ("\\.cpp\\'" "\\.c\\'") This smells of legacy: just as we do for Elisp, it should be the major mode which specifies how to get the diagnostics, so we don't need to care about filenames. So this should be marked as obsolete. > +(add-to-list 'flymake-diagnostic-functions > + 'flymake-proc-legacy-flymake) Should also be (add-hook 'flymake-diagnostic-functions #'flymake-proc-legacy-flymake) > +(progn > + (define-obsolete-variable-alias [...] There are too many obsolete aliases in there, IMO. We should think a bit more about each one of them. Many of them alias `flymake-foo` to `flymake-proc--bar`, which doesn't make much sense: if `flymake-proc--bar` is used outside of flymake-proc (via some old name), then it probably shouldn't have "--" in its name. If flymake-proc.el is considered legacy that should disappear in the long run, then I'm not sure all that renaming is justified (we can keep using the old names until we get rid of them altogether). Also some parts of flymake-proc seem to make sense for "all" backends rather than only for flymake-proc. Some concrete examples: > + (define-obsolete-variable-alias 'flymake-compilation-prevents-syntax-check > + 'flymake-proc-compilation-prevents-syntax-check "26.1" > + "If non-nil, don't start syntax check if compilation is running.") This doesn't look specific to the specific flymake-proc backend, so maybe it should stay in flymake(-ui).el > + (define-obsolete-variable-alias 'flymake-xml-program > + 'flymake-proc-xml-program "26.1" > + "Program to use for XML validation.") In the long run, this should likely move to some xml major mode, so `flymake-proc-xml-program` is probably not the "right" name for it. So I think we may as well keep using `flymake-xml-program` for now, until it's moved to some other package. > + (define-obsolete-variable-alias 'flymake-allowed-file-name-masks > + 'flymake-proc-allowed-file-name-masks "26.1" Since I argued above that flymake-proc-allowed-file-name-masks should be obsolete, there's not much point renaming flymake-allowed-file-name-masks to flymake-proc-allowed-file-name-masks. > + :group 'flymake Inside flymake(-ui).el, those `:group 'flymake` are redundant. > +(defun flymake-make-diagnostic (buffer > + beg > + end > + type > + text) > + "Mark BUFFER's region from BEG to END with a flymake diagnostic. AFAIK this function does not really "mark". It just creates a diagnostic object which will/may cause the corresponding region to be marked at some later time. > +TYPE is a key to `flymake-diagnostic-types-alist' and TEXT is a > +description of the problem detected in this region." > + (flymake--diag-make :buffer buffer :beg beg :end end :type type :text text)) You could get almost the same function (and completely skip the definition of flymake--diag-make) with: (:constructor flymake-make-diagnostic (buffer beg end type text)) Maybe we should extend cl-defstruct so we can give it docstrings for :constructors, since I suspect that the desire to provide a proper docstring was the main motivation for not using a :constructor here. > + (cl-remove-if-not > + (lambda (ov) > + (and (overlay-get ov 'flymake-overlay) `ov` is by definition an overlay, so using property name `flymake-overlay` sounds redundant: just `flymake` would do just fine. (actually it's a bit worse: the property name `flymake-overlay` makes it sound like its value will be an overlay, whereas it's just t). > + (or (not filter) > + (cond ((functionp filter) (funcall filter ov)) > + ((symbolp filter) (overlay-get ov filter)))))) > + (save-restriction > + (widen) > + (let ((ovs (if (and beg (null end)) > + (overlays-at beg t) > + (overlays-in (or beg (point-min)) > + (or end (point-max)))))) > + (if compare > + (cl-sort ovs compare :key (or key > + #'identity)) > + ovs))))) You probably should `cl-remove-if-not` before doing to `cl-sort`. Both for performance reasons (fewer overlays to sort) and so that your `compare` and `key` functions don't have to deal with non-flymake overlays. > +* A (possibly empty) list of objects created with > + `flymake-make-diagnostic', causing flymake to annotate the > + buffer with this information and consider the backend has > + having finished its check normally. The docstring should say if it's OK for the backend to call REPORT-FN several times with such diagnostics (so it can start a background process and call the REPORT-FN every time it gets a chunk of reports from the background process), or whether it should instead wait until it has all the diagnostics before calling REPORT-FN. > +* The symbol `:progress', signalling that the backend is still > + working and will call REPORT-FN again in the future. This description leaves me wondering why a backend would ever want to do that. What kind of effect does it have on the UI? > +* The symbol `:panic', signalling that the backend has > + encountered an exceptional situation and should be disabled. > + > +In the latter cases, it is also possible to provide REPORT-FN > +with a string as the keyword argument `:explanation'. The string > +should give human-readable details of the situation.") I don't understand this last paragraph. AFAICT from this docstring, REPORT-FN will be a function which takes a single argument, so I don't know what "provide REPORT-FN with a string as the keyword argument `:explanation'" means. Does it mean something like (funcall REPORT-FN `(:explanation ,str)) Maybe just say that the third option is to pass to REPORT-FN a value of the form `(:panic . EXPLANATION)` where EXPLANATION is a string. [ Note: I don't like using &key for functions, so I recommend not to use it for API interfaces such as flymake-diagnostic-functions, although I don't object to using them internally in flymake if you so prefer. ] Regarding flymake-diagnostic-functions I've been wondering about its behavior for very large buffers. More specifically I've been wondering whether it would make sense to: A- provide to the backend function the BEG/END of the region changed since the last time we called it. B- make it possible to focus the work on the currently displayed part of the buffer. But I guess it's not worth the effort: most/all backends won't be able to make any use of that information anyway. > +(defvar flymake-diagnostic-types-alist > + `((:error > + . ((category . flymake-error) > + (mode-line-face . compilation-error))) > + (:warning > + . ((category . flymake-warning) > + (mode-line-face . compilation-warning))) > + (:note > + . ((category . flymake-note) > + (mode-line-face . compilation-info)))) This mapping from diag-type to properties seems redundant with the one from category symbol to properties. I'm no friend of the `category` property, so I'd recommend you use (defvar flymake-diagnostic-types-alist `((:error (face . flymake-error) (bitmap . flymake-error-bitmap) (severity . ,(warning-numeric-level :error)) (mode-line-face . compilation-error)) ...)) but if you prefer using `category`, you can just use (intern (format "flymake-category-%s" diag-type)) and then put all the properties on those symbols. > + (dolist (backend flymake-diagnostic-functions) In order to properly handle flymake-diagnostic-functions as a hook (e.g. handle the t case), you probably want to use: (run-hook-wrapped 'flymake-diagnostic-functions (lambda (backend) (cond ((memq backend flymake--running-backends) (flymake-log 2 "Backend %s still running, not restarting" backend)) ((memq backend flymake--disabled-backends) (flymake-log 2 "Backend %s is disabled, not starting" backend)) (t (flymake--run-backend backend))) nil)) > (defun flymake-mode-on () > "Turn flymake mode on." > (flymake-mode 1) > - (flymake-log 1 "flymake mode turned ON for buffer %s" (buffer-name))) > + (flymake-log 1 "flymake mode turned ON")) We should make this an obsolete alias of `flymake-mode`. > -;;;###autoload > (defun flymake-mode-off () > "Turn flymake mode off." > (flymake-mode 0) > - (flymake-log 1 "flymake mode turned OFF for buffer %s" (buffer-name))) > + (flymake-log 1 "flymake mode turned OFF")) I'd make this obsolete (for those rare cases where it's needed, you can just as well call (flymake-mode -1)). > + (reported (hash-table-keys flymake--diagnostics-table))) AFAICT you only use `reported` as a boolean, so it would be more efficient to use (reported (> (hash-table-count flymake--diagnostics-table) 0)) [ I noticed this because of `hash-table-keys`: almost every time it's used, it's an inefficient way to solve the problem, in my experience ;-) ] > +;;; Dummy autoloads ensure that this file gets autoloaded, not just > +;;; flymake-ui.el where they actually live. > + > +;;;###autoload > +(defun flymake-mode-on () "Turn flymake mode on." nil) > + > +;;;###autoload > +(defun flymake-mode-off () "Turn flymake mode off." nil) > + > +;;;###autoload > +(define-minor-mode flymake-mode nil) Yuck! > +(require 'flymake-ui) > +(require 'flymake-proc) > +(require 'flymake-elisp) `flymake-elisp.el` should not be loaded as soon as you enable flymake-mode, since we may never use Elisp in that session: the dependency goes in the wrong direction. `flymake-ui.el` should be renamed flymake.el (which will solve the above yuckiness). Admittedly, this will cause problems with mutual dependencies between flymake.el and flymake-proc.el, so it'll take some effort to solve these issues. I think the best solution is to make it so flymake.el doesn't (require 'flymake-proc). It will require moving some stuff from flymake-proc.el to flymake.el (e.g. all the backward compatibility APIs), but I think the result may be even cleaner. Stefan