Repository: sympy/sympy
Pull request: Move the super() call to the end of FunctionClass.__init__
The `super()` call calls `BasicMeta.__init__`, which adds the class to a set of
classes (`sympy.core.core.all_classes`). This computes the hash of the class.
However, the hash of a Function depends on its `nargs` (`class_key()`), meaning if
it is set dynamically, the hash will change and the class won't be found in
the `all_classes` set any more, breaking `sympify(obj)`.

This happens for things like `Function('f', eval=lambda self, t: ...)` where
`nargs` is determined from the `eval` lambda.


<!-- 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://github.com/blog/1506-closing-issues-via-pull-requests .-->

Fixes #15170.

#### Brief description of what is fixed or changed


#### 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 -->
* core
  * Fix an issue with undefined functions defined with a custom `eval` (like `Function('f', eval=...)`).
<!-- END RELEASE NOTES -->


Problem statement:
UndefinedFunction.__hash__()'s use of class_key() broke existing code
Hi, I just learned that commit e210a762d (_"Allow Function('f', is_real=True) != Function('f')"_) broke my code, and I'm not sure if this is a bug in SymPy or not. The problem appears to have been caused by this change:

	class UndefinedFunction(FunctionClass):
	+    def __hash__(self):
	+        return hash((self.class_key(), frozenset(self._extra_kwargs)))

The behavior change is as follows. If I had code such as

	temp = sympy.Function(
		'temp',
		fdiff=lambda self, argindex=1: sympy.cos(self.args[argindex - 1]),
		eval=classmethod(lambda cls, t: None),
		_imp_=classmethod(lambda cls, t: sympy.sin(t))
	)
	print(sympy.diff(temp(sympy.abc.t), sympy.abc.t))

it would print

	cos(t)

under the old implementation of `__hash__`, but under the new implementation, it merely prints

	Derivative(temp(t), t)

which is undesired behavior.

It appears to me that this is a **bug** caused by the value of `_nargs` changing between successive invocations of `__hash__`, which causes the exact _same_ object to hash to two different values. However, I don't really understand the change and I'm entirely sure what the fix would be.

Would you know if this is a bug or not, and what the solution might be? Thank you.

Hints from issue discussion:
I don't know why it worked before. The issue is this:

```py
>>> expr = temp(t)
>>> expr.fdiff
<bound method <lambda> of temp(t)>
>>> expr = sympify(expr)
>>> expr.fdiff
<bound method Function.fdiff of temp(t)>
```

The reason is that sympify doesn't recognize `temp(t)` as a SymPy object, and so eventually falls back to parsing `str(temp(t))`, which removes all the custom attributes. 

We shouldn't be using [`sympy_classes`](https://github.com/sympy/sympy/blob/f4984def9f735823411bc5d5b0d9bcb322738303/sympy/core/sympify.py#L266) to determine if an object is a SymPy class. It will eventually be removed. sympify should probably short circuit and return the object as is if it's an instance of a Basic subclass, but I don't know if that will break anything. 

Assuming it does, a less drastic fix to this would be go automatically define `_sympy_` on Function objects that define custom methods. 
OK, I guess `temp` actually is added to sympy_classes from the `BasicMeta.__init__`. 

I think you must be right about the hash changing. That is the only thing that can explain this behavior (from debugging `sympify`).

```py
>>> [i for i in sympy_classes if str(i) == 'temp'][0]
temp
>>> cls
temp
>>> [i for i in sympy_classes if str(i) == 'temp'][0] == cls
True
>>> [i for i in sympy_classes if str(i) == 'temp'][0] is cls
True
>>> hash([i for i in sympy_classes if str(i) == 'temp'][0])
7991973985617563503
>>> hash(cls)
7991973985617563503
>>> cls in sympy_classes
False
>>> [i for i in sympy_classes if str(i) == 'temp'][0] in sympy_classes
False
```

