From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: joaotavora@gmail.com (=?utf-8?B?Sm/Do28gVMOhdm9yYQ==?=) Newsgroups: gmane.emacs.devel Subject: Re: Flymake refactored Date: Fri, 29 Sep 2017 01:22:20 +0100 Message-ID: <87y3oygxqb.fsf@lolita> References: <87h8vmj3tr.fsf@lolita> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1506644597 11068 195.159.176.226 (29 Sep 2017 00:23:17 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 29 Sep 2017 00:23:17 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Sep 29 02:23:10 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 1dxj5B-0002Iv-BN for ged-emacs-devel@m.gmane.org; Fri, 29 Sep 2017 02:23:09 +0200 Original-Received: from localhost ([::1]:33133 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxj5I-0002hF-M3 for ged-emacs-devel@m.gmane.org; Thu, 28 Sep 2017 20:23:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:40424) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dxj4Y-0002gy-Nn for emacs-devel@gnu.org; Thu, 28 Sep 2017 20:22:33 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dxj4V-0005ae-8t for emacs-devel@gnu.org; Thu, 28 Sep 2017 20:22:30 -0400 Original-Received: from mail-wr0-x22a.google.com ([2a00:1450:400c:c0c::22a]:51124) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dxj4U-0005aC-Up for emacs-devel@gnu.org; Thu, 28 Sep 2017 20:22:27 -0400 Original-Received: by mail-wr0-x22a.google.com with SMTP id b21so5524731wrg.7 for ; Thu, 28 Sep 2017 17:22:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=avgEu1Ia+o8ljd6syGnpIjKONOYwmJarmJh8s5GCY3c=; b=a0TLdSZu9gAQ3j1hAtfU8Djc6/AEpmdTeXIjVIiErvo6eKf/u5iww/JnwzsLHzKIpZ VSy8Swzs3HvIK/FB0jfVAo49cAQ4EsnsYO5APJYAO3U2DJE9tJwKPJDO0aHOXC/zz/6a 3hQJC+Ei+jMq/vwtAJ7C9IU4aLt6e515ygcvCb2RnY6tGpTXZs+iB8WquUv0iXsjBZoZ wqhLLOkqqruxx68H5LIvpum6nQqEpaQSZ+Yr9Gdazr9yWJdNXipInlu0xFB2SlG+zbDy lhtVsTJegZRYRNGvn8Bi+Bn+sbGBdHfcOp88z9Ai7vyFe4sS0aX6yIhwcMOMQnsRrVWN oRaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=avgEu1Ia+o8ljd6syGnpIjKONOYwmJarmJh8s5GCY3c=; b=UPvOswFb4DYz6uuDP1pPoVdz5YjMSiqCQZXB6ZhPlvv8YeDzYkrBrDVmWyxT3eAcUY BRRB767lwxOYXFIH63+835AhFmdxW/pP3Y9eT4Y5Y44n1Hl79ynI9VfFXhjURwXLSEKJ aBp/mOUKwtH7HzSqfdku/QOY9o0V1GQG7RVAANnypfDPloEHbef5NYVc3dCqCeZN9Rqp i/+6PI/9d+nxVenq/P5XI5hdd0jP1tGK7O9aftMZn+H1YpdgpVo23YZXqAxnDYayDJZs pyWR79fFbygXvgbSPNVEnwgDi6KHqxiM+//vt67v4JyMdlbzaCZGCVe5LNGKETIm9gS2 hG8g== X-Gm-Message-State: AHPjjUgKCAsBT5CAgSV0ysZAUnA+vsvOcwBZgmERnyzO42z58dYixTxk EzcXZ1m/ZSipSSDa6AfCXoGYRaR+ X-Google-Smtp-Source: AOwi7QDJMCgd2lIoS0WHmjJsbBDtestgiF6MBuib0lpa1+tpjuidNyJcnPiBqyG4Oi/qvA8MbhtTFA== X-Received: by 10.223.186.6 with SMTP id o6mr5354065wrg.263.1506644543827; Thu, 28 Sep 2017 17:22:23 -0700 (PDT) Original-Received: from lolita.yourcompany.com (188.139.62.94.rev.vodafone.pt. [94.62.139.188]) by smtp.gmail.com with ESMTPSA id q188sm1243512wmb.43.2017.09.28.17.22.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 28 Sep 2017 17:22:23 -0700 (PDT) In-Reply-To: (Stefan Monnier's message of "Thu, 28 Sep 2017 15:52:26 -0400") X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c0c::22a 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:218875 Archived-At: Hi Stefan, > This looks really good, thank you very much. I think this kind of > functionality is very important and we should strive to polish it Thanks, and thanks very much for such a speedy review. Before I start replying to it, I just wanted to say that I'll have to switch context again away from hacking very soon, so I wanted to get the most out of these early review cycles At the end of this mail, I've summarized a list of the changes you suggested, from less to more controversial. If you see something trivial that you can fix right away, or something less trivial but uncontroversial, just push it to the branch. > I'm sorry I didn't give you feedback earlier, so here's my impressions > on a fairly quick look at the overall diff. No problem. I've only picked it up the last week or so. It was sitting pretty quiet in the scratch branch in a state that didn't really merit reviewing. From my perspective, 6 hours between asking and getting a decent review is pretty good. > Right, as you guessed I don't think this is acceptable for the default > (I use M-n/M-p bindings in my smerge-mode config and am pretty happy Well, in your case I would like to use M-n/M-p for flymake and overriden vwhen smerge-mode kicks in, which is a much more serious kind of conflict. When smerge-mode turns itself off again (it doesn't lately, but it used to), you get flymake again. OTOH, perhaps it's a good thing they are empty. They should be reserved for the user IMO. > Maybe we could hook it into `next-error` so you can use C-x ` or `M-g n/p` > for it, but I'm not sure if it will work well (the problem with the > `next-error` thingy is that there can be many different "kinds" or > error sources, so flymake would end up competing with compile.el, grep, > etc...). Agree. Let's not do anything :-). I didn't mention it, but you can navigate through errors with the mouse wheel on the little red/yellow indicators. >> +;; Copyright (C) 2017 Jo=C3=A3o T=C3=A1vora > > Of course, this should say FSF instead. :-) > We should change checkdoc.el directly to avoid such hackish surgery. > ... > Similarly, we should change directly byte-compile-log-warning such that > tnhis kind of surgery is not needed. Sure, no problem, i was in a hurry and didn't want to touch more files. (...but didn't you use to be the advocate for add-function everywhere, as in add-function versus hooks? Isn't this slightly better?) >> +(defun flymake-elisp-checkdoc (report-fn) >> + "A flymake backend for `checkdoc'. >> +Calls REPORT-FN directly." >> + (when (derived-mode-p 'emacs-lisp-mode) > > This test should not be needed. ..if flymake-elisp-checkdoc is always placed in the correct buffer-local value of flymake-diagnostic-functions. IDK, is it a good idea to admit that (lazy) users place everything in the global hook? Inn which case it should actually error, which is a synchronous form of panic that causes the UI to disable it for the buffer. >> +(defun flymake-elisp--batch-byte-compile (&optional file) > [...] >> + (unwind-protect >> + (cl-letf (((symbol-function 'byte-compile-log-warning) >> + (lambda (string &optional fill level) >> + (push (list string byte-compile-last-position fill= level) >> + collected) >> + t))) > > >> + (add-to-list (make-local-variable 'flymake-diagnostic-functions) >> + 'flymake-elisp-checkdoc t) >> + (add-to-list (make-local-variable 'flymake-diagnostic-functions) >> + 'flymake-elisp-byte-compile t)) > > `flymake-diagnostic-functions` should be a(n abnormal) hook which we > manipulate with `add-hook`: > > (add-hook 'flymake-diagnostic-functions #'flymake-elisp-checkdoc nil = t) > (add-hook 'flymake-diagnostic-functions > #'flymake-elisp-byte-compile nil t) No problem. I actually wanted a hook, but no run-hook-* function had the the proper semantics. And I didn't want to write the logic for the 't' and everything. But now I notice run-hook-wrapped should do a nice job, rig= ht? >> +(add-hook 'emacs-lisp-mode-hook >> + 'flymake-elisp-setup-backends) > You should change elisp-mode.el directly instead. OK. > Actually I'd argue that the contents of flymake-elisp.el should move > to elisp-mode.el. OK. > This also means remove the (require 'flymake-ui) > and instead mark `flymake-make-diagnostic` as autoloaded. Hmmm. If I read your reasoning correctly, you want to avoid elisp-mode.el knowing about flymake and also avoiding a compilation warning. But isn't having flymake-make-diagnostic and the hook already "knowing" about flymake)? Isn't (eval-when-compile (require 'flymake-ui)) better? But maybe I don't read you correctly and I'm just confused. >> +(defcustom flymake-proc-allowed-file-name-masks >> + '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" >> + flymake-proc-simple-make-init >> + nil >> + flymake-proc-real-file-name-considering-includes) >> + ("\\.xml\\'" flymake-proc-xml-init) >> + ("\\.html?\\'" flymake-proc-xml-init) >> + ("\\.cs\\'" flymake-proc-simple-make-init) >> + ("\\.p[ml]\\'" flymake-proc-perl-init) >> + ("\\.php[345]?\\'" flymake-proc-php-init) >> + ("\\.h\\'" flymake-proc-master-make-header-init flymake-proc-master= -cleanup) >> + ("\\.java\\'" flymake-proc-simple-make-java-init flymake-proc-simpl= e-java-cleanup) >> + ("[0-9]+\\.tex\\'" flymake-proc-master-tex-init flymake-proc-master= -cleanup) >> + ("\\.tex\\'" flymake-proc-simple-tex-init) >> + ("\\.idl\\'" flymake-proc-simple-make-init) >> ;; ("\\.cpp\\'" 1) >> ;; ("\\.java\\'" 3) >> ;; ("\\.h\\'" 2 ("\\.cpp\\'" "\\.c\\'") > > This smells of legacy: just as we do for Elisp, it should be the major > mode which specifies how to get the diagnostics, so we don't need to > care about filenames. > So this should be marked as obsolete. Sure, but you're assuming a correspondence between the cars of those and major modes. And to be honest that really sounds like boring work. My plan was to leave flymake-proc.el quiet in a dark corner and flymake-proc-legacy-flymake in the global hook until we write proper backends for a sufficient number of modes. >> +(progn >> + (define-obsolete-variable-alias > [...] > > There are too many obsolete aliases in there, IMO. > We should think a bit more about each one of them. Oh no :-) > > Many of them alias `flymake-foo` to `flymake-proc--bar`, which doesn't > make much sense: if `flymake-proc--bar` is used outside of flymake-proc > (via some old name), then it probably shouldn't have "--" in its name. Makes a lot of sense, I removed them. When I started I too obsessed with compatibility so you might have guessed I started by making one alias for every symbol indiscriminately, marked it internal for good measure. Then I hand picked some that looked like they could be used outside. Now I consider flymake-easy.el working minimally to be sufficient backward compatibility. > If flymake-proc.el is considered legacy that should disappear in the > long run, then I'm not sure all that renaming is justified (we can keep > using the old names until we get rid of them altogether). I don't agree, I like the -proc prefix to remind me not to touch it. > Also some parts of flymake-proc seem to make sense for "all" backends > rather than only for flymake-proc. Sure, they should be extracted into some flymake-util.el or flymake-common.el or just flymake.el. But they could just be rewritten. And fixed: the "master" mechanism (good old "master" naming eh), for example, is broken. It cannot understand that C++ .tpp files are included from .hpp included from .cpp. Don't get me wrong, legacy flymake has been immensely useful for me, but again I think it should stay put until we have a better alternative. I think we could handle a bit of duplication here. > Some concrete examples: > >> + (define-obsolete-variable-alias 'flymake-compilation-prevents-syntax-= check >> + 'flymake-proc-compilation-prevents-syntax-check "26.1" >> + "If non-nil, don't start syntax check if compilation is running.") > > This doesn't look specific to the specific flymake-proc backend, so > maybe it should stay in flymake(-ui).el IDK, there's no real conflict AFAIK, it's just a convenience. And M-x compile is for external processes just like flymake-proc.el. But OK. >> + (define-obsolete-variable-alias 'flymake-xml-program >> + 'flymake-proc-xml-program "26.1" >> + "Program to use for XML validation.") > > In the long run, this should likely move to some xml major mode, so > `flymake-proc-xml-program` is probably not the "right" name for it. > So I think we may as well keep using `flymake-xml-program` for now, > until it's moved to some other package. No, let's write a good backend for xml-mode/sgml-modes instead. There are probably super cool xml linters out there, some perhaps in elisp (nxml-mode of James Clark fame for one) >> + :group 'flymake > > Inside flymake(-ui).el, those `:group 'flymake` are redundant. A nitpick indeed :-). Fixed. >> +(defun flymake-make-diagnostic (buffer >> + beg >> + end >> + type >> + text) >> + "Mark BUFFER's region from BEG to END with a flymake diagnostic. > > AFAIK this function does not really "mark". It just creates It used to. Fixed the docstring. > >> +TYPE is a key to `flymake-diagnostic-types-alist' and TEXT is a >> +description of the problem detected in this region." >> + (flymake--diag-make :buffer buffer :beg beg :end end :type type :text= text)) > > You could get almost the same function (and completely skip the > definition of flymake--diag-make) with: > > (:constructor flymake-make-diagnostic (buffer beg end type > text)) > Maybe we should extend cl-defstruct so we can give it docstrings > for :constructors, since I suspect that the desire to provide a proper > docstring was the main motivation for not using a :constructor here. Also that, but actually I kept that indirection to remind me that I probably want to create different objects for different types of objects in the future. But then didn't go that way.=20 >> + (cl-remove-if-not >> + (lambda (ov) >> + (and (overlay-get ov 'flymake-overlay) > > `ov` is by definition an overlay, so using property name > `flymake-overlay` sounds redundant: just `flymake` would do just fine. > (actually it's a bit worse: the property name `flymake-overlay` makes it > sound like its value will be an overlay, whereas it's just t). Not just "a bit worse", Stefan: a truly horrible crime. Fixed. > You probably should `cl-remove-if-not` before doing to `cl-sort`. > Both for performance reasons (fewer overlays to sort) and so that your > `compare` and `key` functions don't have to deal with > non-flymake overlays. Well spotted. Fixed. > The docstring should say if it's OK for the backend to call REPORT-FN > several times with such diagnostics (so it can start a background > process and call the REPORT-FN every time it gets a chunk of reports > from the background process), or whether it should instead wait until it > has all the diagnostics before calling REPORT-FN. Indeed. I think it should be OK to do so, but currently it isn't. Something to think about. Since each REPORT-FN is a different lambda we could use that fact to track if a calling it "too much". >> +* The symbol `:progress', signalling that the backend is still >> + working and will call REPORT-FN again in the future. > > This description leaves me wondering why a backend would ever want to > do that. What kind of effect does it have on the UI? Nothing for the moment. But consider in the future that the UI wants to know if it should wait for a backend that is taking too long to report. This could be its keepalive. Or maybe your idea of calling REPORT-FN multiple times takes care of it. > > I don't understand this last paragraph. AFAICT from this docstring, > REPORT-FN will be a function which takes a single argument, so I don't > know what "provide REPORT-FN with a string as the keyword argument > `:explanation'" means. Does it mean something like > > (funcall REPORT-FN `(:explanation ,str)) No it means (funcall REPORT-FN (nth (random 2)'(:panic :progress)) :explanation "this")= =20 > Maybe just say that the third option is to pass to REPORT-FN a value of > the form `(:panic . EXPLANATION)` where EXPLANATION is a string. > [ Note: I don't like using &key for functions, so I recommend not to > use it for API interfaces such as flymake-diagnostic-functions, > although I don't object to using them internally in flymake if you so > prefer. ] The first argument to REPORT-FN can be a list or a non-nil symbol. In the latter case keywords are accepted. Actually :explanation is always accepted, as is :force. I just forgot to describe it. Probably other keywords will come in handy. Keywords are good, they're free destructuring, better than pcase'ing funky cons (much as I respect pcase :-). > Regarding flymake-diagnostic-functions I've been wondering about its > behavior for very large buffers. More specifically I've been wondering > whether it would make sense to: > A- provide to the backend function the BEG/END of the region changed > since the last time we called it. > B- make it possible to focus the work on the currently displayed part of > the buffer. > But I guess it's not worth the effort: most/all backends won't be able > to make any use of that information anyway. This makes sense. The backend is called in the buffer so it has B. I was also very much thinking of making A, by collecting regions in flymake-after-change-function, and storing them in a buffer-local variable, like flymake-check-start-time or flymake-last-change-time. Or perhaps dynamically binding it around the backend call. Or just passing them as arguments. >> +(defvar flymake-diagnostic-types-alist >> + `((:error >> + . ((category . flymake-error) >> + (mode-line-face . compilation-error))) >> + (:warning >> + . ((category . flymake-warning) >> + (mode-line-face . compilation-warning))) >> + (:note >> + . ((category . flymake-note) >> + (mode-line-face . compilation-info)))) > > This mapping from diag-type to properties seems redundant with the one > from category symbol to properties. I'm no friend of the `category` > property, so I'd recommend you use > > (defvar flymake-diagnostic-types-alist > `((:error (face . flymake-error) > (bitmap . flymake-error-bitmap) > (severity . ,(warning-numeric-level :error)) > (mode-line-face . compilation-error)) > ...)) How would I mixin existing stuff for a supposed :my-error that is just like :error, but overrides some properties. > but if you prefer using `category`, you can just use > (intern (format "flymake-category-%s" diag-type)) and then put all the > properties on those symbols. OK. Will do. >> + (dolist (backend flymake-diagnostic-functions) > > In order to properly handle flymake-diagnostic-functions as a hook > (e.g. handle the t case), you probably want to use: > > (run-hook-wrapped 'flymake-diagnostic-functions > (lambda (backend) > (cond ((memq backend flymake--running-backends) > (flymake-log 2 "Backend %s still running, not restarting" > backend)) > ((memq backend flymake--disabled-backends) > (flymake-log 2 "Backend %s is disabled, not starting" > backend)) > (t > (flymake--run-backend backend))) > nil)) That's it (I had seen run-hook-wrapped in the meantime). Thanks! >> (defun flymake-mode-on () >> "Turn flymake mode on." >> (flymake-mode 1) >> - (flymake-log 1 "flymake mode turned ON for buffer %s" (buffer-name))) >> + (flymake-log 1 "flymake mode turned ON")) > > We should make this an obsolete alias of `flymake-mode`. OK. >> -;;;###autoload >> (defun flymake-mode-off () >> "Turn flymake mode off." >> (flymake-mode 0) >> - (flymake-log 1 "flymake mode turned OFF for buffer %s" (buffer-name))) >> + (flymake-log 1 "flymake mode turned OFF")) > > I'd make this obsolete (for those rare cases where it's needed, you can > just as well call (flymake-mode -1)). OK. >> + (reported (hash-table-keys flymake--diagnostics-table))) > > AFAICT you only use `reported` as a boolean, so it would be more > efficient to use > > (reported (> (hash-table-count flymake--diagnostics-table) 0)) > > [ I noticed this because of `hash-table-keys`: almost every time it's > used, it's an inefficient way to solve the problem, in my > experience ;-) ] Actually, I was thinking of making cl-set-difference assertions between that and other sets in the future. hash-table-keys seems a nice way. >> +;;; Dummy autoloads ensure that this file gets autoloaded, not just >> +;;; flymake-ui.el where they actually live. >> >> +;;;###autoload >> +(define-minor-mode flymake-mode nil) > > Yuck! Yes, kinda horrible. I was in a hurry. >> +(require 'flymake-ui) >> +(require 'flymake-proc) >> +(require 'flymake-elisp) > > `flymake-elisp.el` should not be loaded as soon as you enable > flymake-mode, since we may never use Elisp in that session: the > dependency goes in the wrong direction. OK. > `flymake-ui.el` should be renamed flymake.el (which will solve the > above yuckiness). Admittedly, this will cause problems with mutual > dependencies between flymake.el and flymake-proc.el, so it'll take some > effort to solve these issues. Perhaps solved by making the function flymake-proc-legacy-flymake, the variable flymake-proc-err-line-patterns, and some others autoloaded? > I think the best solution is to make it so flymake.el doesn't (require > 'flymake-proc). It will require moving some stuff from flymake-proc.el > to flymake.el (e.g. all the backward compatibility APIs), but I think > the result may be even cleaner. I think that's my idea from the previous paragraph too. OK. To summarize: Already fixed: * Scrap the M-n, M-p commit. * Delete many obsolete aliases. * :group in defcustom * flymake-make-diagnostic docstring * overlay property is just 'flymake * cl-remove-if-not and cl-sort Will fix ASAP: * Move flymake-compilation-prevents-syntax-check to flymake-ui.el * flymake.el autoload yuckiness. * Make a flymake-category. * Make flymake-diagnostic-functions a hook. Use run-hook-wrapped. * Obsolete alias for flymake-mode-on/off * Don't use hash-table-keys, unless I really need it. Will fix, but harder: * proper checkdoc.el interface * proper byte-compile.el interface * Move flymake-elisp.el contents to elisp-mode.el * Allow REPORT-FN to be called multiple times from backends * Passing recent changes to backends * Rename flymake-ui.el to flymake.el and make some autoloads. Have doubts: * remove starting test in flymake-elisp-checkdoc * Use defstruct constructor for flymake-make-diagnostic * A replacement for &key in REPORT-FN's lambda list. Don't like: * removing category in flymake-diagnostic-types-alist * mark flymake-proc-err-line-patterns obsolete and add to some other variable from across emacs progmodes. * remove the -proc prefix from defs in the proc backend. * reconsider parts from flymake-proc into an util package. No effect: * Copyright FSF in the header Thanks, Jo=C3=A3o