bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when eldoc is enabled

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when eldoc is enabled

[Carlos Pita]

Simply open the font lock buffer *Python-font-lock* while keeping an
inferior mode python buffer at its side. Now start typing, wait a couple
of seconds and type another character, play like this for a while and
you will find out that many new lines are added to the buffer and that
the current line is replicated between those empty lines. This is bad by
itself, but the symptom that made me realize of the problem is that
unclosed strings pair with their replicas in other lines and hence get
quite a random fontification.

Easier: enter the font lock buffer and scroll to the top. Maybe write
something. Sooner than later the buffer will scroll down by
itself. Delete everything, add a character, wait a moment, newlines
appear below the character.

I started disabling minor modes one by one until I found out that the
culprit was eldoc. I haven't investigated further but I believe that
just disabling eldoc there would be fine, don't you?

I will submit a patch in a moment.

Btw, I've submitted a patch for another font lock buffer related bug [1]
a time ago and I might be wrong but I got the impression that python.el
is rather forgotten these days. I wouldn't mind becoming a maintainer at
all, or helping with maintenance, if necessary.

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32390

Best regards
--
Carlos

---------------------------------------

In GNU Emacs 26.1.90 (build 4, x86_64-pc-linux-gnu, GTK+ Version 3.24.1)
 of 2018-12-05 built on archlinux
Repository revision: c7897c27861fd8b2690f40e77402edced6aa0ceb
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
Recent messages:
delete-backward-char: Beginning of buffer
Winner mode disabled
delete-backward-char: Beginning of buffer
Xclip mode disabled
Invalid byte code in /home/carlos/local/stow/emacs-26/share/emacs/26.1.90/lisp/emacs-lisp/cl-extra.elc
Show-Paren mode disabled
Ido-Ubiquitous mode disabled
Invalid byte code in /home/carlos/local/stow/emacs-26/share/emacs/26.1.90/lisp/emacs-lisp/cl-extra.elc
Eldoc mode disabled in current buffer
delete-backward-char: Text is read-only

Configured using:
 'configure --prefix=/home/carlos/local/stow/emacs-26
 --libexecdir=/home/carlos/local/stow/emacs-26/lib --with-x-toolkit=gtk3
 --with-xft --with-modules'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS GLIB
NOTIFY ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Help

Minor modes in effect:
  semantic-minor-modes-format: ((:eval (if (or semantic-highlight-edits-mode semantic-show-unmatched-syntax-mode)  S)))
  global-magit-file-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  pdf-occur-global-minor-mode: t
  diff-auto-refine-mode: t
  pyvenv-mode: t
  shell-dirtrack-mode: t
  ido-everywhere: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail pulse cl-print edebug misearch
multi-isearch magit-extras magit-bookmark magit-submodule magit-obsolete
magit-blame magit-stash magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-collab ghub-graphql treepy graphql ghub url-http tls gnutls url-gw
nsm url-auth url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap magit-files
magit-refs magit-status magit magit-repos magit-apply magit-wip
magit-log which-func magit-diff smerge-mode magit-core magit-autorevert
autorevert filenotify magit-process magit-margin magit-mode git-commit
magit-git magit-section magit-utils magit-popup crm log-edit message rmc
puny rfc822 mml mml-sec epa derived epg mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader pcvs-util add-log
with-editor async-bytecomp async dash autoload lisp-mnt checkdoc
whitespace visual-fill-column face-remap vc-git term/xterm xterm server
projectile pdf-occur ibuf-ext ibuffer ibuffer-loaddefs tablist
tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet dired
dired-loaddefs pdf-isearch let-alist pdf-misc imenu pdf-tools pdf-view
bookmark pp jka-compr pdf-cache pdf-info tq pdf-util image-mode ox-md
ox-html table ox-beamer ox-latex ox-ascii ox-publish ox org-capture
org-protocol org-element avl-tree generator org org-macro org-footnote
org-pcomplete org-list org-faces org-entities noutline outline
org-version ob-python ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob
ob-table ob-keys ob-exp ob-comint ob-core ob-eval org-compat org-macs
org-loaddefs find-func cal-menu calendar cal-loaddefs cl-extra yasnippet
elec-pair highlight-indentation flymake-proc flymake warnings help-fns
radix-tree help-mode elpy find-file-in-project ivy delsel colir color
ivy-overlay ffap thingatpt windmove diff-mode elpy-shell pyvenv esh-var
esh-io esh-cmd esh-opt esh-ext esh-proc esh-arg esh-groups eshell
esh-module esh-mode esh-util elpy-profile elpy-django elpy-refactor
subr-x python tramp-sh tramp tramp-compat tramp-loaddefs trampver
ucs-normalize shell pcomplete parse-time format-spec advice json map
grep compile comint ansi-color files-x easy-mmode doom-themes-org
company-oddmuse company-keywords company-etags etags xref project
company-gtags company-dabbrev-code company-dabbrev company-files
company-capf company-cmake company-xcode company-clang company-semantic
company-eclim company-template company-bbdb doom-tomorrow-night-theme
doom-themes doom-themes-common xclip winner ring paren
ido-completing-read+ memoize s cus-edit minibuf-eldef ido gnus nnheader
gnus-util rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums time-date
mail-utils mm-util mail-prsvr wid-edit company edmacro kmacro pcase
cus-start cus-load finder-inf info package easymenu epg-config
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 1011425 82213)
 (symbols 48 51276 0)
 (miscs 40 1854 1775)
 (strings 32 170805 3260)
 (string-bytes 1 4865573)
 (vectors 16 88778)
 (vector-slots 8 2035058 160578)
 (floats 8 582 818)
 (intervals 56 6500 1583)
 (buffers 992 36))

