Package: emacs;
Reported by: Alan Mackenzie <acm <at> muc.de>
Date: Thu, 10 Oct 2024 10:24:02 UTC
Severity: normal
Tags: patch
Message #35 received at 73725 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 73746 <at> debbugs.gnu.org, acm <at> muc.de, Mattias Engdegård <mattias.engdegard <at> gmail.com>, 73725 <at> debbugs.gnu.org Subject: Re: bug#73725: Master: Wrong position for byte compiler warning message. Date: Tue, 22 Oct 2024 12:48:06 +0000
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)
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.