Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fused arguments specified by annotation or locals #3391

Merged
merged 3 commits into from Mar 3, 2020

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Mar 2, 2020

Issues were:

  1. DefNode.has_fused_arguments was set too early (before
    locals/annotations) were evalutated, so function was not treated
    as fused.
  2. When re-evaluating the specializations of the fused function
    it was treated as a redefinition because the locals/annotation was
    reapplied over the specialized type.

Issues were:
1. DefNode.has_fused_arguments was set too early (before
locals/annotations) were evalutated, so function was not treated
as fused.
2. When re-evaluating the specializations of the fused function
it was treated as a redefinition because the locals/annotation was
reapplied over the specialized type.
Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks!

Cython/Compiler/Nodes.py Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
import cython

class TestCls:
def func1(self, arg: NotInPy):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't a string type name make this available in older Py3 versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A string type name didn't work. It was what I originally tried. I'll try to see if I can work out why before revising this though.

tests/run/pure_fused.py Outdated Show resolved Hide resolved
tests/run/pure_fused.pxd Show resolved Hide resolved
@scoder scoder added this to the 0.29.16 milestone Mar 2, 2020
Including annotation as string (required changes to
StringNode.analyse_as_type), and extra tests for fused type defined
as cython.fused_type in the Py file
Causing it to fail on pure Python 3.0-3.6
Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

if global_entry and global_entry.type and (
global_entry.type.is_extension_type
or global_entry.type.is_struct_or_union
or global_entry.type.is_builtin_type
or global_entry.type.is_cpp_class):
if global_entry and global_entry.is_type and global_entry.type:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say right now if there are any cases where we would like to disallow it, but I guess we can just wait until a user finds one and complains. :)

Cython/Compiler/Nodes.py Show resolved Hide resolved
@scoder scoder merged commit a8cb127 into cython:master Mar 3, 2020
alexhenrie pushed a commit to alexhenrie/cython that referenced this pull request Mar 25, 2020
)

1. DefNode.has_fused_arguments was set too early (before
locals/annotations) were evalutated, so function was not treated
as fused.
2. When re-evaluating the specializations of the fused function
it was treated as a redefinition because the locals/annotation was
reapplied over the specialized type.
3. Including annotation as string (required changes to
StringNode.analyse_as_type), and extra tests for fused type defined
as cython.fused_type in the Py file
@scoder scoder modified the milestones: 0.29.16, 0.29.17, 3.0 Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants