Closed nbfalcon closed 3 years ago
Thank you so much! I've been meaning to attempt this refactor but haven't had the time, since it's a lot of abstraction to hold in one's head at once. I'll have a look at this as soon as I can.
Sorry for the many "debugging" commits, but I cannot reliably run the tests locally since I don't use eldev
.
Finally the tests work again!
I notice that you modified or removed a few tests. Can you explain why? Are the tests made irrelevant by the new implementation, or are there some features that could not be implemented in this way, which were removed instead? Also, can you undo the version number change? I'll handle the versioning when I make a new release.
I have explained the reasoning for modifying/removing tests in conversation comments. As for the version number change, this should be a major release, since it contains breaking changes. 3.4 -> 2.4 is wrong though, so I have reverted.
Hmm, so I guess the underlying problem with these 2 tests is that the new code treats KEYS as an unevaluated macro argument (with a special case to handle a surrounding quote form without evaluating it). In my original implementation, I tried my best to make KEYS work like a regular function argument, despite with-simulated-input
being a macro. I don't like the current state, because a key sequence like '("a" "b" "c")
works, but another form that evaluates to the same thing, e.g. (list "a" "b" "c")
, does not work, because we aren't actually evaluating the quoted form, we're just stripping the quote and keeping the unevaluated form inside it.
Ideally, I'd like to retain support for the current way of calling with-simulated-input
(i.e. full compatibility with the current test suite), even if we decide to deprecate it with a warning in favor of the new way of calling it. Is it possible to do this without breaking the edebug spec completely? If not, I think we need to unambiguously establish the new unevaluated syntax as the correct one, and issue a deprecation warning for any quoted forms encountered, along with an informative error message for forms that look like the caller was expecting them to be evaluated, such as (list "a" "b" "c")
.
The problem isn't in the edebug spec though (they could also be made to support lists, or fall-back to sexps if KEYS is just a variable), but rather in that if the forms can be entirely constructed at run-time, there is no way for the byte-compiler to know what variables are actually used, resulting in warnings and this not working with dynamic binding.
Do you want me to also handle (list ...) and issue a depreciation warning, to issue a depreciation warning for any form ((functionp car) ...)?
if the forms can be entirely constructed at run-time, there is no way for the byte-compiler to know what variables are actually used, resulting in warnings and this not working with dynamic binding.
I think it's fine if the old method only works in a non-byte-compiled setting, since that's the only way it ever worked in the first place. You could just issue an error if such a form is encountered at compile time. Does that seem reasonable? This would make it backward-compatible with all current uses of the package, I believe.
Detecting old vs new-style forms isn't possible reliably though, so we would have to rely on a heuristic, e.g. checking if the car of the form is a function. Do you think this would be enough?
IMHO a feature to generate forms at run-time isn't worth it, since such uses would be very unidiomatic (the eval
might as well be explicit) and doing so would result in byte-compile warnings any time locals are used.
As a side note, execute-kbd-macro
can be nested, so the following works:
(with-simulated-input '("abn" (execute-kbd-macro (kbd "RET")))
(read-string "M: "))
Yeah, I agree it's not ideal. The only reason I want to do it at all is to maintain backward compatibility as much as possible, because it's not very polite to issue an upgrade that breaks people's code without a very good reason. I guess the ambiguity comes when you have something like:
(defvar something "hello")
(defun something (&rest args) args)
(with-simulated-input (something "SPC world RET") ...)
In which case it would be valid to interpret it in either way. So it looks like an heuristic is indeed required. Perhaps check if the car is bound as a function and also not bound as a variable, and if so, assume it is an old-style form? I think this is safe, because in that case the form can't possibly be interpreted as a new-style form without throwing an error.
By the way, don't worry about updating the README or anything. I'll make sure to update the docs appropriately before making a new release. And to answer your earlier point about version numbers, this will definitely be a major version bump.
Do you think checking for global variable definitions is enough? There is no way to check in the current lexenv elegantly. Also, do you think we should support (list ) and (quote ...) still? The latter definitely makes sense, since it was previously the only way to get list forms (minimizing breakage). Should we issue a warning?
Hmm, I guess you're right, there's no way to tell in general if the symbol is bound in the current lexical environment. Let me see if I can search on Github for every current user of with-simulated-input and see how it is being used in practice. If no one is using the "eval keys at runtime" feature, then it might be safe to just drop it with no backward compatibility beyond quote and maybe list.
Ok, so I see 4 packages on Github that appear to be using runtime-evaluated KEYS: https://github.com/DarwinAwardWinner/with-simulated-input/issues/13#issuecomment-813099803
I wonder if it's possible to rewrite all of these in such a way that they will work with both the new and old style KEYS. If we could do that, then it would be safe to drop support for the old-style runtime-evaluated KEYS.
Ok, so for all the packages using runtime-eval, it looks like this usage pattern should work for both the old and new style for interpolating a string input stored in a lexical variable:
(let ((input-string "a RET"))
(with-simulated-input '((execute-kbd-macro (kbd input-string))) (read-string "")))
So, I think the best strategy is for me to submit pull requests to the packages in question making the appropriate changes, and then merge this and release the new version. What do you think?
As for handling (list ...)
, if it's as simple as just taking the cdr of the input, then sure, go ahead and implement it as another deprecated form.
After analyzing the examples, "dynamic" forms all boil down to computing a single string at run-time. I propose that we just use
(with-simulated-input '(inputs...))
for a list of inputs, and assume anything else (not starting with quote
) is assumed to be a single-string input, evaluated beforehand. This is probably a good idea, not only for backwards compatibility, since function-call computed inputs are useful.
Ok, so you are basically proposing the following semantics:
kbd
.quote
is treated the same way as the current release version: a list of elements, each of which is either a string literal (treated as above) or a lisp form to be evaluated. Importantly, the return values of these lisp forms are discarded; they are only called for side effects.quote
is treated as a single lisp form to be evaluated, which must return a single string that is interpreted as above. (Technically the first case is a special case of this one.)Is that correct? If so, then I believe all of the above matches the current semantics, except that we are dropping support for runtime evaluation of a KEYS argument that returns anything other than a string. And your argument is that dropping this feature is fine, because:
eval
inside a quoted list of lisp forms, and requiring an explicit eval in this case is arguably an improvement.Is that an adequate summary of the current state of things? I want to make sure I fully understand what you're proposing.
Almost, the last part is correct. After the last commit, we have the following semantics:
(with-simulated-input '(KEYS...))
; every element is either a variable (evaluating to string), a string or a cons (evaluated); even the first element, which would not make it a function call
(with-simulated-input (list KEYS...))
; every element of KEYS is either a variable or a function call yielding a string which is evaluated beforehand to build KEYS. Quoted forms are considered to be actions. TODO: Perhaps the former type of forms should be wrapped in execute-kbd-macro so that they are evaluated in the same order as the actions?
(with-simulated-input (SYMBOL ARGS...))
; Call SYMBOL with ARGS, the result being a string that is executed.
(with-simulated-input (NOT-A-SYMBOL KEYS...))
; Like (with-simulated-input '(NOT-A-SYMBOL KEYS...))
All of these variations can be stepped trough using edebug
with equal support.
I agree that this is a lot of special casing for backwards compatbility, but at least all that complexity is contained within the macro, not the function doing the work.
Note that with-simulated-input
now relies on autoloading for with-simulated-input-1
to be loaded at run-time if the file using it is compiled and only uses (eval-when-compile (require 'with-simulated-input))
. This IMHO simplifies the package, since all the backwards-compatibility interpretation of KEYS is left to a simple macro. I don't believe this to be a problem though, since most users use the package for testing only, where run-time = expansion time. If you want though, I can change this.
I agree that this is a lot of special casing for backwards compatibility
I think this is fine, as long as we're clear about which cases are intended only for backward compatibility, and as long as the semantics are sufficiently straightforward if you ignore the backward compatibility support. So, for the 4 cases you described above, I think 2 and 3 are purely for backward compatibility and not considered to be intended uses going forward, right? What about 1 and 4? Are they both equally intended, or is one of them intended to be the canonical usage pattern? It seems to me that 1 is preferred, since it avoids the chance of unintentionally falling into case 3 if the first element is a symbol.
Long-term, I guess the ideal is to only have case 4, and without the restriction of the first element not being a symbol. In order to do that, we would establish the current semantics as an intermediate step, with cases 2 and 3 producing deprecation warnings, and then later we would drop support for them entirely and eliminate the "not a symbol" limitation on case 4 at the same time, and at that point case 4 becomes the canonical usage.
By the way, do we actually need case 2 at all? I guess it was only ever used within the test suite, and you've already rewritten those tests to avoid that case. As for case 3, it is only ever used a single time in the virtualenvwrapper.el tests, and that code is actually wrong and should be replaced. (It interprets the result of executable-find
as a key sequence instead of a literal string to be inserted.) I think I will submit a pull request that both fixes this bug and eliminates this sole extant usage of case 3, which would mean that we could drop case 3 as well, along with the "not a symbol" caveat in case 4. If I'm not mistaken, that would pretty much solve the problems with backward compatibility, right? Then we could just support cases 1 and 4, with the only difference between them being the quote symbol in 1. (At that point we should probably designate one or the other as the canonical usage and deprecate the other, to avoid confusion going forward. But this can be handled in a future version.)
Also, thank you for your patience as I work through this. I want to make sure I get the syntax and semantics right this time around.
Having thought about it some more, I think that if we ignore backward compatibility concerns, case 4 is the ideal syntax. Case 1, with the quote symbol, wrongly implies that KEYS is evaluated like a normal lisp form. So assuming we can eliminates cases 2 and 3 as described above, I think we should embrace the fact that with-simulated-input
is a macro that implements a sort of "control structure" and designate case 4 as the preferred syntax, with case 1 being supported only for backward compatibility.
The developer of virtualenvwrapper.el accepted my pull request, which if I'm not mistaken means that there are no more extant uses of case 3 on Github, and we can safely drop support for it and eliminate the caveat for case 4.
Reducing special casing is better, I agree. However, IMHO we need some way to represent:
(list ...)
forms? I.e. check if evaluating a form yields a string, then call execute-kbd-macro
on the resulting string.I propose the following:
(with-simulated-input (function args...)) ; call FUNCTION with ARGS to yield a KEYS string
(with-simulated-input '(FORMS...)) ; like (with-simulated-input (FORMS...)) currently; string results of FORMS will be passed to `execute-kbd-macro`, so something like:
(let ((x (<compute-string>))) (with-simulated-input '("M: " x "RET") (read-string "M: "))
; would currently be expressed as
(let ((x (<compute-string>))) (with-simulated-input ("M: " (execute-kbd-macro x) "RET") (read-string "M: "))
I'm hesitant to take lisp forms that return strings in KEYS and treat those strings as key sequences, because you could easily have a lisp form that returns a string only incidentally, e.g. (setq x (minibuffer-contents))
, which also returns the result of (minibuffer-contents)
as a string. This is the reason I originally treated string literals and lisp forms differently in the current version: because I could always be sure that string literals had no side effects, so interpreting them as key sequences was unambiguous. Technically I could also interpret bare symbols as variable names to be evaluated and used as strings representing key sequences, since they also unambiguously have no side effects, but I think it makes more sense to treat all non-literal forms the same instead of treating variables differently from other lisp forms.
So for a string computed by a lisp form that the caller wants to treat as a key sequence, I think it's fine to require the caller to wrap it in execute-kbd-macro
or insert
(and possibly also kbd
) explicitly. This makes the semantics simple to explain: string literals are executed as key sequences, everything else is evaluated as a lisp form for side effects (with return value ignored). And it also makes it more clear what's happening to someone who is not familiar with with-simulated-input
.
So, I don't think (with-simulated-input (FUN args...))
is needed as a special case, since it is already supported as (with-simulated-input ((execute-kbd-macro (FUN args...))))
, which makes it explicit that the return value of the function is being treated as a key sequence. Admittedly the extra set of parens around execute-kbd-macro
is confusing at first, but a single-variable let
has the same issue, so it won't be an unfamiliar issue to elisp programmers.
In summary, I think everything can be supported with just 2 cases:
(with-simulated-input STRING-LITERAL ...)
(with-simulated-input (STRING-OR-LISP-FORM ...) ...)
, where each STRING-OR-LISP-FORM
is either a string literal key sequence or a lisp form evaluated for side effects only. We also need to support a third case (with-simulated-input '(STRING-OR-LISP-FORM...) ...)
for backward-compatibility, possibly with a deprecation warning. (But I will make the deprecation decision later after merging, so you don't need to worry about that.)
Does that sound reasonable? The main "gotcha" that I see is that return values of lisp forms are ignored. But I think this is still less surprising than treating string return values differently from all others. And the simpler semantics makes it easier to add new features later without breaking things (e.g. treating numbers as single characters to be inserted).
The other advantage of making things explicit in this way is that it forces people to choose between insert
and execute-kbd-macro
. I feel like in the majority of cases people want insert
for computed strings, but if with-simulated-input
intrepreted them directly as key sequences it would be more like defaulting to execute-kbd-macro
. That was essentially the problem that this pull request fixed.
This sounds reasonable - defaulting to execute-kbd-macro
is not sensible for all use cases. However, if variables are also evaluated for side-effects only, they would not make any sense in KEYS. As such I propose the following:
(with-simulated-input (SYMBOL ARGS)) ; call SYMBOL with ARGS; this is syntax-sugar for (with-simulated-input ((...)) ...)
(with-simulated-input (STRING-OR-FORM...)) ; warning if a variable is used in STRING-OR-FORM
(with-simulated-input '(STRING-OR-FORM...)) ; backwards-compatibility
What do you think?
Hmm... I think issuing a warning for bare symbols in KEYS is definitely reasonable. (We could later add warnings for other forms that are known to be side-effect-free, such as lambda expressions. But just warning on symbols is fine for now.)
I'm still not sure I like the idea of (with-simulated-input (SYMBOL ARGS...) ...)
as a shortcut for (with-simulated-input ((SYMBOL ARGS...)) ...)
, because you can't replace SYMBOL with an arbitrary expression that evaluates to a function and have it work the same way. I think I'm going to say no to that feature for now, or at least I think it doesn't need to be part of this pull request, since the main goal here is just to replace my original closure hack with a proper implementation without breaking backward compatibility. It's always possible to add the shortcut in a later update with little to no chance of breaking any existing code.
@DarwinAwardWinner you can't do that in regular Elisp/Common LISP either (since it isn't Scheme):
((identity #'1+) 10)
will yield an error ((invalid-function (identity #'+))
), because (identity #'1+)
is not a symbol.
((lambda (x) x) 10)
strangely works, though (it has to be lambda
, not some macro that expands to lambda
). I have yet to see that form anywhere.
((identity +) 10 20)
works in racket just fine, though.
Oh, I forgot that lambdas were a special case in this context. So I guess with this proposed shortcut, KEYS would only be interpreted as (FUN ARGS...)
if FUN is either a symbol or a lambda form, right? And both of these are side-effect free when evaluated directly, so in theory it's safe to allow this shortcut, at least from a syntactic perspective, because when KEYS is a list of strings and forms with side effects, it doesn't make sense for any of those forms to be symbols or lambda forms.
However, I still have my doubts about allowing this usage, because it adds another special case. Because now the semantics become: when KEYS is a list of strings and forms, the return value of the forms is ignored. But when KEYS is a single lisp form, we evaluate it and use the return value as a string, which is then passed to execute-kbd-macro
. This is not intuitive, and was even the source of the bug I fixed in the pull request referenced above. Another option would be to allow KEYS to be a single lisp form, but still ignore the return value, i.e. use it for side effects only, e.g.:
(with-simulated-input
;; A bad example since the `progn' is redundant, but you get the idea
(progn (insert "hello world") (execute-kbd-macro (kbd "RET")))
(read-string "Enter text: "))
Again though, I think this current PR should be focused on switching to the new core implementation, and then we can consider adding this new syntactic sugar and decide on its exact semantics later.
The syntax sugar can be added in another PR. Should I drop '(...) as well?
Should I drop '(...) as well?
No, that has to be kept because it is required for backward compatibility. I'll implement an appropriate deprecation warning for it before the next release.
Current state:
'(KEYS...)
('STRING
will not work, but I doubt that anyone uses this)(KEYS)
STRING
= (STRING)
SYMBOL
= (SYMBOL)
-> ignored (no input). Is this correct?edebug
support
SYMBOL
=(SYMBOL)
-> ignored (no input). Is this correct?
That's fine for now. I might make this an error instead, but I'll figure that out after merging and write appropriate tests for it. As mentioned, we can decide later if this should be supported as a shortcut for something else.
Overall, i think this is basically ready to merge. I'll give it one more review from top to bottom before merging, when I have the time (might not be for a week or more since I'm on semi-vacation).
Since you are contributing a substantial amount of core code to this package, if you would like to be listed as a 2nd author, please add one more commit to this PR adding yourself to the author list, as described here. (And also add a Maintainer field listing me, to make it unambiguous who people should bother for support.)
Done, I have added myself to the Authors list and you as ;; Maintainer:
.
As for the call-expression shortcut, it no longer seem that sensible to me on second thought, since I agree that execute-kbd-macro
should be made explicit and that insert
is better most of the time. It would be very easy to implement, though.
The only remaining "bad" users of with-simulated-input
use a symbol for run-time eval of keys, which are gotten from alists. IMHO this is a valid use case, and we should support it more conveniently (perhaps a lone symbol = execute-kbd-macro
), or perhaps symbols in general should be executed? In many cases insert
is better, except when the input is static.
Allowing symbols would only require reverting 2a48669.
The only remaining "bad" users of with-simulated-input use a symbol for run-time eval of keys, which are gotten from alists.
I'm not sure what you mean here. Can you give an example of this? Do you mean something like (with-simulated-input myvar ...)
, where myvar is a variable whose value is the desired KEYS? Is there some extant code like this?
All existing code in #13 uses a KEYS argument that is a single variable acquired from some list of keyboard macros, e.g. spiral, where they iterate over an alists of (KEYS-STRING . host). My proposal is to treat symbols as input for execute-kbd-macro
, like with the old implementation.
Ok, so we do need to support run-time evaluation of symbols in some capacity. However, for backward compatibility, the only case we absolutely need to support is when KEYS is just a single symbol and that symbol's value is a string. That is easy enough to enable, of course. So this needs to work for backward compatibility:
(let ((myinput "hello RET"))
(with-simulated-input myinput (read-string "")))
However, do we really want to support variable interpolation everywhere? What about the following?
;; myinput is a list of string key sequences
(let ((myinput '("hello" "RET")))
(with-simulated-input myinput (read-string "")))
;; myinput is a list of lisp forms to evaluate
(let ((myinput '((insert "hello") (exit-minibuffer))))
(with-simulated-input myinput (read-string "")))
;; A lisp form in myinput references another lexical variable
(let ((greeting "hello")
(myinput '((insert greeting) (exit-minibuffer))))
(with-simulated-input myinput (read-string "")))
All of the above examples work with the current release version, of course, but we don't need to support them because no one currently uses them. And indeed, I think part of the goal here is to eliminate these cases, because the rules for when and how they are evaluated are too complicated. So maybe we should only support the first example above, in which KEYS is a single symbol with a string value, and not any of these others. What do you think?
Also, I should note that "treat symbols as input for execute-kbd-macro
" is actually not what the current release version does. It doesn't have any special handling for symbols at all. It might appear that it does because this works (same as the first example above):
(let ((myinput "hello RET"))
(with-simulated-input myinput (read-string "")))
But that is just because it evaluates KEYS like a normal argument (instead of an unevaluated macro argument). If you have KEYS as a list containing symbols, those symbols are no-ops:
;; This returns the empty string because evaluating `non-greeting'
;; just returns a string and doesn't input anything into the
;; minibuffer
(let ((non-greeting "This is not a greeting")
(myinput '(non-greeting (exit-minibuffer))))
(with-simulated-input myinput (read-string "")))
Variable are now also supported, and behave like strings. The variable will be checked to be a string first, so e.g. passing a lambda wouldn't cause it to be called.
There is one use where the variable can evaluate to nil: https://github.com/alphapapa/org-ql/blob/208e103ecc146db71d878df3bd09c6eb60c2797d/tests/test-org-ql.el#L1460. Should this be supported or fixed in test-org-ql
?
So, I decided to take a slightly different approach: I attempted to implement full feature parity with the current release version using the new code that you provided. The idea is to first implement the change in internal implementation without any changes in behavior, and then separately decide whether any changes in behavior are warranted, rather than trying to manage both at once.
First, I revamped the test suite to better document the current behavior (and achieve 100% test coverage in the process). Then I merged with your branch, and finally I modified your code to pass all the tests. The result is, I think, a version of your code that implements all the currently-supported ways of using with-simulated-input
, which you can see in the rewrite-bleed branch. And here it is passing all the tests: https://github.com/DarwinAwardWinner/with-simulated-input/actions/runs/812273252
This current code is very much a prototype, not release-ready. The error messages are placeholders, I probably need to update the edebug specification as well, and I should probably add some tests for the new syntax supported by this branch that isn't supported by the old code, to make sure I haven't broken that in the process of making the old syntax work.
Note: I did have to make a slight compromise in one of the KEYS arguments in the tests in order to make it pass, but this is a case where the old KEYS probably shouldn't have worked in the first place, so I'm ok with it. (And of course it's in the "nobody uses this" section, so I doubt anyone will notice.)
I've added tests for un-quoted lists in a86f35f. I've also attempted to update the debug declaration in e7cc96e. If you don't mind, could you have a look at it and check my work?
Also, is there any way to automatically verify the correctness of the debug declaration? Perhaps by checking every use of the macro in the tests and making sure they conform to the debug spec?
error
messages should not be written in upper case ("INVALID VAR VALUE ..."
)(cond ((null keys) ... ) (stringp keys ...))
could just be (cond (or (null keys) (stringp keys)) (... ,keys))
, since ,keys can just evaluate to nil
.(eval `(lambda () ...))
; this isn't necessary in non-quoted expansions, since lambda
will be called and create a closure)As a side note, I have now pushed a patch that handles symbols evaluating to nil. This means that now all cases actually used would be handled. Function call forms aren't actually currently used by any dependency (from those listed in your issue). Do you think they need to be supported?
Ok, I think I have a version that's nearly ready for release, in the bleeding-edge branch. It is fully backward-compatible with all the version 2.x syntax as well as the new no-quote keys list syntax, and it has warnings for the parts of the keys syntax that are considered deprecated, as well as some common cases that indicate likely misuse of the macro (e.g. constant expressions in either KEYS or BODY). As a bonus, I also implemented support for passing characters in KEYS. I'm going to sit on it for a day or two to see if I notice anything else that needs fixing, and then I'm going to increment the version number and push it to master. (And then if nobody yells at me in a week or so, I'll tag a stable release.)
FYI, I have merged most of the commits in this pull request, along with my additions to get as close to 100% backward-compatibility as possible. When I get the chance, I'll extract the relevant discussions about any possible changes to the syntax into a separate issue and then close this pull request. Thank you for doing the hard part for me!
Rewrite
with-simulated-input
to expand more cleanly: returnlambda
forms so that correct closures are created at run-time, and make it hygienic by usingmake-symbol
for local variables.This should fix all current issues, but the list interpolation feature will have to be sacrificed, since there is no way to implement it properly without resorting to closure hacks (I don't believe it to be useful, though, since users could just use
execute-kbd-macro
in their forms to get the same effect).is now the canonical form, but for backwards compatibility, quoted lists are handled the same way.
Using LOOPFUNC is impossible if interspersed forms are to be allowed.
As a bonus,
with-simulated-input
can now be stepped trough usingedebug
.Drop
wsi-make-closure
/wsi-current-lexical-environment
.