From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#44864: 27.1; advice.el: ad-with-originals deprecated, but no advice on replacement Date: Thu, 26 Nov 2020 12:54:51 -0500 Message-ID: References: <20201126170912.GA680140@d-and-j.net> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8426"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: 44864@debbugs.gnu.org To: Julian Gilbey Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Nov 26 18:57:52 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kiLWh-00021w-Qs for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 26 Nov 2020 18:57:52 +0100 Original-Received: from localhost ([::1]:49296 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kiLWg-0006VG-TZ for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 26 Nov 2020 12:57:50 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44314) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kiLUw-0005DY-2k for bug-gnu-emacs@gnu.org; Thu, 26 Nov 2020 12:56:05 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:59495) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kiLUv-0001DP-Rz for bug-gnu-emacs@gnu.org; Thu, 26 Nov 2020 12:56:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kiLUv-0001VU-R4 for bug-gnu-emacs@gnu.org; Thu, 26 Nov 2020 12:56:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 26 Nov 2020 17:56:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 44864 X-GNU-PR-Package: emacs Original-Received: via spool by 44864-submit@debbugs.gnu.org id=B44864.16064133035705 (code B ref 44864); Thu, 26 Nov 2020 17:56:01 +0000 Original-Received: (at 44864) by debbugs.gnu.org; 26 Nov 2020 17:55:03 +0000 Original-Received: from localhost ([127.0.0.1]:42806 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kiLTy-0001Th-KD for submit@debbugs.gnu.org; Thu, 26 Nov 2020 12:55:03 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:23350) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kiLTw-0001T5-Am for 44864@debbugs.gnu.org; Thu, 26 Nov 2020 12:55:01 -0500 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 7EB8010025B; Thu, 26 Nov 2020 12:54:54 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 33ECE100216; Thu, 26 Nov 2020 12:54:52 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1606413292; bh=ShxMXrZiZxecFMZLhAcVK/ZVsc95vVGjw/NGhXTSu0k=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=JnoHvQkUfazZG40EPpeX5UsshaVSFXFuTrN68N2BUju6bXnRb47hp7FhKFPIeUpl5 vqsnyaT51tptkN1FezTk8LSG+Yqs7qRe5DVmKzbPcit32LW/JRtRgEXIQSAD1czPOy HSMdlpkz9r/pNmolwxootJs3TLOG/TNnWc4ezQfYUsnYe1qYVCCEzdB/HwC9xCavaE hXiqXemLzCnGiB30mcqzJmWHumMwTf9UNNFa55Yapa96+YszUKwWGy1FMWJ8Gyjss5 sguyiNCdyour7gK7Vv9flGzBj9tGV5lgwEvKkk3oMEDKbA5UuOPbc0E+C9t704oieW VkVFJldPWsWsg== Original-Received: from alfajor (69-165-136-52.dsl.teksavvy.com [69.165.136.52]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id F31B812032C; Thu, 26 Nov 2020 12:54:51 -0500 (EST) In-Reply-To: <20201126170912.GA680140@d-and-j.net> (Julian Gilbey's message of "Thu, 26 Nov 2020 17:09:12 +0000") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:194363 Archived-At: > That sounds eminently sensible. Scouring the rest of Dave's file, I > found the following comment at the beginning of the function you've > just quoted from: Hmm... seems relevant indeed. > (defun multi-install-mode (mode &optional chunk-fn base) > "Add MODE to the multiple major modes supported by the current buffer. > CHUNK-FN, if non-nil, is a function to select the mode of a chunk, > added to the list `multi-chunk-fns'. BASE non-nil means that this > is the base mode." > (unless (memq mode multi-indirect-buffers-alist) ; be idempotent > ;; This is part of a grim hack for lossage in AUCTeX, which > ;; bogusly advises `hack-one-local-variable'. This loses, due to > ;; the way advice works, when we run `multi-hack-local-variables' > ;; below -- there ought to be a way round this, probably with CL's > ;; flet. Any subsequent use of it then fails because advice has > ;; captured the now-unbound variable `late-hack'... Thus ensure > ;; we've loaded the mode in advance to get any autoloads sorted > ;; out. Do it generally in case other modes have similar > ;; problems. [The AUCTeX stuff is in support of an undocumented > ;; feature which is unnecessary and, anyway, wouldn't need advice > ;; to implement. Unfortunately the maintainer seems not to > ;; understand the local variables mechanism and wouldn't remove > ;; this. To invoke minor modes, you should just use `mode:' in > ;; `local variables'.] I'm not sure I understand the details of the problem described, but I think I see the general idea. Indeed when multi-mode.el does: (let ((late-hack (symbol-function 'hack-one-local-variable))) (fset 'hack-one-local-variable (lambda (var val) (unless (eq var 'mode) (funcall late-hack var val)))) (unwind-protect (hack-local-variables) (fset 'hack-one-local-variable late-hack)))) if the code run within `hack-local-variables` ends up looking at the definition of `hack-one-local-variable` and saving it somewhere for later use (e.g. if `defadvice` is used at this moment, maybe because AUCTeX was autoloaded during the course of running `hack-local-variables`), then we could have a problem. To some extent the final `fset` above should hide the problem, but it probably just ends up putting advice in an inconsistent state or worse (depending on the version of `advice`). I think with the new version of `advice.el` (introduced when `nadvice.el` was introduced, to make them interact tolerably) the problem should be minor: the final `fset` should not put `advice.el` in an inconsistent state but should simply undo the `defadvice` that happened during `hack-local-variables` (which is still undesirable, arguably, but should be mostly harmless as long as you don't rely on the feature implemented by that advice). > So I'm guessing that's what he's referring to. Indeed. You might like to try the patch below (guaranteed 100% untested). Do you think it would be worthwhile to add it to GNU ELPA? Stefan diff --git a/multi-mode.el b/multi-mode.el index cece376..06291cf 100644 --- a/multi-mode.el +++ b/multi-mode.el @@ -1,6 +1,6 @@ -;;; multi-mode.el --- support for multiple major modes +;;; multi-mode.el --- support for multiple major modes -*- lexical-binding: t; -*- -;; Copyright (C) 2003, 2004, 2007, 2009 Free Software Foundation, Inc. +;; Copyright (C) 2003-2020 Free Software Foundation, Inc. ;; Author: Dave Love ;; Keywords: languages, extensions, files @@ -164,27 +164,26 @@ Buffer local.") "Original value of `imenu-create-index-function' for the buffer's mode.") (make-variable-buffer-local 'multi-late-index-function) +(define-obsolete-variable-alias 'multi-alist 'multi-mode-alist nil) (defvar multi-mode-alist nil "Alist of elements (MODE . FUNCTION) specifying a buffer's multiple modes. MODE is a major mode and FUNCTION is a function used as an element of `multi-chunk-fns' or nil. Use nil if MODE is detected by another element of the alist.") -(if (fboundp 'define-obsolete-variable-alias) - (define-obsolete-variable-alias 'multi-alist 'multi-mode-alist) - (make-obsolete-variable 'multi-alist 'multi-mode-alist)) - ;; See the commentary below. (defun multi-hack-local-variables () "Like `hack-local-variables', but ignore `mode' items." - (let ((late-hack (symbol-function 'hack-one-local-variable))) - (fset 'hack-one-local-variable - (lambda (var val) - (unless (eq var 'mode) - (funcall late-hack var val)))) + (let ((f (lambda (orig-fun var val) + (unless (eq var 'mode) + (funcall orig-fun var val))))) (unwind-protect - (hack-local-variables) - (fset 'hack-one-local-variable late-hack)))) + (progn + (advice-add 'hack-one-local-variable :around f) + (hack-local-variables)) + (advice-remove 'hack-one-local-variable f)))) + +(defvar multi-mode) (defun multi-install-mode (mode &optional chunk-fn base) "Add MODE to the multiple major modes supported by the current buffer. @@ -235,21 +234,7 @@ is the base mode." (funcall mode)) ;; Now we can make it local: (set (make-local-variable 'multi-mode) t) - ;; Use file's local variables section to set variables in - ;; this buffer. (Don't just copy local variables from the - ;; base buffer because it may have set things locally that - ;; we don't want in the other modes.) We need to prevent - ;; `mode' being processed and re-setting the major mode. - ;; It all goes badly wrong if `hack-one-local-variable' is - ;; advised. The appropriate mechanism to get round this - ;; appears to be `ad-with-originals', but we don't want to - ;; pull in the advice package unnecessarily. `flet'-like - ;; mechanisms lose with advice because `fset' acts on the - ;; advice anyway. - (if (featurep 'advice) - (ad-with-originals (hack-one-local-variable) - (multi-hack-local-variables)) - (multi-hack-local-variables)) + (multi-hack-local-variables) ;; Indentation should first narrow to the chunk. Modes ;; should normally just bind `indent-line-function' to ;; handle indentation. @@ -290,9 +275,9 @@ is the base mode." ;; Kill the base buffer along with the indirect one; careful not ;; to infloop. (add-hook 'kill-buffer-hook - '(lambda () - (setq kill-buffer-hook nil) - (kill-buffer (buffer-base-buffer))) + (lambda () + (setq kill-buffer-hook nil) + (kill-buffer (buffer-base-buffer))) t t) ;; This should probably be at the front of the hook list, so ;; that other hook functions get run in the (perhaps) @@ -306,7 +291,7 @@ is the base mode." (setq buffer-file-coding-system coding) ;; For benefit of things like VC (setq buffer-file-name file) - (vc-find-file-hook)) + (vc-refresh-state)) ;; Propagate updated values of the relevant buffer-local ;; variables to the indirect buffers. (dolist (x alist) @@ -373,8 +358,7 @@ Fontifies chunk-by-chunk within the region from START for up to Works piece-wise in all the chunks with the same major mode. Assigned to `imenu-create-index-function'." (let ((selected-mode major-mode) - imenu-alist ; accumulator - last mode) + imenu-alist) ; accumulator (multi-map-over-chunks (point-min) (point-max) (lambda () @@ -454,8 +438,7 @@ Destructively modifies `multi-mode-list' to avoid consing in Return a list (MODE START END), the value returned by the function in the list for which START is closest to POS (and before it); i.e. the innermost mode is selected. POS defaults to point." - (let ((fns multi-chunk-fns) - (start (point-min)) + (let ((start (point-min)) (mode (with-current-buffer (multi-base-buffer) major-mode)) (end (point-max)) @@ -500,28 +483,6 @@ mode is selected. POS defaults to point." (fundamental-mode) (error "`multi-mode-alist' not defined for multi-mode"))) -;; In 21.3, Flyspell breaks things, apparently by getting an error in -;; post-command-hook and thus clobbering it. In development code it -;; doesn't do that, but does check indirect buffers it shouldn't. I'm -;; not sure exactly how this happens, but checking flyspell-mode in -;; the hook functions cures this. For the moment, we'll hack this up. -;; (Let's not bring advice into it...) -(eval-after-load "flyspell" - '(progn - (defalias 'flyspell-post-command-hook - `(lambda () - ,(concat (documentation 'flyspell-post-command-hook) - "\n\n[Wrapped by multi-mode.]") - (if flyspell-mode - (funcall ,(symbol-function 'flyspell-post-command-hook))))) - - (defalias 'flyspell-pre-command-hook - `(lambda () - (concat (documentation 'flyspell-pre-command-hook) - "\n\n[Wrapped by multi-mode.]") - (if 'flyspell-mode - (funcall ,(symbol-function 'flyspell-pre-command-hook))))))) - ;; This is useful in composite modes to determine whether a putative ;; major mode is safe to invoke. (defun multi-mode-major-mode-p (value)