Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-evaluated Filter equality lost in 3.0.37 #1729

Closed
Carreau opened this issue Feb 27, 2023 · 3 comments
Closed

Non-evaluated Filter equality lost in 3.0.37 #1729

Carreau opened this issue Feb 27, 2023 · 3 comments

Comments

@Carreau
Copy link
Contributor

Carreau commented Feb 27, 2023

I think that 7776bf9 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
Copy link
Member

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
Copy link
Member

@Carreau: Can you test whether this PR fixes the issue for you: #1730 ? (I'm improving a few other things there as well + added a regression test.)

@Carreau
Copy link
Contributor Author

Carreau commented Feb 28, 2023

Closing as completed. Thanks !

@Carreau Carreau closed this as completed Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants