Repository: sympy/sympy

Issue:
Deprecate string fallback in sympify
Currently if everything else fails in sympify(), it takes `str(expr)` and tries to parse it. We should deprecate and remove this feature. It is problematic for a few reasons:

- It can affect performance in major ways. See for instance https://github.com/sympy/sympy/issues/18056 and https://github.com/sympy/sympy/issues/15416 where it's caused up to 100x slowdowns. The issue is that whenever a function, and hence sympify is passed something it doesn't know how to sympify, for instance, a Python function type, it passes the string to parse_expr. This is significantly slower than the direct conversions that happen by default. This occurs specifically whenever `sympify()` is used in library code instead of `_sympify()` (or equivalently `sympify(strict=True)`), but presently this is done a lot. We should ultimately stop using strict=True in library code, but this is a harder change to make.

- It can cause security issues, since strings are evaled, and objects can return whatever string they want in their `__repr__`. See also https://github.com/sympy/sympy/pull/12524.

- It really isn't very useful to begin with. Just because an object's string form can be parsed into a SymPy expression doesn't mean it should be parsed that way. This is usually correct for custom numeric types, but an object's repr could be anything. For instance, if the string form of an object looks like a valid Python identifier, it will parse as a Symbol.

There are plenty of ways to make custom objects work inside of sympify(). 

- Firstly, if an object is intended to work alongside other SymPy expressions, it should subclass from Basic (or Expr). If it does, sympify() will just return it unchanged because it will already be a valid SymPy object.

