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.

Sign up for GitHub

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

Jump to bottom

Fix PyInteger / float division #6195

Merged
merged 6 commits into from
Jun 28, 2024
Merged

Fix PyInteger / float division #6195

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
88473aa
Fix PyInteger / float division
da-woods May 12, 2024
ba165be
Add tests for a few more operators
da-woods May 13, 2024
60f0f0e
Adjust other analyse_operations
da-woods Jun 6, 2024
20a4ddb
Update Cython/Compiler/ExprNodes.py
da-woods Jun 27, 2024
ffab33c
Add explanatory comment and assert
da-woods Jun 27, 2024
d0d6fb1
Remove assert, add clarifying comment
da-woods Jun 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions Cython/Compiler/ExprNodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11830,8 +11830,7 @@ def infer_type(self, env):
def analyse_types(self, env):
self.operand1 = self.operand1.analyse_types(env)
self.operand2 = self.operand2.analyse_types(env)
self.analyse_operation(env)
return self
return self.analyse_operation(env)

def analyse_operation(self, env):
if self.is_pythran_operation(env):
Expand All @@ -11843,12 +11842,17 @@ def analyse_operation(self, env):
self.coerce_operands_to_pyobjects(env)
self.type = self.result_type(self.operand1.type,
self.operand2.type, env)
assert self.type.is_pyobject
self.is_temp = 1
if not self.type.is_pyobject:
original_type, self.type = self.type, py_object_type
# Hopefully this can be optimized out in some cases
return self.coerce_to(original_type, env)

elif self.is_cpp_operation():
self.analyse_cpp_operation(env)
else:
self.analyse_c_operation(env)
return self
scoder marked this conversation as resolved.
Show resolved Hide resolved

def is_py_operation(self):
return self.is_py_operation_types(self.operand1.type, self.operand2.type)
Expand Down Expand Up @@ -12276,8 +12280,7 @@ def analyse_types(self, env):
elif operand2.type in builtin_sequence_types:
self.operand1 = operand1.coerce_to_index(env)

self.analyse_operation(env)
return self
return self.analyse_operation(env)

@staticmethod
def is_builtin_seqmul_type(type):
Expand Down Expand Up @@ -12414,7 +12417,7 @@ def infer_builtin_types_operation(self, type1, type2):

def analyse_operation(self, env):
self._check_truedivision(env)
NumBinopNode.analyse_operation(self, env)
result = NumBinopNode.analyse_operation(self, env)
if self.is_cpp_operation():
self.cdivision = True
if not self.type.is_pyobject:
Copy link
Contributor

Choose a reason for hiding this comment

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

This code only (safely) works if NumBinopNode.analyse_operation() returns exactly self, not anything else. That is not an assumption we should generally make. It's really the whole point of returning a node that it allows us to return a different node.

It's difficult to deal with this correctly, though. It's ambiguous if the base method returns a different node or even different type of node. It might be a wrapper around the original (self) node, or it might have replaced the node entirely. We can't know this here. Not sure how to handle this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The real assumption here is that result contains self (i.e. self is still relevant and can be manipulated, even if it's wrapped in a coercion node or something like that).

I've added a comment and an assert to clarify/enforce that.

I'm not hugely happy with this either though, but I think it's better than it initially looks.

Expand All @@ -12426,6 +12429,7 @@ def analyse_operation(self, env):
# Need to check ahead of time to warn or raise zero division error
self.operand1 = self.operand1.coerce_to_simple(env)
self.operand2 = self.operand2.coerce_to_simple(env)
return result

def compute_c_result_type(self, type1, type2):
if self.operator == '/' and self.ctruedivision and not type1.is_cpp_class and not type2.is_cpp_class:
Expand Down Expand Up @@ -12578,12 +12582,13 @@ def zero_division_message(self):
return "float divmod()"

def analyse_operation(self, env):
DivNode.analyse_operation(self, env)
result = DivNode.analyse_operation(self, env)
if not self.type.is_pyobject:
if self.cdivision is None:
self.cdivision = env.directives['cdivision'] or not self.type.signed
if not self.cdivision and not self.type.is_int and not self.type.is_float:
error(self.pos, "mod operator not supported for type '%s'" % self.type)
return result

def generate_evaluation_code(self, code):
if not self.type.is_pyobject and not self.cdivision:
Expand Down
2 changes: 2 additions & 0 deletions tests/run/type_inference.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ def arithmetic():
assert typeof(f) == "long", typeof(f)
g = 4 // <int>2
assert typeof(g) == "long", typeof(g)
h = int(2) / 3.0
da-woods marked this conversation as resolved.
Show resolved Hide resolved
assert typeof(h) == "double", typeof(h)

cdef class some_class:
pass
Expand Down
Loading