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

Improve Strict Equality To Account For Named Closures #852

Closed
rmorshea opened this issue Dec 8, 2022 · 4 comments · Fixed by #1256
Closed

Improve Strict Equality To Account For Named Closures #852

rmorshea opened this issue Dec 8, 2022 · 4 comments · Fixed by #1256
Labels
priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior

Comments

@rmorshea
Copy link
Collaborator

rmorshea commented Dec 8, 2022

Current Situation

Right now, strictly_equal does not understand how to check if named closures are the same. Here's an example of such a closure:

def add(first):
    def inner(second):
        return first + second
    return inner

incr = add(1)
assert incr(2) == 3

Here, while add(1) is not add(1) both produce the same behavior, IDOM won't recognize that they're the same.

Proposed Actions

It turns out that we can fairly reliable look at a function's __qualname__, __closure__, and __defaults__ to determine whether it's the same function. The logic to check this would look like:

def function_is_strictly_equal(f1, f2):
    return (
        f1.__qualname__ == f2.__qualname__
        and "<lamba>" not in f1.__qualname__
        and all(strictly_equal(c1, c2) for c1, c2 in zip(f1.__closure__, f2.__closure__))
        and all(strictly_equal(c1, c2) for c1, c2 in zip(f1.__defaults__, f2.__defaults__))
    )

The catch here is that technically, a user could do the following:

def make_closures(x):
    def do_something(y): ...
    do_something_else = do_something

    def do_something(z): ...

    return do_something, do_something_else

f1, f2 = make_closures()
assert not function_is_strictly_equal(f1, f2)  # will fail

This will fail because both functions, while they may implement different logic, have the same qualname. This is basically the same reason that we cannot compare lambdas. Since they all have the same name.

There may be ways to work around this. For example, f1 and f1 were defined on different lines. You could check this using __code__.co_firstlineno.

@rmorshea rmorshea added the flag-triage Not prioritized. label Dec 8, 2022
@Archmonger
Copy link
Contributor

Alternatively, for function defs we can check if their code is equal using string equality

@rmorshea
Copy link
Collaborator Author

rmorshea commented Dec 8, 2022

I think we could probably skip the qualname check and allow lambda comparisons by, as you say, just checking byte code equivalence.

@Archmonger
Copy link
Contributor

Archmonger commented Dec 8, 2022

I would think the same applies to named closures as well. This would make the implementation for all function types the same.

But to be bullet proof, would need to also check if the __file__ and lineno are also equivalent, since technically the same function definition can be copy pasted with potentially different decorators.

@rmorshea
Copy link
Collaborator Author

rmorshea commented Dec 8, 2022

True. Though technically, all we really care about is whether, given the same inputs, the output is the same (assuming the function is pure). The file name then, only matters in so far as there might be different globals that the function is referencing.

@Archmonger Archmonger added priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior and removed flag-triage Not prioritized. labels Dec 30, 2022
@Archmonger Archmonger added this to the 2.0 milestone Dec 30, 2022
@Archmonger Archmonger modified the milestones: Luxury, Essential Jan 29, 2023
@rmorshea rmorshea removed this from the Essential milestone Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-moderate Should be resolved on a reasonable timeline. type-revision About a change in functionality or behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants