From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#73725: Master: Wrong position for byte compiler warning message. Date: Tue, 22 Oct 2024 12:48:06 +0000 Message-ID: References: <8A05D4D2-C77B-4089-B82C-4DB3E88F1276@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39071"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 73746@debbugs.gnu.org, acm@muc.de, Mattias =?UTF-8?Q?Engdeg=C3=A5rd?= , 73725@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Oct 22 14:48:59 2024 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 1t3EJn-0009yD-AW for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 22 Oct 2024 14:48:59 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1t3EJS-0002Vp-B7; Tue, 22 Oct 2024 08:48:38 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t3EJP-0002VD-NO for bug-gnu-emacs@gnu.org; Tue, 22 Oct 2024 08:48:35 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1t3EJP-0002tT-2d for bug-gnu-emacs@gnu.org; Tue, 22 Oct 2024 08:48:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=From:In-Reply-To:MIME-Version:References:Date:To:Subject; bh=2UeGuHRSjX9CnjwH+HQMYXEDz4O/FmRS7TicYGlsk+E=; b=v6XDZ4OW5AE0W4hyOZnTFKmR+YwB7Xd7y4rbVxYtMSSIWVJq/GP1xiFZxIS3M1qd5rOYr1UpMS0WyWfvp1hqVlqIyd+R+M4w2UJM6QJUXSPnBupBzZvaSEf98VISV3EY/+/6f650hNQB42t7X/EQYOaElhuX5drRrwdBYEXRQjZZ1hGh2Ljd9XLQyoKx3KLkZCV7KM21nJCrdmW9wlRK6eFPDccC2igOhByxioeF2NlPTpubxIXpnYzmCGBiqXxURcq2PGoqpVldybeglEIhrVtOKwkQT42KZgscu7Mc6UUiKbVld9u+Du8wPgBAVZVC726GDSmyLiZtDyfIS80GNw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1t3EJq-00012f-JR for bug-gnu-emacs@gnu.org; Tue, 22 Oct 2024 08:49:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 22 Oct 2024 12:49:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73725 X-GNU-PR-Package: emacs Original-Received: via spool by 73725-submit@debbugs.gnu.org id=B73725.17296013263967 (code B ref 73725); Tue, 22 Oct 2024 12:49:02 +0000 Original-Received: (at 73725) by debbugs.gnu.org; 22 Oct 2024 12:48:46 +0000 Original-Received: from localhost ([127.0.0.1]:54983 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t3EJZ-00011u-G5 for submit@debbugs.gnu.org; Tue, 22 Oct 2024 08:48:46 -0400 Original-Received: from mail.muc.de ([193.149.48.3]:63474) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t3EJW-00011e-Fk for 73725@debbugs.gnu.org; Tue, 22 Oct 2024 08:48:43 -0400 Original-Received: (qmail 62759 invoked by uid 3782); 22 Oct 2024 14:48:07 +0200 Original-Received: from muc.de (p4fe15a4c.dip0.t-ipconnect.de [79.225.90.76]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Tue, 22 Oct 2024 14:48:06 +0200 Original-Received: (qmail 11185 invoked by uid 1000); 22 Oct 2024 12:48:06 -0000 Content-Disposition: inline In-Reply-To: X-Submission-Agent: TMDA/1.3.x (Ph3nix) X-Primary-Address: acm@muc.de 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:294100 Archived-At: Hello, Stefan. Thanks for giving my patch such a thorough review. On Sun, Oct 20, 2024 at 12:35:46 -0400, Stefan Monnier wrote: > > I haven't heard anything back in over a week, so I assume my patch for > > bug#73725 and bug#73746 is OK. I've committed it. > Sorry, got pushed too far down my todo while I was busy and forgot about it. > Side note: thinking some more about the problem you're trying to fix, > I can't help find it is somewhat funny: it's exactly the kind of > "position-preservation" work we were hoping to avoid having to do by > using SWPs. Well, we always knew that macros would be problematic. > Anyway, the general approach seems about right. > See comments below: > > @@ -510,7 +510,9 @@ byte-optimize-form > > (while > > (progn > > ;; First, optimize all sub-forms of this one. > > - (setq form (byte-optimize-form-code-walker form for-effect)) > > + (setq form > > + (macroexp-preserve-posification > > + form (byte-optimize-form-code-walker form for-effect))) > > > > ;; If a form-specific optimizer is available, run it and start over > > ;; until a fixpoint has been reached. > Is this needed? I'd expect we need `macroexp-preserve-posification` > only at those places where an optimization returns a form whose `car` is > different from that of FORM. So I expect this to happen in more > specific places inside byte-opt.el than here. Yes, it is needed. byte-optimize-form-code-walker sometimes does return a form with a different car. For example, a progn form with a single sub-form comes back without the progn. Even if there are several sub-forms, the code uses macroexp-progn to substitute a different progn symbol without the position. I don't know why it does this. One of my ideas was to fix byte-optimize-form-code-walker by fixing each individual bit where the SWP got lost. In the end I gave that up as too much work, and too difficult to test. > IOW, this deserves a clear comment explaining why we need it here, > probably with some kind of example. OK, I'll put one in, along the lines of the above, but less wordy. > > @@ -519,7 +521,8 @@ byte-optimize-form > > (let ((opt (byte-opt--fget (car form) 'byte-optimizer))) > > (and opt > > (let ((old form) > > - (new (funcall opt form))) > > + (new (macroexp-preserve-posification > > + form (funcall opt form)))) > > (byte-compile-log " %s\t==>\t%s" old new) > > (setq form new) > > (not (eq new old)))))))) > E.g. this is the kind of place where it makes sense to me. > > +(defun sub-macroexp--posify-form (form call-pos depth) > Please don't eat up namespace gratuitously. > IOW stick to the "macroexp-" prefix. Sorry, I wasn't thinking straight when I named that. I've already corrected it in my sources, it will appear in the next patch or commit I submit. > > + "Try to apply the transformation of `macroexp--posify-form' to FORM. > > +FORM and CALL-POS are as in that function. DEPTH is a small integer, > > +decremented at each recursive call, to prevent infinite recursion. > > +Return the changed form, or nil if no change happened." > Somewhere in the doc or in a comment we should document the "design", > which AFAICT is to try and find "one" place where we can put the > `call-pos` info: the docstring suggests we'll apply it to all the > symbols within `depth` (which would be both costly and undesirable), > whereas we actually stop (more or less) as soon as we find a good spot. Good point. The doc string of that function is a little clumsy, as it refers to the doc string of the next function macroexp--posify-form, since it is the recursive part of that function. Maybe I should just write out that of the first function in full, even though that would duplicate a lot of stuff. But I'll write somewhere that we modify "a single symbol", or something like that. > > + (let (new-form) > > + (cond > > + ((zerop depth) nil) > > + ((and (consp form) > > + (symbolp (car form)) > > + (car form)) > > + (setcar form (position-symbol (car form) call-pos)) > > + form) > AFAICT this can and occasionally will throw away valuable > position information: if `(car form)` is an SWP we don't know for sure > here that `call-pos` is always a better position info than the one > already carried by `(car form)`. Thanks. Such occasions will be when the form returned by the macro has as a car a SWP whose position is inside the macro. Such a position would indeed be a better position for the warning message that the position of the macro call. > We could consider combining position information (but this would make > the whole system more complex: when printing warnings/errors we'd have > to find a way to make use of the various recorded positions, ...). > But a simpler choice is to check (not (symbol-with-pos-p (car form))). I think the simpler choice is the one to go with, here. ;-) > > + ((symbolp form) > > + (if form ; Don't position nil! > > + (position-symbol form call-pos))) > Same here. Yes. > > +(defmacro macroexp-preserve-posification (pos-form &rest body) > > + "Evaluate BODY..., posifying the result with POS-FORM's position, if any." > This lacks a `declare` with `debug` and `indent` specs. Yes. Sorry, I'll fix these. > > + `(let ((call-pos (cond > > + ((consp ,pos-form) > > + (and (symbol-with-pos-p (car ,pos-form)) > > + (symbol-with-pos-pos (car ,pos-form)))) > > + ((symbol-with-pos-p ,pos-form) > > + (symbol-with-pos-pos ,pos-form)))) > > + (new-value (progn ,@body))) > > + (if call-pos > > + (macroexp--posify-form new-value call-pos) > > + new-value))) > > + > > (defun macroexp-macroexpand (form env) > > "Like `macroexpand' but checking obsolescence." > > (let* ((macroexpand-all-environment env) > > new-form) > > + (macroexp-preserve-posification > > + form > > (while (not (eq form (setq new-form (macroexpand-1 form env)))) > > + (setq macroexpanded t) > > (let ((fun (car-safe form))) > > (setq form > > (if (and fun (symbolp fun) > This `(setq macroexpanded t)` looks like some leftover code, at least > I couldn't find this var declared or used elsewhere. I remember removing it, but can't remember exactly why. When I byte compile the code, I don't get an undeclared variable warning for it, for some reason. > But yes, I suspect it would make a fair bit of sense to perform the > "preserve-posification" only when `macroexpand-1` did return a new form. > Did you try to do it (as suggested by this leftover code) and found it > was not worth the trouble or that it didn't work? I think that with the variable `macroexpand', the code was getting a bit bulky. But the check for macroexpand-1 returning a new form could easily be performed in the macro macroexp-preserve-posification. So I'll try that, and see what sort of effect it has on the timings. > If so, please add a comment recording it. > > @@ -253,7 +316,7 @@ > > (if (symbolp (symbol-function fun)) "alias" "macro")) > > new-form (list 'obsolete fun) nil fun) > > new-form)))) > > - form)) > > + new-form))) > Why? I can't remember. The change doesn't seem to make sense. > > -(defun macroexpand-all (form &optional environment) > > +(defun macroexpand-all (form &optional environment keep-pos) > Any reason why we need this new argument? > Can't we just always try to preserve the positions? There are lots of calls to macroexpand-all (around 37), and I was concerned about possible accidental side effects in these. Also, always trying to preserve the position might slow down compilation, but I haven't measured that yet. > Stefan -- Alan Mackenzie (Nuremberg, Germany)