prompt-toolkit / python-prompt-toolkit

Library for building powerful interactive command line applications in Python
https://python-prompt-toolkit.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
9.11k stars 718 forks source link

Non-evaluated Filter equality lost in 3.0.37 #1729

Closed Carreau closed 1 year ago

Carreau commented 1 year ago

I think that 7776bf9b737768eb479910d20b88cdcf5343a3a6 lead to some weird things in filter equality.


In [6]: from prompt_toolkit.filters import Condition, Filter
   ...:
   ...: # make sure not the same Filter
   ...: a = Condition(lambda: True)
   ...:
   ...:
   ...: f3x = (a & a) & a
   ...: f3y = (a & a) & a
   ...: #print(fx.filters)
   ...: #print(fy.filters)
   ...: print(f3x == f3y). # False !
   ...:
   ...: f2x = a & a
   ...: f2y = a & a
   ...: #print(fx.filters)
   ...: #print(fy.filters)
   ...: print(f2x == f2y) # True.
False
True

Note that this is not when you evaluate the filter, but looking as wether the filter are identical. We recently in IPython landed a patch from @krassowski that need to do str <-> Filter (for configurability) and compare. That may be the wrong things to do but at least it surfaced this.

I believe there are interning questions due to upstream, and having a custom __eq__ in _AndList and _OrList seem to fix it, but I wanted to open an issue first.

jonathanslenders commented 1 year ago

Hi, thanks for reporting!

It looks like the problem is that the intermediate result of (a&a) is not referred to anymore when the whole expression is evaluated, due to the whole expression being collapsed into one _AndList. The cache is a WeakValueDictionary and because of that it disappears from the cache.

I wonder whether the memory leak that I fixed back then returns if this would be a normal dict. I have to check that.

jonathanslenders commented 1 year ago

@Carreau: Can you test whether this PR fixes the issue for you: https://github.com/prompt-toolkit/python-prompt-toolkit/pull/1730 ? (I'm improving a few other things there as well + added a regression test.)

Carreau commented 1 year ago

Closing as completed. Thanks !