27.1; compiled replace-string breaks repeat-complex-command

Interactive commands that act on the region are handled specially such

that when repeated with `repeat-complex-command`, the repeated command

uses the current region rather than the region used for the previous

invocation of the command.

`replace-string` does not respect this; it uses the previous region when

repeated with `repeat-complex-command`.

Note that loading `replace-string` from source (rather than byte

compiled) fixes this problem. So it's probably a problem with byte

compiled commands.

I swear I filed a bug for this a long time ago, and I can't remember if

it's a regression or it hasn't landed in a release yet. I can't find

the original bug.

In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)

of 2020-08-28 built on juergen

Windowing system distributor 'The X.Org Foundation', version 11.0.12010000

System Description: Arch Linux

Configured features:

XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY

INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB

TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON

PDUMPER LCMS2 GMP

Important settings:

value of $LANG: en_US.UTF-8

locale-coding-system: utf-8-unix

Allen Li <darkfeline@felesatra.moe> writes:

(I'm going through old bug reports that unfortunately weren't resolved

at the time.)

I can reproduce this problem in Emacs 29?

In any case, it's because `replace-string' specifies the start/end

position in the `interactive' spec (as it should), so it lands in

`command-history', and `repeat-complex-command' just executes that.

Other commands, like `flush-lines', have pass in nil as start/end, and

then computes the start/end in the body of the function.

So this can be fixed by rewriting `replace-string' to do the same...

but surely there's a lot of commands out there that say:

(interactive

[...]

(list

(if (use-region-p) (region-beginning))

And all of these would have the same problem. (interactive "r") does

not, because in that case:

(defun foo (start end)

(interactive "r")

(message "%s %s" start end))

The following ends up there in the history:

(foo (region-beginning) (region-end))

Does anybody know of a more general solution to this?

The reason replace-string works when it's not compiled is the because

then this ends up in command-history:

(replace-string "buffer" "foo" nil (if (use-region-p) (region-beginning)) (if (use-region-p) (region-end)) nil nil)

For some reason.

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

tags 45607 + moreinfo

quit

This feature is broken by design as I explained in

It isn't broken, you just expect it to do some magic that it never

meant to do.

As in many other cases, the perfect is the enemy of the good here.

Juri Linkov <juri@linkov.net> writes:

Hm... Is there any way forward here? Could we institute some special

form to be used in interactive specs that will be recorded in

command-history in a useful manner? That is, code that today is:

(if (use-region-p) (region-beginning))

(if (use-region-p) (region-end))

could be something like

(interactive-region-beginning)

(interactive-region-end)

and whatever updates command-history would reify those as is instead of

their return values?

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

On Tue, Jun 7, 2022 at 11:58 AM Eli Zaretskii <eliz@gnu.org> wrote:

I think it's reasonable to consider the different behavior of evaled vs

compiled to be a bug. Which one is correct can be debated, but the fact

that they're different is a bug. Would you disagree?

That's not what I alluded to, not at all. I was talking about

repeat-complex-command itself and its alleged "broken" state.

Lars Ingebrigtsen <larsi@gnus.org> writes:

Sometimes it's helpful to actually look at the code. All this magic

comes from:

/* If the list of args INPUT was produced with an explicit call to

`list', look for elements that were computed with

(region-beginning) or (region-end), and put those expressions into

VALUES instead of the present values.

This function doesn't return a value because it modifies elements

of VALUES to do its job. */

static void

fix_command (Lisp_Object input, Lisp_Object values)

{

/* FIXME: Instead of this ugly hack, we should provide a way for an

interactive spec to return an expression/function that will re-build the

args without user intervention. */

if (CONSP (input))

And what this does is to try to hack its way through the lisp code in an

interactive spec like

/* Skip through certain special forms. */

while (EQ (car, Qlet) || EQ (car, Qletx)

|| EQ (car, Qsave_excursion)

|| EQ (car, Qprogn))

looking for `region-beginning' and friends. But we now byte-compile the

interactive specs, so all this fails spectacularly.

So we need a brand new way to specify which options are

`region-beginning' etc. Perhaps with a declare form? (That translates

into symbol properties, I guess.)

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

Lars Ingebrigtsen <larsi@gnus.org> writes:

I.e.,

(defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)

...

(declare (arg start (if (use-region-p) (region-beginning)))

(arg end (if (use-region-p) (region-end))))

and fix_command would pick them up from the symbol plist and use those

forms instead of the value for these arguments.

This could be generally useful if we have other things like this that we

want to have reified in a particular way in the command history.

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

I thank you for trying to fix this. And

what you've said so far make sense to me.

This problem has long bugged me.

Lars Ingebrigtsen <larsi@gnus.org> writes:

If we do that, it would be impossible to explicitly specify START and

END values that are different from an active region from ELisp code. If

the region is active, those arguments would always just be ignored.

We would substitute one ugly corner case with another one, but would

have added more semantic complexity.

We only have a problem for `repeat-complex-command' usage, right? Then

the effect of a new `declare' spec should better be limited to the value

added to `command-history'.

Michael.

Michael Heerdegen <michael_heerdegen@web.de> writes:

[...]

Isn't that all that fix_command does?

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

Lars Ingebrigtsen <larsi@gnus.org> writes:

Please ignore my comment if it does, I haven't checked.

Michael.

Indeed, some users might want to have numbers for START and END values

to repeat the command exactly on the same previous region, but other users

might want to repeat the command on a newly selected region with

(region-beginning)/(region-end) in the command history.

OTOH, keeping numbers in the history breaks replace-string for

rectangular regions, because the command history will contain

inconsistent numbers: some numbers from the previous replacement,

and other numbers from the new rectangular region. For example:

(replace-string "buffer" "foo" nil 1 2 nil '((3 . 4) (5 . 6)))

where 1 2 are the old region boundaries, and '((3 . 4) (5 . 6))

is a new rectangular region boundaries.

Juri Linkov <juri@linkov.net> writes:

[...]

Have a look at fix_command -- it tries to parse code in an interactive

spec to find instances of

preserved_fns = pure_list (intern_c_string ("region-beginning"),

intern_c_string ("region-end"),

intern_c_string ("point"),

intern_c_string ("mark"));

in the code. (Which doesn't work now, of course, since the spec is

byte-compiled.) My `declare' suggestion would just make this work

again, and fix a regression. That is, this isn't new functionality.

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

Does `declare' put some property on the command's symbol?

Then if a user doesn't want this fix_command thing, it's

easy to customize and remove a special property from the symbol

of a command like `replace-string'.

Juri Linkov <juri@linkov.net> writes:

`declare' forms can do basically anything, but, yes, in this case, I

think a symbol property would make the most sense.

--

(domestic pets only, the antidote for overdose, milk.)

bloggy blog: http://lars.ingebrigtsen.no

Lars Ingebrigtsen <larsi@gnus.org> writes:

I've now fixed the replace-string problem in Emacs 29.

close 45607 29.1

quit

Should the same interactive-args now be added to other

replacement commands?

Juri Linkov <juri@linkov.net> writes:

It should be added to all commands that work on the region like this,

yes. But I wondered whether we should make some trivial helper

functions first like

(defun use-region-beginning ()

"Return the start of the region if `use-region-p'."

(and (use-region-p) (region-beginning)))

and the same for -end to avoid having to repeat that code phrase so many

places.

Indeed, this will help to make the history items shorter:

(replace-string "a" "b" nil (if (use-region-p) (region-beginning)) (if (use-region-p) (region-end)))

->

(replace-string "a" "b" nil (use-region-beginning) (use-region-end))

Bonus points for calling use-region-p just once, not twice.

Juri Linkov <juri@linkov.net> writes:

OK, so I've now done this. So somebody™ should go through the code base

and adjust the code and add the new `declare' forms. ?

Eli Zaretskii <eliz@gnu.org> writes:

I don't really see a convenient way to do that in these cases -- the

only ones I could think of (adding a new function to return both

start/end, and then splice the results in into the `interactive' specs

etc) would lead to obfuscated code.