`sympy_classes` is a Python `set`. 
I see what is happening. The very adding of the class to sympy_classes in the metaclasses causes the hash to be calculated before _nargs is computed, because _nargs is added in `__init__`, which gets called at the top of the function. before _nargs is set https://github.com/sympy/sympy/blob/f4984def9f735823411bc5d5b0d9bcb322738303/sympy/core/function.py#L148

I think it should be enough to just move the `super` call to the end of the function. Let's see if that breaks anything. 
Fix at https://github.com/sympy/sympy/pull/15174
By the way, I believe a workaround that should work is to manually add the class to `sympy.core.core.all_classes`. 
Oh wow, that was fast! Awesome, thank you!

Commit message: Fix a test so it works in Python 2

Patch to review:
diff --git a/sympy/core/function.py b/sympy/core/function.py
index 9c4453ab5055..479b352f8cb6 100644
--- a/sympy/core/function.py
+++ b/sympy/core/function.py
@@ -145,7 +145,6 @@ def __init__(cls, *args, **kwargs):
         # honor kwarg value or class-defined value before using
         # the number of arguments in the eval function (if present)
         nargs = kwargs.pop('nargs', cls.__dict__.get('nargs', _getnargs(cls)))
-        super(FunctionClass, cls).__init__(args, kwargs)
 
         # Canonicalize nargs here; change to set in nargs.
         if is_sequence(nargs):
@@ -162,6 +161,8 @@ def __init__(cls, *args, **kwargs):
             nargs = (as_int(nargs),)
         cls._nargs = nargs
 
+        super(FunctionClass, cls).__init__(args, kwargs)
+
     @property
     def __signature__(self):
         """
diff --git a/sympy/core/tests/test_function.py b/sympy/core/tests/test_function.py
index 6633a5ac4af3..79606697b566 100644
--- a/sympy/core/tests/test_function.py
+++ b/sympy/core/tests/test_function.py
@@ -5,6 +5,7 @@
 from sympy.utilities.pytest import XFAIL, raises
 from sympy.abc import t, w, x, y, z
 from sympy.core.function import PoleError, _mexpand
+from sympy.core.sympify import sympify
 from sympy.sets.sets import FiniteSet
 from sympy.solvers.solveset import solveset
 from sympy.utilities.iterables import subsets, variations
@@ -965,3 +966,23 @@ def test_undef_fcn_float_issue_6938():
     assert not f(pi).evalf().is_number
     x = Symbol('x')
     assert not f(x).evalf(subs={x:1.2}).is_number
+
+def test_undefined_function_eval():
+    # Issue 15170. Make sure UndefinedFunction with eval defined works
+    # properly. The issue there was that the hash was determined before _nargs
+    # was set, which is included in the hash, hence changing the hash. The
+    # class is added to sympy.core.core.all_classes before the hash is
+    # changed, meaning "temp in all_classes" would fail, causing sympify(temp(t))
+    # to give a new class. We will eventually remove all_classes, but make
+    # sure this continues to work.
+
+    fdiff = lambda self, argindex=1: cos(self.args[argindex - 1])
+    eval = classmethod(lambda cls, t: None)
+    _imp_ = classmethod(lambda cls, t: sin(t))
+
+    temp = Function('temp', fdiff=fdiff, eval=eval, _imp_=_imp_)
+
+    expr = temp(t)
+    assert sympify(expr) == expr
+    assert type(sympify(expr)).fdiff.__name__ == "<lambda>"
+    assert expr.diff(t) == cos(t)


Code review comments:
diff_hunk: @@ -162,6 +161,8 @@ def __init__(cls, *args, **kwargs):
             nargs = (as_int(nargs),)
         cls._nargs = nargs
 
+        super(FunctionClass, cls).__init__(args, kwargs)
path: sympy/core/function.py
text: @user1:
It looks like ManagedProperties and BasicMeta will read this as two args (and empty keyword dict) but it does not matter as they do not use them.

@author:
Oh I didn't notice that. Obviously it should be `*args, **kwargs`.