bug#33959:

[Carlos Pita]

I've changed the subject because the problem seems to be company and
not eldoc. It's weird since my first suspicion was about company menu
opening and inserting those empty lines so I turned off company local
and global minor modes to no avail... maybe I inadvertently toggle the
mode on. I will experiment a bit more until I'm sure which mode is
causing the issue, but company makes a lot of sense.

bug#33959:

[Carlos Pita]

Well, I think I was right both times but because the problem is more
complex than I thought at first.

When I was editing the font lock buffer itself, it was eldoc that was
adding an extra line.

But when I was editing the shell buffer line, it was company that was
adding those lines to the font lock buffer (probably because of a
weird effect of an empty menu or something invisibly opening and
adding content to the shell buffer). Btw, company is disabled in the
font lock buffer since the globalized mode is off for buffers whose
name starts with a whitespace:

(defun company-mode-on ()
  (when (and (not (or noninteractive (eq (aref (buffer-name) 0) ?\s)))
  ....

The first effect (eldoc) is simple to fix by just disabling eldoc in
the font lock buffer, but this is not even necessary since it requires
the user to directly edit the font lock buffer in order to trigger it,
and this is not a relevant use case.

For the second effect (company) I propose to delete from the beginning
of buffer each time instead of from the beginning of line, since the
font lock buffer won't contain multiline input in any case. Currently
after each change in the current line this is being done:

     (delete-region (line-beginning-position) (point-max))

but I see no reason to not call (erase-buffer) altogether.

Do you?

bug#33959:

[Carlos Pita]

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

> but I see no reason to not call (erase-buffer) altogether.
>
> Do you?

Yes, obviously, multiline input, I forgot.

Nevertheless I found the origin of the problem. Somehow when company
is enabled in the shell buffer
python-shell-font-lock-comint-output-filter-function is getting empty
strings as output and then the ";; Otherwise just add a newline."
clause is activated.

No matter the reason why empty output is being passed to the filter,
it's wrong for the filter to add a new line to the font lock buffer if
this happens.

I'm attaching two patches, since
python-shell-font-lock-comint-output-filter-function is an old friend
of mine and we have spent many nights together now:

1. The first one (Avoid-spurious...) strictly fixes this issue by
moving the not-empty condition to the top and doing everything else
unless not-empty. Period. That works.

2. The second one (Fix-font-lock...) combines 1 with my previous patch
for fixing ipython multiline input (#32390) since both changes
overlap.

I suggest to directly apply patch 2 since it improves prompt detection
heuristic and fixes two bugs. Then you can close this and #32390.

Regards
--
Carlos

[-- Attachment #2: 0001-Avoid-spurious-empty-lines-in-font-lock-buffer-fixes.patch --]
[-- Type: text/x-patch, Size: 2290 bytes --]

From d816a29f5a9cdcebd8f69ad0a6555eaca051587e Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Thu, 3 Jan 2019 02:28:30 -0300
Subject: [PATCH] Avoid spurious empty lines in font lock buffer, fixes #33959

---
 lisp/progmodes/python.el | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 27d31ab..81a6e89 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2556,19 +2556,19 @@ python-shell-font-lock-cleanup-buffer
 
 (defun python-shell-font-lock-comint-output-filter-function (output)
   "Clean up the font-lock buffer after any OUTPUT."
-  (if (and (not (string= "" output))
-           ;; Is end of output and is not just a prompt.
-           (not (member
-                 (python-shell-comint-end-of-output-p
-                  (ansi-color-filter-apply output))
-                 '(nil 0))))
-      ;; If output is other than an input prompt then "real" output has
-      ;; been received and the font-lock buffer must be cleaned up.
-      (python-shell-font-lock-cleanup-buffer)
-    ;; Otherwise just add a newline.
-    (python-shell-font-lock-with-font-lock-buffer
-      (goto-char (point-max))
-      (newline)))
+  (unless (string= "" output)
+    (if ;; Is end of output and is not just a prompt.
+        (not (member
+              (python-shell-comint-end-of-output-p
+               (ansi-color-filter-apply output))
+              '(nil 0)))
+        ;; If output is other than an input prompt then "real" output has
+        ;; been received and the font-lock buffer must be cleaned up.
+        (python-shell-font-lock-cleanup-buffer)
+      ;; Otherwise just add a newline.
+      (python-shell-font-lock-with-font-lock-buffer
+        (goto-char (point-max))
+        (newline))))
   output)
 
 (defun python-shell-font-lock-post-command-hook ()
@@ -2597,6 +2597,7 @@ python-shell-font-lock-post-command-hook
                                   (point-max))))
              (replacement-length (length replacement))
              (i 0))
+
         ;; Inject text properties to get input fontified.
         (while (not (= i replacement-length))
           (let* ((plist (text-properties-at i replacement))
-- 
2.20.1


[-- Attachment #3: 0001-Fix-font-lock-output-filter-bugs-33959-and-32390.patch --]
[-- Type: text/x-patch, Size: 2484 bytes --]

From dc80df310933f9f5f9b9bf065e72ee79a6b16134 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Thu, 3 Jan 2019 02:31:52 -0300
Subject: [PATCH] Fix font lock output filter bugs #33959 and #32390

Improves multiline input detection heuristic.
Fixes multiline input for ipython.
Avoid adding spurious empty lines to the font lock buffer.
Filter ansi color from output before matching.
---
 lisp/progmodes/python.el | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 27d31ab..e98d88e 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2556,20 +2556,18 @@ python-shell-font-lock-cleanup-buffer
 
 (defun python-shell-font-lock-comint-output-filter-function (output)
   "Clean up the font-lock buffer after any OUTPUT."
-  (if (and (not (string= "" output))
-           ;; Is end of output and is not just a prompt.
-           (not (member
-                 (python-shell-comint-end-of-output-p
-                  (ansi-color-filter-apply output))
-                 '(nil 0))))
-      ;; If output is other than an input prompt then "real" output has
-      ;; been received and the font-lock buffer must be cleaned up.
-      (python-shell-font-lock-cleanup-buffer)
-    ;; Otherwise just add a newline.
-    (python-shell-font-lock-with-font-lock-buffer
-      (goto-char (point-max))
-      (newline)))
-  output)
+  (unless (string= output "")
+    (if (let ((output (ansi-color-filter-apply output)))
+          (and (python-shell-comint-end-of-output-p output)
+               (not (string-match "\\.\\.\\." output))))
+        ;; If output ends with an initial (not continuation) input prompt
+        ;; then the font-lock buffer must be cleaned up.
+        (python-shell-font-lock-cleanup-buffer)
+      ;; Otherwise just add a newline.
+      (python-shell-font-lock-with-font-lock-buffer
+        (goto-char (point-max))
+        (newline)))
+    output))
 
 (defun python-shell-font-lock-post-command-hook ()
   "Fontifies current line in shell buffer."
@@ -2597,6 +2595,7 @@ python-shell-font-lock-post-command-hook
                                   (point-max))))
              (replacement-length (length replacement))
              (i 0))
+
         ;; Inject text properties to get input fontified.
         (while (not (= i replacement-length))
           (let* ((plist (text-properties-at i replacement))
-- 
2.20.1

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Carlos Pita <carlosjosepita@gmail.com> writes:

> Nevertheless I found the origin of the problem. Somehow when company
> is enabled in the shell buffer
> python-shell-font-lock-comint-output-filter-function is getting empty
> strings as output and then the ";; Otherwise just add a newline."
> clause is activated.

> 1. The first one (Avoid-spurious...) strictly fixes this issue by
> moving the not-empty condition to the top and doing everything else
> unless not-empty. Period. That works.

Hmm, I'm not sure if it's working here.  My *Python* buffer looks like
this:

    Python 3.7.0 (default, Sep 15 2018, 19:13:07) 
    Type 'copyright', 'credits' or 'license' for more information
    IPython 6.5.0 -- An enhanced Interactive Python. Type '?' for help.

    In [1]: 1 + len('123') + 99 + len('aa')
    In [13]: 1 + len('123') + 99 + len('aa')
    Out[13]: 105

    In [14]: 1 + len('123') + 99 + len('aa')
    In [14]: 1 + len('123') + 99 + len('aa')
    Out[14]: 105

    In [15]: 1 + len('123') + 99 + len('aa')
    In [21]: 1 + len('123') + 99 + len('aa')
    Out[21]: 105

    In [22]: 

And my " *Python-font-lock*" looks like this:


    1 + len('123') + 99 + len('aa')



    1 + len(
    1 + len('123'
    1 + len('123') + 99 + len('aa')

The behaviour seems rather inconsistent.  I think to solve this properly
we need some deterministic tests which reproduce the problem.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when eldoc is enabled

[Noam Postavsky]

Carlos Pita <carlosjosepita@gmail.com> writes:

> Btw, I've submitted a patch for another font lock buffer related bug [1]
> a time ago and I might be wrong but I got the impression that python.el
> is rather forgotten these days. I wouldn't mind becoming a maintainer at
> all, or helping with maintenance, if necessary.

Yeah, there's no maintainer for python.el, so maintenance is sporadic at
best.  I think having one would definitely be a big help.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when eldoc is enabled

[Carlos Pita]

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

Hi Noam,

Yeah, there's no maintainer for python.el, so maintenance is sporadic at
> best.  I think having one would definitely be a big help.
>

I would like to know your opinion regarding the approach that was suggested
near the end of https://github.com/jorgenschaefer/elpy/issues/1531 in order
to avoid direct access to python.el yet keep the ability to port changes.
Essentially, it consists in keeping a more active drop-in python.el as an
external package with periodic updates to core python.el once the changes
were more or less tested by real world usage.

[-- Attachment #2: Type: text/html, Size: 896 bytes --]

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

Hi Noam,

> And my " *Python-font-lock*" looks like this:
>
>
>     1 + len('123') + 99 + len('aa')
>
>
>
>     1 + len(
>     1 + len('123'
>     1 + len('123') + 99 + len('aa')
>
> The behaviour seems rather inconsistent.  I think to solve this properly
> we need some deterministic tests which reproduce the problem.


The state of your " *Python-font-lock*" buffer is exactly what I mean.
The duplications you see are caused by spurious newlines added when
empty output is passed to the filter. There are two bad things there:
i) empty strings are transformed into new lines and 2) empty strings
are an undesirable side effect of company being triggered in a hidden
buffer (this is analogous to the problem with org mode hidden buffer
and yasnippet that I recently reported). Try disabling company mode in
python font lock buffer; even though, this is not necessary if (i)
above is fixed, which IMO should be done independently of the nasty
company interaction.

Besides, remember that patch 2 is fixing two different issues. This is
somewhat undesirable but there are a lot of overlapping between them.
Nevertheless, I provided patch 1 in case you are unwilling to apply
one of them.

Best regards
--
Carlos

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Carlos Pita <carlosjosepita@gmail.com> writes:

> Hi Noam,
>
>> And my " *Python-font-lock*" looks like this:
>>
>>
>>     1 + len('123') + 99 + len('aa')
>>
>>
>>
>>     1 + len(
>>     1 + len('123'
>>     1 + len('123') + 99 + len('aa')
>>
>> The behaviour seems rather inconsistent.  I think to solve this properly
>> we need some deterministic tests which reproduce the problem.
>
>
> The state of your " *Python-font-lock*" buffer is exactly what I mean.
> The duplications you see are caused by spurious newlines added when
> empty output is passed to the filter. There are two bad things there:
> i) empty strings are transformed into new lines and 2) empty strings
> are an undesirable side effect of company being triggered in a hidden
> buffer (this is analogous to the problem with org mode hidden buffer
> and yasnippet that I recently reported). Try disabling company mode in
> python font lock buffer; even though, this is not necessary if (i)
> above is fixed, which IMO should be done independently of the nasty
> company interaction.
>
> Besides, remember that patch 2 is fixing two different issues. This is
> somewhat undesirable but there are a lot of overlapping between them.
> Nevertheless, I provided patch 1 in case you are unwilling to apply
> one of them.

The above state happens after I've applied your patch #2.  So I can't
tell if it fixes anything.  That's why I think we need some
deterministic tests to show what is fixed and what is broken.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

> The above state happens after I've applied your patch #2.

Ah, ok! I hadn't understood that was *after* applying the patch. Let
me try to reproduce your example then.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

I'm unable to get your output, here the font lock buffer always
contains one line.

Nevertheless, I don't quite understand your example. Here:

In [14]: 1 + len('123') + 99 + len('aa')
In [14]: 1 + len('123') + 99 + len('aa')
Out[14]: 105

How do you manage to have two input lines with the same prompt number?
Is that an artifact of copy pasting from the REPL? If it is, if your
example just consists of successively sending the line `1 + len('123')
+ 99 + len('aa')` many times, I'm unable to reproduce the case (after
my patch is applied, that is, with this definition

(defun python-shell-font-lock-comint-output-filter-function (output)
  "Clean up the font-lock buffer after any OUTPUT."
  (unless (string= output "")
    (if (let ((output (ansi-color-filter-apply output)))
          (and (python-shell-comint-end-of-output-p output)
               (not (string-match "\\.\\.\\." output))))
        ;; If output ends with an initial (not continuation) input prompt
        ;; then the font-lock buffer must be cleaned up.
        (python-shell-font-lock-cleanup-buffer)
      ;; Otherwise just add a newline.
      (python-shell-font-lock-with-font-lock-buffer
        (goto-char (point-max))
        (newline)))
    output))
)

On Tue, Apr 16, 2019 at 7:08 PM Carlos Pita <carlosjosepita@gmail.com> wrote:
>
> > The above state happens after I've applied your patch #2.
>
> Ah, ok! I hadn't understood that was *after* applying the patch. Let
> me try to reproduce your example then.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Carlos Pita <carlosjosepita@gmail.com> writes:

> I'm unable to get your output, here the font lock buffer always
> contains one line.

Hmm, it might have to do with the fact that I'm running ipython over
TRAMP, since my main box doesn't have IPython 6.x.  But I suspect that
only affects the timing, so that it's still theoretically possible
(although less likely) for it to happen when running locally.

> Nevertheless, I don't quite understand your example. Here:
>
> In [14]: 1 + len('123') + 99 + len('aa')
> In [14]: 1 + len('123') + 99 + len('aa')
> Out[14]: 105
>
> How do you manage to have two input lines with the same prompt number?

I don't know, I didn't realize it was abnormal.  As far as I can tell,
the prompt number increases by a random amount (sometimes 0) each time I
press enter.

> Is that an artifact of copy pasting from the REPL?

No, that's the real text of my *Python* buffer, unedited.

> if your example just consists of successively sending the line `1 +
> len('123') + 99 + len('aa')` many times, I'm unable to reproduce the
> case (after my patch is applied, that is, with this definition

My example consists of successively *typing* the line "1 + len('123') +
 99 + len('aa')", it usually takes about 3 tries before I see trouble.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Noam Postavsky <npostavs@gmail.com> writes:

> Carlos Pita <carlosjosepita@gmail.com> writes:
>
>> I'm unable to get your output, here the font lock buffer always
>> contains one line.
>
> Hmm, it might have to do with the fact that I'm running ipython over
> TRAMP, since my main box doesn't have IPython 6.x.  But I suspect that
> only affects the timing, so that it's still theoretically possible
> (although less likely) for it to happen when running locally.

I installed IPython 6.5.0 locally using pip, and I still see the same
behaviour (including repeated input prompt numbers).

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Noam Postavsky <npostavs@gmail.com> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>
>> Carlos Pita <carlosjosepita@gmail.com> writes:
>>
>>> I'm unable to get your output, here the font lock buffer always
>>> contains one line.
>>
>> Hmm, it might have to do with the fact that I'm running ipython over
>> TRAMP, since my main box doesn't have IPython 6.x.  But I suspect that
>> only affects the timing, so that it's still theoretically possible
>> (although less likely) for it to happen when running locally.
>
> I installed IPython 6.5.0 locally using pip, and I still see the same
> behaviour (including repeated input prompt numbers).

I think I figured out what the difference is: I forgot to add
--simple-prompt to python-shell-interpreter-args.  After doing that,
your patch does indeed fix the problem.  I'd still like a deterministic
test, though, since this bug is such a pain to reproduce.

bug#33959:

[Carlos Pita]

Noam I'd like to get this merged (because my local patch queue is
getting too big :P )

> I'd still like a deterministic test, though, since this bug is such a pain to reproduce.

I'm not quite understanding what do you want me to do. I don't think
there is a suite of unit tests to prevent regressions in python.el and
you can't reasonably expect me to write that entire suite just to get
this minor patch accepted, so I figure you're after another thing
here. Do you want very explicit instructions that ensure
reproducibility of the issue, is that it?

Best regards
--
Carlos

bug#33959:

[Lars Ingebrigtsen]

Carlos, you seem to be dropping the text from all the Subject headers
in the emails you're sending, leaving just the bug number.  This makes
it difficult to remember what the patches are about, because you have to
open up the bug report to do so.

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

bug#33959:

[Carlos Pita]

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

Ah, ok, sorry, I don't have the old threads in my account anymore and
thought debbugs would take care of that, but you're right that CCs will get
an empty subject, my bad, sorry again.

[-- Attachment #2: Type: text/html, Size: 215 bytes --]

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

Carlos Pita <carlosjosepita@gmail.com> writes:

> Noam I'd like to get this merged (because my local patch queue is
> getting too big :P )

Heh, I know the feeling :)

>> I'd still like a deterministic test, though, since this bug is such a pain to reproduce.
>
> I'm not quite understanding what do you want me to do. I don't think
> there is a suite of unit tests to prevent regressions in python.el

Well, test/lisp/progmodes/python-tests.el exists, so yes there is?

> you can't reasonably expect me to write that entire suite just to get
> this minor patch accepted, so I figure you're after another thing
> here. Do you want very explicit instructions that ensure
> reproducibility of the issue, is that it?

Mainly I've been hesitant to apply this patch because I don't understand
why it works.  It kind of looks like it might just be papering over some
subtle problems (similar to fixing a race condition by adding a sleep
call).  And also the overlap with the Bug#32390 fix made things more
confusing; now that I've pushed it, at least that problem is gone.

Writing a regression test would be one way of showing where exactly the
problem comes from, and would help understand why the solution is
correct.

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

> And also the overlap with the Bug#32390 fix made things more
> confusing; now that I've pushed it, at least that problem is gone.

I'm in the process of creating a patch for this that applies on top of
your recent commit and it's just wrapping everything into:

   (unless (string= output "") ... )

So I believe that if you already bought to other one this is way
easier to buy :)

Even without further digging into the specifics of the issue, I'd
advanced an argument for adding that guard:

> No matter the reason why empty output is being passed to the filter,
> it's wrong for the filter to add a new line to the font lock buffer if
> this happens.

And the check for empty output was always there, but in conjunction
with a check for "just a prompt":

(if (and (not (string= "" output)) (not just-a-prompt))
       do-something
   add-a-newline)

I think it's clear this logic is faulty. Both an empty output and a
just-a-prompt output are reasons not to do-something. But only
just-a-prompt is a reason to add-a-newline.

Have I been persuasive enough now :) ?

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

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

This should cleanly apply on top of the one you recently merged.

[-- Attachment #2: 0001-Avoid-spurious-empty-lines-in-font-lock-buffer-fixes.patch --]
[-- Type: text/x-patch, Size: 2113 bytes --]

From 5d313b0bb11ec7ace8ab6b3cee7aac3345aa4692 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Mon, 14 Oct 2019 21:37:20 -0300
Subject: [PATCH] Avoid spurious empty lines in font lock buffer, fixes #33959

* lisp/progmodes/python.el: add clause to avoid writing a newline to the
  font lock buffer when an empty string is received as output.
---
 lisp/progmodes/python.el | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index b168b62..86a809b 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2600,18 +2600,19 @@ python-shell-font-lock-cleanup-buffer
 
 (defun python-shell-font-lock-comint-output-filter-function (output)
   "Clean up the font-lock buffer after any OUTPUT."
-  (if (let ((output (ansi-color-filter-apply output)))
-        (and (python-shell-comint-end-of-output-p output)
-             ;; Assume "..." represents a continuation prompt.
-             (not (string-match "\\.\\.\\." output))))
-      ;; If output ends with an initial (not continuation) input prompt
-      ;; then the font-lock buffer must be cleaned up.
-      (python-shell-font-lock-cleanup-buffer)
-    ;; Otherwise just add a newline.
-    (python-shell-font-lock-with-font-lock-buffer
-      (goto-char (point-max))
-      (newline)))
-  output)
+   (unless (string= output "")
+    (if (let ((output (ansi-color-filter-apply output)))
+          (and (python-shell-comint-end-of-output-p output)
+               ;; Assume "..." represents a continuation prompt.
+               (not (string-match "\\.\\.\\." output))))
+        ;; If output ends with an initial (not continuation) input prompt
+        ;; then the font-lock buffer must be cleaned up.
+        (python-shell-font-lock-cleanup-buffer)
+      ;; Otherwise just add a newline.
+      (python-shell-font-lock-with-font-lock-buffer
+        (goto-char (point-max))
+        (newline)))
+    output))
 
 (defun python-shell-font-lock-post-command-hook ()
   "Fontifies current line in shell buffer."
-- 
2.20.1

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

Hi Noam, I don't want to be insistent (and I'm probably being) but it
won't take much time for someone to complaint about [1] because it
removed the `(not (string= "" output))` guard from the original code.
As it is now, it's certainly buggy. I know it would be desirable to
provide an unit test for [1] but this new commit I'm providing can't
harm (remember it's just an "around advice": `(unless (string= output
"") ... )`) and I believe my argument against the way the empty output
condition was checked before is sound and simple enough. Thanks and
sorry for the insistence.

---

[1] 7acc621e3 (Fix python-shell font-lock cleanup for unclosed quotes
(Bug#32390))

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Carlos Pita]

I've improved the commit message following the guidelines described in
CONTRIBUTE.

Here is the commit
https://github.com/memeplex/emacs/commit/6792b4a7e6daad602ad92f0987bfea7499a50205

You can generate a nicely formatted patch by adding the .patch suffix
to that URL.

Best regards
--
Carlos

bug#33959: 26.1.90; python.el font-lock buffer wreaks havoc when company is enabled

[Noam Postavsky]

tags 33959 fixed
close 33959 27.1
quit

Carlos Pita <carlosjosepita@gmail.com> writes:

> Hi Noam, I don't want to be insistent (and I'm probably being)

No worries, sorry for taking so long on this.

> As it is now, it's certainly buggy. I know it would be desirable to
> provide an unit test for [1] but this new commit I'm providing can't
> harm (remember it's just an "around advice": `(unless (string= output
> "") ... )`) and I believe my argument against the way the empty output
> condition was checked before is sound and simple enough. Thanks and
> sorry for the insistence.

Looking at this now afresh, I agree that this change looks safe
enough.  I think I was confused before by the other patch removing that
same guard in the first branch of the `if'.

I do suspect that the filter function receiving an empty string is a bug
in itself though.  Meanwhile I've pushed your fix (since we'll want it
in the GNU ELPA python.el for Emacs 26 and earlier regardless of whether
the underlying process filter bug is fixed).

[2: ab67287872]: 2019-10-22 20:11:49 -0400
  Avoid extra lines in python-shell font lock buffer (Bug#33959)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ab6728787245e0d46bd8a8919e30c882f6011182