smartparens
smartparens copied to clipboard
Fuco1
Coding issue: code transformation commands should use the '*' in the interactive spec stating they do not modify read-only buffers.
Expected code behavior
The interactive functions that transform code, such as sp-add-to-previous-sexp should use the "*" character in the interactive form to prevent operation inside a read-only buffer. So instead of using:
(interactive "P")
it should use:
(interactive "*P")
I could provide a PR if I see a reply here.
Version information
smartparens version: 20210904.1621
With or without the * I get the same error Buffer is read-only: #<buffer *scratch8*>.
What version of Emacs are you using. I can't see the difference between the two versions of the interaction spec. For "read only" I used read-only-mode. How did you setup the buffer?
With or without the * I get the same error Buffer is read-only: #<buffer *scratch8*>.
What version of Emacs are you using. I can't see the difference between the two versions of the interaction spec. For "read only" I used read-only-mode. How did you setup the buffer?
I was just reading the code. I tried setting a buffer with read-only-mode and applied sp-add-to-previous-sexp and it did refuse to execute with the error stating the buffer was read-only as expected.
I reported that because I normally always put a * in the interactive spec of code that modifies a buffer as the docstring of interactive states:
"... In addition, if the string begins with ‘*’, an error is signaled if the buffer is read-only."
I assume something else detects the buffer being read only. What part of you code detects it?
I'm using Emacs 26.3
This specification is probably for functions which should not be run in read-only buffers, even though they don't modify the buffer.
Attempting to modify a read-only buffer results in an error anyway, which is what happens here.
The code in emacs/src/callint.c that defines the behaviour checks buffer state when '*' is specified and returns an error if the buffer is read-only.
IMO using the '*' explicitly is a good practice. It would always be possible for a function to modify a global variable before attempting to modify the read-only buffer. In that case the '*" interactive specification will prevent the function to execute. If you do not have it the function will run, will modify the variable and then the code trying to modify the buffer will raise the error, potentially leaving the data in an invalid state.
If that code was mine I would add the '*'. It's not my code.
Yes I agree. I'd even go further and say that functions which shouldn't run in read-only buffers, regardless of whether they modify the buffer, should take steps to check if the buffer is read only themselves.
The interactive specification "*" is not necessarily enough, as it won't catch non-interactive calls.
But I don't think any of smartparens' buffer modification functions has such hidden side effects.
It's possible that the current code has no side effect. But a language feature like that is a kind of defensive-programming mechanism. It does not hurt not having it in place (except for extra C code lines executed) unless someone wants to and it will protect against future modifications. It also has a documentation value.
But again, this is not my code. If the author wants to close the ticket instead, that's fine to me.
@ThibautVerron , the reason non-interactive functions do not have checks for read-only buffers might be performance. However a large chunck of code is called via interactive functions. If all these interactive function take advantage of the '*' specification that makes the function explicitly check for the buffer being read-only then the probability of preventing execution of non-interactive code that should check for buffer being read-only and does not is reduced.
I agree with you that non-interactive function that modifies variables and modifies a buffer should check for buffer being read-only before proceeding.
@ Fuco1, I modified the ticket title to properly describe what I reported. I am advocating using this defensive-programming utility that prevents interactive function to operate on a read-only buffer. I would do it but if you prefer leaving your code as is then it's your call.
@ThibautVerron , the reason non-interactive functions do not have checks for read-only buffers might be performance. However a large chunck of code is called via interactive functions. If all these interactive function take advantage of the '*' specification that makes the function explicitly check for the buffer being read-only then the probability of preventing execution of non-interactive code that should check for buffer being read-only and does not is reduced.
I agree with you that non-interactive function that modifies variables and modifies a buffer should check for buffer being read-only before proceeding.
I didn't mean (only) adding non-interactive functions. An interactive function can still be called non-interactively, in which case the interactive form will not be evaluated.
For a (stupid) example, I could write a function like this:
(defun my-function ()
(interactive)
(sp-delete-sexp)
(sp-forward-sexp))
and if I call this function interactively, the interactive spec of sp-delete-sexp will never be evaluated.
There are two errors there of course, I should add the star to the interactive spec, and/or I should use call-interactively. But those are not mistakes you would usually expect to bite you.
Afaik the only solution for those cases is to identify those dangerous functions, document the danger and have a hard check. It's not an argument for or against using the interactive specification everywhere, of course.
@ThibautVerron You are correct: an interactive spec is only checked for the function called interactively, either from the user action or from a call-interactively call, and not from a direct call. It would be nice this last case was handled, but it's not. From what I understand of the implementation that's because it's the interactive and call-interactive code that does the check.
Emacs Lisp is a dynamic language and that's one of its protection mechanisms. It's not protecting in 100% of the use cases, but it's useful.
I use all of these protection mechanism as much as I can. I know it's not enough. For dynamic languages a good unit test coverage also helps.
The problem is often not when the code is written but later when maintenance, refactoring or plain changes are done by the original author or someone else. Anything (including valid, up-to-date docs) that reduces probability of error in the long term is a good thing.