(And it's not like this is performance critical anyway.)

What about cl-destructuring-bind and its ilk?

It's IMO inelegant to make the same test twice in a row.

Eli Zaretskii <eliz@gnu.org> writes:

That'd possible, of course, but awkward.

Yup.

Interesting, there are not too many uses of this pattern,

and most of them are related to replacement commands.

Ok, I could replace them with adding interactive-args.

lisp/isearch.el

2392: (if (use-region-p) (region-beginning))

2393: (if (use-region-p) (region-end))

lisp/replace.el

464: (if (use-region-p) (region-beginning))

465: (if (use-region-p) (region-end))

558: (if (use-region-p) (region-beginning))

559: (if (use-region-p) (region-end))

606: (if (use-region-p) (region-beginning))

607: (if (use-region-p) (region-end))

761: (if (use-region-p) (region-beginning))

762: (if (use-region-p) (region-end))

lisp/textmodes/paragraphs.el

518: (if (use-region-p) (region-beginning))

519: (if (use-region-p) (region-end))))

lisp/vc/log-view.el

581: (list (if (use-region-p) (region-beginning) (point))

582: (if (use-region-p) (region-end) (point))))

596: (list (if (use-region-p) (region-beginning) (point))

597: (if (use-region-p) (region-end) (point))))

Juri Linkov <juri@linkov.net> writes:

That's fewer than I'd have guessed -- but I guess that quite a few

commands stash the logic down into the function body instead of putting

it into the `interactive' spec. Commands like `duplicate-dwim', for

instance, could be pretty easily fixed in that way, for instance.

OTOH, `duplicate-dwim' is optimized to call `use-region-p' only once:

(cond

((use-region-p)

(let* ((beg (region-beginning))

(end (region-end))

Juri Linkov <juri@linkov.net> writes:

I don't think that makes much difference when it comes to interactive

specs:

(benchmark-run 1000000 (use-region-p))

=> (0.038645288 0 0.0)

If you're calling `duplicate-dwim' interactively more than a million

times a second, you'll get a slowdown of a couple hundredths of a

second.

But still `use-region-p' is used to start a new logical branch of `cond'.

Juri Linkov <juri@linkov.net> writes:

The suggestion was to put that logic into the interactive spec and bind

to new parameters beg/end (like other commands do). The `cond' would

then be adjusted to react to beg/end.

Done, with adding also 'use-region-noncontiguous-p'.