Opened 6 years ago

Closed 5 years ago

#489 closed defect (fixed)

Redefinition of Python functions

Reported by: scoder Owned by: vitja
Priority: major Milestone: 0.15
Component: Python Semantics Keywords:
Cc: vitja.makarov@…


Python allows this:

def f(): pass
def f(): pass
def f():
    def f(): pass
    def f(): pass
    def f(): pass

Cython currently forbids the redefinition of a function (even in the cython-closures branch). This restriction should be lifted and (re-)assignments to a function name should be allowed.

Attachments (1)

allow-function-redefine.patch (7.4 KB) - added by vitja 5 years ago.
Allow function redefinition

Download all attachments as: .zip

Change History (7)

comment:1 Changed 5 years ago by vitja

  • Cc vitja.makarov@… added

Attached patch forces assignment synthesis for python functions, now it sets is_lambda entry attribute to true, is_lambda should be renamed to something more generic (force_synth?).

Now function entry is private (it holds all the cname and friends), and public entry is forced to be of type of py_object_type.

Module methods aren't now defined in global method table. They are create on the fly.

Looking at tests results it seems to work just fine.

comment:2 Changed 5 years ago by scoder

A few comments on your patch:

  • I don't see a need for an inner function in declare_pyfunction(), just make that a method. Note that "classic" isn't a very explicit name.
  • it lacks a test for inner functions that don't need a closure
  • it lacks a test for inner functions whose names end up in the closure themselves
  • callable() is not supported in Py3 (even if that may get reverted in the future), just test for isinstance(x, (int, list)) instead
  • I prefer letting test classes inherit from object, old-style classes are ugly and evil
  • I'd use a keyword argument in to make it clearer what the not self.is_wrapper means in this context
  • pretty_name seems to be used only for lambda functions, so I think it's a separate change that's unrelated to this patch. I'm not sure, but I think that the name chosen in should depend explicitly on the is_lambda flag and the common functionality that you need for redefined functions and lambda functions should not depend on is_lambda but a more explicitly named flag (not sure ATM what that functionality is, so I can't suggest a better name)

comment:3 Changed 5 years ago by scoder

The patch lacks error tests and the modified code actually fails to detect redefinitions of cdef functions as Python functions and vice versa, which is not supported (and IMHO doesn't make sense either). Similarly, redefinitions in cdef classes aren't currently supported but are not detected either.

Changed 5 years ago by vitja

Allow function redefinition

comment:4 Changed 5 years ago by scoder

One quick comment: there is a difference between


which is True for any Python type, including builtin types and extension types, and

entry.type is py_object_type

which is True only for the Python object type, not for subtypes. The latter is the right thing to test for functions, as e.g. "bytes" is not a valid type for a variable that holds a Python functions.

comment:5 Changed 5 years ago by scoder

  • Milestone changed from wishlist to 0.15
  • Owner set to vitja

comment:6 Changed 5 years ago by scoder

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.