Repository: sympy/sympy

Issue:
sympify("abs(x)*5",evaluate=False) crashes
Long story short: The following program crashes:
```
import sympy
sympy.sympify("abs(x)*5",evaluate=False)
```
with the message:
> Traceback (most recent call last):
>   File "bugreport2.py", line 3, in <module>
>     sympy.sympify("abs(x)*5",evaluate=False)
>   File "/home/volker/.local/lib/python3.7/site-packages/sympy/core/sympify.py", line 384, in sympify
>     expr = parse_expr(a, local_dict=locals, transformations=transformations, evaluate=evaluate)
>   File "/home/volker/.local/lib/python3.7/site-packages/sympy/parsing/sympy_parser.py", line 964, in parse_expr
>     code = compile(evaluateFalse(code), '<string>', 'eval')
>   File "/home/volker/.local/lib/python3.7/site-packages/sympy/parsing/sympy_parser.py", line 974, in evaluateFalse
>     node = EvaluateFalseTransformer().visit(node)
>   File "/usr/lib/python3.7/ast.py", line 262, in visit
>     return visitor(node)
>   File "/usr/lib/python3.7/ast.py", line 317, in generic_visit
>     value = self.visit(value)
>   File "/usr/lib/python3.7/ast.py", line 262, in visit
>     return visitor(node)
>   File "/usr/lib/python3.7/ast.py", line 326, in generic_visit
>     new_node = self.visit(old_value)
>   File "/usr/lib/python3.7/ast.py", line 262, in visit
>     return visitor(node)
>   File "/home/volker/.local/lib/python3.7/site-packages/sympy/parsing/sympy_parser.py", line 1047, in visit_BinOp
>     new_node.args = self.flatten(new_node.args, sympy_class)
>   File "/home/volker/.local/lib/python3.7/site-packages/sympy/parsing/sympy_parser.py", line 996, in flatten
>     if isinstance(arg, ast.Call) and arg.func.id == func:
> AttributeError: 'Call' object has no attribute 'id'

sympy.\_\_version\_\_ is '1.4' and sys.version is '3.7.3 (default, Oct  7 2019, 12:56:13) \n[GCC 8.3.0]'

Discussion:
sympy-bot: :white_check_mark:

Hi, I am the [SymPy bot](https://github.com/sympy/sympy-bot) (v161). 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).



Your release notes are in good order.

Here is what the release notes will look like:
* parsing
  * parse_expr converts built in abs, max, min, pow to sympy functions ([#22169](https://github.com/sympy/sympy/pull/22169) by [@eagleoflqj](https://github.com/eagleoflqj))

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10.

<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. -->

    Fixes #17811
    #### Brief description of what is fixed or changed

    See https://github.com/sympy/sympy/issues/17811#issuecomment-547191254
    #### Other comments


    #### Release Notes

    <!-- Write the release notes for this release below between the BEGIN and END
    statements. The basic format is a bulleted list with the name of the subpackage
    and the release note for this PR. For example:

    * solvers
      * Added a new solver for logarithmic equations.

    * functions
      * Fixed a bug with log of integers.

    or if no release note(s) should be included use:

    NO ENTRY

    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 -->
    * parsing
      * parse_expr converts built in abs, max, min, pow to sympy functions
    <!-- END RELEASE NOTES -->


</details><p>

github-actions[bot]: Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with `+` are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with `-` are speedups.

Significantly changed benchmark results (PR vs master)
```diff

```
Significantly changed benchmark results (master vs previous release)
```diff

```
Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).
oscargus: Seems to make sense! I'm not really experienced with advanced usage of the parsing, but I think it would make even more sense if this was added independently of the user providing a `global_dict` (or otherwise this functionality would break unless the user re-adds them explicitly). Not sure how that should be done though. Can one update the dict with these four if they are not already present? Should it be possible to avoid using them?
eagleoflqj: Any comments? @asmeurer
What about
```py
def parse_expr(s, local_dict=None, transformations=standard_transformations,
global_dict=None, evaluate=True, convert_builtins=(abs, max, min, pow)):
```
that still convert these four by default, but allow user to assign a false value or a subset tuple?
smichr: Don't these automatically call the SymPy equivalent? That way you don' thave to worry about ever importing these; Python will just do the right thing and call the SymPy version.
eagleoflqj: > Don't these automatically call the SymPy equivalent? That way you don' thave to worry about ever importing these; Python will just do the right thing and call the SymPy version.

I'm afraid no on current master:
```py
>>> import sympy
>>> sympy.__version__
'1.10.dev'
>>> sympy.parse_expr('abs(1)')
abs(1)
>>> sympy.parse_expr('Abs(1)')
1
```
oscargus: > What about

Something like that would make sense, yes. I am not fully convinced about using a tuple with functions though, but not really sure exactly how to do it.

Maybe have a default value of `None` and then accept a list with string names? That is probably the easiest way since to can then create the dict and update `global_dict` with those items (or all for `None`)? 

Although it is somehow more correct to have the default value as you do, it is typically not how it is done in Sympy. Also, it may lead to a quite long signature in editors supporting it.

asmeurer: I would expect all builtin functions to just parse as themselves. Then abs and pow would not need to be handled separately. min and max still make sense though. 
asmeurer: I'm not really sure this needs to be a flag. What's the use-case for disabling built-ins parsing? 
eagleoflqj: > I'm not really sure this needs to be a flag. What's the use-case for disabling built-ins parsing?

idk, maybe just provide a way compatible to previous behavior?
smichr: > maybe just provide a way compatible to previous behavior?

This is stalled because of this. Let's just leave that option off until we have good reason to add it. People should not be using builtins as personal function names in general. And if they do, can't they just pass a `local_dict` indicating that the function should not be parsed as a SymPy function?
eagleoflqj: ping @smichr 
oscargus: LGTM!

Patch:
diff --git a/sympy/parsing/sympy_parser.py b/sympy/parsing/sympy_parser.py
index ba1f04aa89..e403654cb4 100644
--- a/sympy/parsing/sympy_parser.py
+++ b/sympy/parsing/sympy_parser.py
@@ -8,6 +8,8 @@
 import ast
 import unicodedata
 from io import StringIO
+import builtins
+import types
 
 from sympy.assumptions.ask import AssumptionKeys
 from sympy.core.basic import Basic
@@ -15,7 +17,7 @@
 from sympy.core.function import arity, Function
 from sympy.utilities.iterables import iterable
 from sympy.utilities.misc import filldedent, func_name
-
+from sympy.functions.elementary.miscellaneous import Max, Min
 
 
 def _token_splittable(token):
@@ -1072,6 +1074,14 @@ def parse_expr(s, local_dict=None, transformations=standard_transformations,
                 raise TypeError(filldedent('''
                     a transformation should be function that
                     takes 3 arguments'''))
+
+    builtins_dict = vars(builtins)
+    for name, obj in builtins_dict.items():
+        if isinstance(obj, types.BuiltinFunctionType):
+            global_dict[name] = obj
+    global_dict['max'] = Max
+    global_dict['min'] = Min
+
     code = stringify_expr(s, local_dict, global_dict, transformations)
 
     if not evaluate:


Test patch:
diff --git a/sympy/parsing/tests/test_sympy_parser.py b/sympy/parsing/tests/test_sympy_parser.py
index 460dfe99af..2e326c576d 100644
--- a/sympy/parsing/tests/test_sympy_parser.py
+++ b/sympy/parsing/tests/test_sympy_parser.py
@@ -318,3 +318,15 @@ def test_parsing_definitions():
     assert T[:5, 8] == standard_transformations + (t[8],)
     assert parse_expr('0.3x^2', transformations='all') == 3*x**2/10
     assert parse_expr('sin 3x', transformations='implicit') == sin(3*x)
+
+
+def test_builtins():
+    cases = [
+        ('abs(x)', 'Abs(x)'),
+        ('max(x, y)', 'Max(x, y)'),
+        ('min(x, y)', 'Min(x, y)'),
+        ('pow(x, y)', 'Pow(x, y)'),
+    ]
+    for built_in_func_call, sympy_func_call in cases:
+        assert parse_expr(built_in_func_call) == parse_expr(sympy_func_call)
+    assert str(parse_expr('pow(38, -1, 97)')) == '23'