- For objects that you control, you can add the `_sympy_` method. The [sympify docstring](https://docs.sympy.org/latest/modules/core.html#sympy.core.sympify.sympify) has an example of this.

- For objects that you don't control, you can add a custom converter to the `sympy.core.sympify.converter` dictionary (the docstring also has an example of this).



Discussion:
sympy-bot: :white_check_mark:

Hi, I am the [SymPy bot](https://github.com/sympy/sympy-bot) (v158). I'm here to help you write a release notes entry. Please read the [guide on how to write release notes](https://github.com/sympy/sympy/wiki/Writing-Release-Notes).



* No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. <details><summary>Click here to see the pull request description that was parsed.</summary>

    <!-- Your title above should be a short description of what
    was changed. Do not include the issue number in the title. -->

    #### References to other Issues or PRs
    <!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
    format, e.g. "Fixes #1234" (see
    https://tinyurl.com/auto-closing for more information). Also, please
    write a comment on that issue linking back to this pull request once it is
    open. -->
    #18066
    #18056

    #### Brief description of what is fixed or changed

    I've noticed that default_sort_key sympifies elements with string fallback.
    And the issue #18056 still has problem in the master if it is about sorting
    ```python3
    from sympy import *
    from sympy.core.compatibility import default_sort_key

    class C:
        def __repr__(self):
            return 'x.y'

    a = Symbol('x')
    b = C()

    sorted([a, b], key=default_sort_key)
    ```
    ```
    <string> in <module>

    AttributeError: 'Symbol' object has no attribute 'y'
    ```

    I think that default_sort_key still have to rely on `str` for ordering any external object with sympy, but it's better not to execute evaluate some nonsensical syntax that can cause problems

    #### Other comments


    #### Release Notes

    <!-- Write the release notes for this release below. See
    https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
    on how to write release notes. The bot will check your release notes
    automatically to see if they are formatted correctly. -->

    <!-- BEGIN RELEASE NOTES -->
    NO ENTRY
    <!-- END RELEASE NOTES -->

</details><p>

codecov[bot]: # [Codecov](https://codecov.io/gh/sympy/sympy/pull/19006?src=pr&el=h1) Report
> Merging [#19006](https://codecov.io/gh/sympy/sympy/pull/19006?src=pr&el=desc) into [master](https://codecov.io/gh/sympy/sympy/commit/7485df5e724ad6d9df0e0a8acaa5d861d9a35077?src=pr&el=desc) will **increase** coverage by `0.041%`.
> The diff coverage is `100%`.

```diff
@@              Coverage Diff              @@
##            master    #19006       +/-   ##
=============================================
+ Coverage   75.741%   75.782%   +0.041%     
=============================================
  Files          647       647               
  Lines       168584    168623       +39     
  Branches     39723     39734       +11     
=============================================
+ Hits        127688    127787       +99     
+ Misses       35344     35272       -72     
- Partials      5552      5564       +12
```

sylee957: I'm not sure how to resolve those sage errors. It could be from the sage's side.

Patch:
diff --git a/sympy/core/basic.py b/sympy/core/basic.py
index 971e63a0b8..4951b9ad50 100644
--- a/sympy/core/basic.py
+++ b/sympy/core/basic.py
@@ -2045,7 +2045,7 @@ def __iter__(self):
 def _make_find_query(query):
     """Convert the argument of Basic.find() into a callable"""
     try:
-        query = sympify(query)
+        query = _sympify(query)
     except SympifyError:
         pass
     if isinstance(query, type):
diff --git a/sympy/core/compatibility.py b/sympy/core/compatibility.py
index e57d25fdbb..6d918977a1 100644
--- a/sympy/core/compatibility.py
+++ b/sympy/core/compatibility.py
@@ -527,7 +527,7 @@ def default_sort_key(item, order=None):
     else:
         if not isinstance(item, str):
             try:
-                item = sympify(item)
+                item = sympify(item, strict=True)
             except SympifyError:
                 # e.g. lambda x: x
                 pass
diff --git a/sympy/core/containers.py b/sympy/core/containers.py
index b9ddeac3a9..d17aeb78fe 100644
--- a/sympy/core/containers.py
+++ b/sympy/core/containers.py
@@ -49,7 +49,7 @@ class Tuple(Basic):
 
     def __new__(cls, *args, **kwargs):
         if kwargs.get('sympify', True):
-            args = ( sympify(arg) for arg in args )
+            args = (sympify(arg) for arg in args)
         obj = Basic.__new__(cls, *args)
         return obj
 
diff --git a/sympy/core/sympify.py b/sympy/core/sympify.py
index e04b8112d1..ac2d4a3f10 100644
--- a/sympy/core/sympify.py
+++ b/sympy/core/sympify.py
@@ -376,19 +376,20 @@ def sympify(a, locals=None, convert_xor=True, strict=False, rational=False,
             # Not all iterables are rebuildable with their type.
             pass
 
-    # At this point we were given an arbitrary expression
-    # which does not inherit from Basic and doesn't implement
-    # _sympy_ (which is a canonical and robust way to convert
-    # anything to SymPy expression).
-    #
-    # As a last chance, we try to take "a"'s normal form via unicode()
-    # and try to parse it. If it fails, then we have no luck and
-    # return an exception
-    try:
-        from .compatibility import unicode
-        a = unicode(a)
-    except Exception as exc:
-        raise SympifyError(a, exc)
+    if not isinstance(a, str):
+        try:
+            a = str(a)
+        except Exception as exc:
+            raise SympifyError(a, exc)
+        from sympy.utilities.exceptions import SymPyDeprecationWarning
+        SymPyDeprecationWarning(
+            feature="String fallback in sympify",
+            useinstead= \
+                'sympify(str(obj)) or ' + \
+                'sympy.core.sympify.converter or obj._sympy_',
+            issue=18066,
+            deprecated_since_version='1.6'
+        ).warn()
 
     from sympy.parsing.sympy_parser import (parse_expr, TokenError,
                                             standard_transformations)
diff --git a/sympy/polys/domains/domain.py b/sympy/polys/domains/domain.py
index f8e6967b4d..9af62b6a8f 100644
--- a/sympy/polys/domains/domain.py
+++ b/sympy/polys/domains/domain.py
@@ -150,8 +150,7 @@ def convert(self, element, base=None):
         else: # TODO: remove this branch
             if not is_sequence(element):
                 try:
-                    element = sympify(element)
-
+                    element = sympify(element, strict=True)
                     if isinstance(element, Basic):
                         return self.from_sympy(element)
                 except (TypeError, ValueError):
diff --git a/sympy/polys/orderings.py b/sympy/polys/orderings.py
index 870bec0bce..59269461d9 100644
--- a/sympy/polys/orderings.py
+++ b/sympy/polys/orderings.py
@@ -113,12 +113,12 @@ def __call__(self, monomial):
         return tuple(O(lamda(monomial)) for (O, lamda) in self.args)
 
     def __repr__(self):
-        from sympy.core import Tuple
-        return self.__class__.__name__ + repr(Tuple(*[x[0] for x in self.args]))
+        contents = [repr(x[0]) for x in self.args]
+        return self.__class__.__name__ + '(' + ", ".join(contents) + ')'
 
     def __str__(self):
-        from sympy.core import Tuple
-        return self.__class__.__name__ + str(Tuple(*[x[0] for x in self.args]))
+        contents = [str(x[0]) for x in self.args]
+        return self.__class__.__name__ + '(' + ", ".join(contents) + ')'
 
     def __eq__(self, other):
         if not isinstance(other, ProductOrder):


Test patch:
diff --git a/sympy/core/tests/test_compatibility.py b/sympy/core/tests/test_compatibility.py
index af3bddba10..2f705c2224 100644
--- a/sympy/core/tests/test_compatibility.py
+++ b/sympy/core/tests/test_compatibility.py
@@ -10,6 +10,12 @@ def test_default_sort_key():
     func = lambda x: x
     assert sorted([func, x, func], key=default_sort_key) == [func, func, x]
 
+    class C:
+        def __repr__(self):
+            return 'x.y'
+    func = C()
+    assert sorted([x, func], key=default_sort_key) == [func, x]
+
 
 def test_as_int():
     raises(ValueError, lambda : as_int(1.1))
diff --git a/sympy/core/tests/test_sympify.py b/sympy/core/tests/test_sympify.py
index 57a8c4d0f1..036c5803b9 100644
--- a/sympy/core/tests/test_sympify.py
+++ b/sympy/core/tests/test_sympify.py
@@ -6,7 +6,7 @@
     CantSympify)
 from sympy.core.decorators import _sympifyit
 from sympy.external import import_module
-from sympy.testing.pytest import raises, XFAIL, skip
+from sympy.testing.pytest import raises, XFAIL, skip, warns_deprecated_sympy
 from sympy.utilities.decorator import conserve_mpmath_dps
 from sympy.geometry import Point, Line
 from sympy.functions.combinatorial.factorials import factorial, factorial2
@@ -274,6 +274,13 @@ def test_lambda_raises():
 def test_sympify_raises():
     raises(SympifyError, lambda: sympify("fx)"))
 
+    class A:
+        def __str__(self):
+            return 'x'
+
+    with warns_deprecated_sympy():
+        assert sympify(A()) == Symbol('x')
+
 
 def test__sympify():
     x = Symbol('x')
diff --git a/sympy/external/tests/test_sage.py b/sympy/external/tests/test_sage.py
index 99ada65796..f26dbc8de9 100644
--- a/sympy/external/tests/test_sage.py
+++ b/sympy/external/tests/test_sage.py
@@ -24,7 +24,7 @@
 
 import sympy
 
-from sympy.testing.pytest import XFAIL
+from sympy.testing.pytest import XFAIL, warns_deprecated_sympy
 
 def is_trivially_equal(lhs, rhs):
     """
@@ -189,7 +189,8 @@ def test_functions():
     check_expression("Chi(x)", "x")
     check_expression("loggamma(x)", "x")
     check_expression("Ynm(n,m,x,y)", "n, m, x, y")
-    check_expression("hyper((n,m),(m,n),x)", "n, m, x")
+    with warns_deprecated_sympy():
+        check_expression("hyper((n,m),(m,n),x)", "n, m, x")
     check_expression("uppergamma(y, x)", "x, y")
 
 def test_issue_4023():
diff --git a/sympy/matrices/expressions/tests/test_funcmatrix.py b/sympy/matrices/expressions/tests/test_funcmatrix.py
index 99721eb3e4..7ec810b01a 100644
--- a/sympy/matrices/expressions/tests/test_funcmatrix.py
+++ b/sympy/matrices/expressions/tests/test_funcmatrix.py
@@ -2,7 +2,7 @@
 from sympy.functions import KroneckerDelta
 from sympy.matrices import Matrix
 from sympy.matrices.expressions import FunctionMatrix, MatrixExpr, Identity
-from sympy.testing.pytest import raises
+from sympy.testing.pytest import raises, warns_deprecated_sympy
 
 
 def test_funcmatrix_creation():
@@ -18,7 +18,8 @@ def test_funcmatrix_creation():
     raises(ValueError, lambda: FunctionMatrix(0, 2j, Lambda((i, j), 0)))
 
     raises(ValueError, lambda: FunctionMatrix(2, 2, Lambda(i, 0)))
-    raises(ValueError, lambda: FunctionMatrix(2, 2, lambda i, j: 0))
+    with warns_deprecated_sympy():
+        raises(ValueError, lambda: FunctionMatrix(2, 2, lambda i, j: 0))
     raises(ValueError, lambda: FunctionMatrix(2, 2, Lambda((i,), 0)))
     raises(ValueError, lambda: FunctionMatrix(2, 2, Lambda((i, j, k), 0)))
     raises(ValueError, lambda: FunctionMatrix(2, 2, i+j))
diff --git a/sympy/tensor/array/tests/test_array_comprehension.py b/sympy/tensor/array/tests/test_array_comprehension.py
index b87e07606d..d9e6e372a8 100644
--- a/sympy/tensor/array/tests/test_array_comprehension.py
+++ b/sympy/tensor/array/tests/test_array_comprehension.py
@@ -1,7 +1,7 @@
 from sympy.tensor.array.array_comprehension import ArrayComprehension, ArrayComprehensionMap
 from sympy.tensor.array import ImmutableDenseNDimArray
 from sympy.abc import i, j, k, l
-from sympy.testing.pytest import raises
+from sympy.testing.pytest import raises, warns_deprecated_sympy
 from sympy.matrices import Matrix
 
 
@@ -73,5 +73,7 @@ def test_arraycomprehensionmap():
     # tests about lambda expression
     assert ArrayComprehensionMap(lambda: 3, (i, 1, 5)).doit().tolist() == [3, 3, 3, 3, 3]
     assert ArrayComprehensionMap(lambda i: i+1, (i, 1, 5)).doit().tolist() == [2, 3, 4, 5, 6]
-    raises(ValueError, lambda: ArrayComprehensionMap(lambda i, j: i+j, (i, 1, 5)).doit())
     raises(ValueError, lambda: ArrayComprehensionMap(i*j, (i, 1, 3), (j, 2, 4)))
+    with warns_deprecated_sympy():
+        a = ArrayComprehensionMap(lambda i, j: i+j, (i, 1, 5))
+        raises(ValueError, lambda: a.doit())