bug#55174: python-shell-send-statement in python.el sends malformed code

bug#55174: python-shell-send-statement in python.el sends malformed code

[Jin Choi]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

python-shell-send-statement mangles the sent code if the region is inactive and the current statement is indented. Example: with the indented statement

	a = 1

it will send the string “if True:” by itself. With a multi-line statement such as
	a = (1,
		2)

it will send
if True:
	2)

The first line is always truncated.

I’ve looked a little into what is happening, and it looks like python-shell-buffer-substring has seen a number of edits over time that don’t work together properly.

* python-shell-send-statement works properly if the region is active because it calls python-shell-send-region with no-cookie set to nil, but when it is not, it calls python-shell-send-region with no-cookie set to t.
* python-shell-send-region calls python-shell-buffer-substring, passing along no-cookie.
* python-shell-buffer-substring does the following:
	- the start position is adjusted to the beginning of the line if the statement was indented
	- if no-cookie is false, fillstr is set to be a coding cookie (e.g., “# -*- coding: utf-8 -*-“) and a number of newlines to get the statement to the correct line
	- Then, this code appears:

    (with-temp-buffer
      (python-mode)
      (when fillstr
        (insert fillstr)) ; inserts coding cookie and newlines if not no-cookie
      (insert substring) ; inserts code
      (goto-char (point-min))
      (when (not toplevel-p)
        (insert "if True:")
        (delete-region (point) (line-end-position))) ; inserts “if True:” and deletes the rest of the first line

This works when no-cookie is false, because it *deletes the cookie line*, incidentally negating any benefit the coding cookie was supposed to provide. It fails when an indented statement is sent and no-cookie is true, because the first line IS the line to be sent. Either it gets deleted entirely, or if it is a multiline statement, the first line is replaced with the “if True:”.

Note that setting no-cookie has the added effect of removing the newlines for the line number matching.

I don’t know what use case the no-cookie argument was supposed to address, but the current implementation seems incompatible with how it’s being used with indented statements.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4374 bytes --]

bug#55174: python-shell-send-statement in python.el sends malformed code

[Lars Ingebrigtsen]

Jin Choi <jsc@alum.mit.edu> writes:

> This works when no-cookie is false, because it *deletes the cookie
> line*, incidentally negating any benefit the coding cookie was
> supposed to provide. It fails when an indented statement is sent and
> no-cookie is true, because the first line IS the line to be
> sent. Either it gets deleted entirely, or if it is a multiline
> statement, the first line is replaced with the “if True:”.

You don't say what Emacs version this is about?

Anyway, if it's about a current Emacs, could you propose a patch to fix
these issues?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55174: python-shell-send-statement in python.el sends malformed code

[Jin Choi]

[-- Attachment #1: Type: text/plain, Size: 3149 bytes --]

On Apr 29, 2022, at 6:21 AM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Jin Choi <jsc@alum.mit.edu> writes:
> 
>> This works when no-cookie is false, because it *deletes the cookie
>> line*, incidentally negating any benefit the coding cookie was
>> supposed to provide. It fails when an indented statement is sent and
>> no-cookie is true, because the first line IS the line to be
>> sent. Either it gets deleted entirely, or if it is a multiline
>> statement, the first line is replaced with the “if True:”.
> 
> You don't say what Emacs version this is about?
> 
> Anyway, if it's about a current Emacs, could you propose a patch to fix
> these issues?
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
> bloggy blog: http://lars.ingebrigtsen.no

This is in emacs 28.1.

Here is my take on a proposed fix. I would also remove the ’t’ no-cookie argument to python-shell-send-region in python-shell-send-statement at line 3378, to have it behave identically in both branches of the if statement, but I have left that alone because I don’t understand the rationale.

This patch:
1. Separates the semantics of no-cookie and fillstr; the code to be sent is always placed at the same line number where it appears in the file.
2. If no-cookie is not specified, a coding cookie is added, otherwise only newlines are used. If the code begins on the first line, there is no room for a coding cookie and one will not be inserted.
3. The “if True:” line is placed on the line before the code sent begins, to avoid as much conflict with the first line as possible. It will error if an indented line is the first line of the file, but that is not a valid Python construct. If an indented line is sent as the second line, it will work but the coding cookie will be deleted by the “if True:”. Neither of these cases is likely.


diff -u /Users/jsc/elisp/python.el.orig /Users/jsc/elisp/python.el
--- /Users/jsc/elisp/python.el.orig	2022-04-28 14:30:09.000000000 -0400
+++ /Users/jsc/elisp/python.el	2022-04-29 11:21:17.000000000 -0400
@@ -3284,22 +3284,25 @@
(goto-char start)
(python-util-forward-comment 1)
(current-indentation))))
- (fillstr (and (not no-cookie)
- (not starts-at-point-min-p)
- (concat
- (format "# -*- coding: %s -*-\n" encoding)
- (make-string
- ;; Subtract 2 because of the coding cookie.
- (- (line-number-at-pos start) 2) ?\n)))))
+ (fillstr (cond (starts-at-point-min-p
+ nil)
+ ((not no-cookie)
+ (concat
+ (format "# -*- coding: %s -*-\n" encoding)
+ (make-string
+ ;; Subtract 2 because of the coding cookie.
+ (- (line-number-at-pos start) 2) ?\n)))
+ (t
+ (make-string (- (line-number-at-pos start) 1) ?\n)))))
(with-temp-buffer
(python-mode)
(when fillstr
(insert fillstr))
- (insert substring)
- (goto-char (point-min))
(when (not toplevel-p)
- (insert "if True:")
+ (forward-line -1)
+ (insert "if True:\n")
(delete-region (point) (line-end-position)))
+ (insert substring)
(when nomain
(let* ((if-name-main-start-end
(and nomain

Diff finished. Fri Apr 29 11:21:22 2022

(Sorry, original not cc’ed to the bugs list)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4374 bytes --]

bug#55174: python-shell-send-statement in python.el sends malformed code

[Jin Choi]

[-- Attachment #1.1: Type: text/plain, Size: 85 bytes --]

Sigh. I’m sorry, that patch got horribly deformed. Here it is as an attachment:


[-- Attachment #1.2: patch.diff --]
[-- Type: application/octet-stream, Size: 1757 bytes --]

diff -u /Users/jsc/elisp/python.el.orig /Users/jsc/elisp/python.el
--- /Users/jsc/elisp/python.el.orig	2022-04-28 14:30:09.000000000 -0400
+++ /Users/jsc/elisp/python.el	2022-04-29 11:21:17.000000000 -0400
@@ -3284,22 +3284,25 @@
                               (goto-char start)
                               (python-util-forward-comment 1)
                               (current-indentation))))
-         (fillstr (and (not no-cookie)
-                       (not starts-at-point-min-p)
-                       (concat
-                        (format "# -*- coding: %s -*-\n" encoding)
-                        (make-string
-                         ;; Subtract 2 because of the coding cookie.
-                         (- (line-number-at-pos start) 2) ?\n)))))
+         (fillstr (cond (starts-at-point-min-p
+                         nil)
+                        ((not no-cookie)
+                         (concat
+                          (format "# -*- coding: %s -*-\n" encoding)
+                          (make-string
+                           ;; Subtract 2 because of the coding cookie.
+                           (- (line-number-at-pos start) 2) ?\n)))
+                        (t
+                         (make-string (- (line-number-at-pos start) 1) ?\n)))))
     (with-temp-buffer
       (python-mode)
       (when fillstr
         (insert fillstr))
-      (insert substring)
-      (goto-char (point-min))
       (when (not toplevel-p)
-        (insert "if True:")
+        (forward-line -1)
+        (insert "if True:\n")
         (delete-region (point) (line-end-position)))
+      (insert substring)
       (when nomain
         (let* ((if-name-main-start-end
                 (and nomain

Diff finished.  Fri Apr 29 11:21:22 2022

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4374 bytes --]

bug#55174: python-shell-send-statement in python.el sends malformed code

[Lars Ingebrigtsen]

Jin Choi <jsc@alum.mit.edu> writes:

> Sigh. I’m sorry, that patch got horribly deformed. Here it is as an
> attachment:

Makes sense to me, and after testing a bit, it seems to work well, so
I've pushed it to Emacs 29.

This change was small enough to apply without assigning copyright to the
FSF, but for future patches you want to submit, it might make sense to
get the paperwork started now, so that subsequent patches can be applied
speedily. Would you be willing to sign such paperwork?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no