Sympy incorrectly attempts to eval reprs in its __eq__ method

[Strilanc]

Passing strings produced by unknown objects into eval is very bad. It is especially surprising for an equality check to trigger that kind of behavior. This should be fixed ASAP.

Repro code:

import sympy
class C:
    def __repr__(self):
        return 'x.y'
_ = sympy.Symbol('x') == C()

Results in:

E   AttributeError: 'Symbol' object has no attribute 'y'

On the line:

    expr = eval(
        code, global_dict, local_dict)  # take local objects in preference

Where code is:

Symbol ('x' ).y

Full trace:

FAILED                   [100%]
        class C:
            def __repr__(self):
                return 'x.y'
    
>       _ = sympy.Symbol('x') == C()

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
sympy/core/expr.py:124: in __eq__
    other = sympify(other)
sympy/core/sympify.py:385: in sympify
    expr = parse_expr(a, local_dict=locals, transformations=transformations, evaluate=evaluate)
sympy/parsing/sympy_parser.py:1011: in parse_expr
    return eval_expr(code, local_dict, global_dict)
sympy/parsing/sympy_parser.py:906: in eval_expr
    code, global_dict, local_dict)  # take local objects in preference
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   AttributeError: 'Symbol' object has no attribute 'y'

<string>:1: AttributeError

Related issue: an unknown object whose repr is x will incorrectly compare as equal to a sympy symbol x:

    class C:
        def __repr__(self):
            return 'x'

    assert sympy.Symbol('x') != C()  # fails

[oscarbenjamin]

See also #12524

[asmeurer]

Safe flag or no, == should call _sympify since an expression shouldn't equal a string.

I also think we should deprecate the string fallback in sympify. It has led to serious performance issues in the past and clearly has security issues as well.

[asmeurer]

Actually, it looks like we also have

>>> x == 'x'
True

which is a major regression since 1.4.

I bisected it to 73caef3.

The bug in the issue doesn't exist in 1.4 either. So we could consider doing a 1.5.1 release fixing this.

[asmeurer]

The thing is, I could have swore this behavior was tested. But I don't see anything in the test changes from #16924 about string comparisons.

[asmeurer]

I suspect this change can also lead to major performance issues as well.

[asmeurer]

Yeah, there is.

SymPy 1.4

In [1]: a = list(map(str, range(1000)))

In [2]: %timeit x in a
2.57 ms ± 96.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

SymPy 1.5

In [1]: a = list(map(str, range(1000)))

In [2]: %timeit x in a
297 ms ± 7.51 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

So we should probably do a patch release. @oscarbenjamin

[oscarbenjamin]

So we should probably do a patch release. @oscarbenjamin

Agreed.

[smichr]

I don't see anything in the test changes

I'm pretty sure the author had no appreciation for theses implications when adding the code ;-)

[asmeurer]

Well we need to deprecate the string fallback in sympify. This isn't the first time it's bitten performance, and I wouldn't be surprised if there are latent issues with it elsewhere. We also should phase out the use of strict=False sympify() in library code (i.e., using sympify() instead of _sympify()), except in a few key places where we really do want to allow string inputs.

[sylee957]

I think that default_sort_key internally calls sympify (possibly with string fallback).