bug#73746: bug#73725: Master: Wrong position for byte compiler warning message.

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

> 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.

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.

IOW, this deserves a clear comment explaining why we need it here,
probably with some kind of example.

> @@ -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.

> +  "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.

> +  (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)`.

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))).

> +     ((symbolp form)
> +      (if form                          ; Don't position nil!
> +          (position-symbol form call-pos)))

Same here.

> +(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.

> +  `(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.

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?
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?

> -(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?


        Stefan