Avoid using strings for exception_value of func_type #5710

da-woods · 2023-09-17T13:21:57Z

The main advantage is that we don't end up hugely sensitive to the exact string representation of a particular number.

The main advantage is that we don't end up hugely sensitive to the exact string representation of a particular number. Fixes cython#5709

scoder · 2023-09-17T13:58:57Z

Cython/Compiler/ExprNodes.py

+    @staticmethod
+    def make_specific_const_node(pos, value, type):


We can probably build this automatically from the subclasses of ConstNode and their types. That reduces the chance of forgetting to update this method. You forgot str_type, at least.
https://stackoverflow.com/questions/3862310/how-to-find-all-the-subclasses-of-a-class-given-its-name

I think a less redundant name would be ConstNode.for_type().

StringNode is a subclass of PyConstNode rather than ConstNode so wouldn't be caught by the subclasses check. I also think it's difficult to do while still supporting Python 2.

I've changed the name though

I also think it's difficult to do while still supporting Python 2.

We are looking at the Cython literal type here, right? Not the Python object type? Then it's easy to do because we see these types in the source code.

I more meant the __subclasses__ mechanism that is used by the stack overflow page that you linked doesn't exist in Python 2, so it's difficult to get a list of all the subclasses of ConstNode

scoder · 2023-09-17T14:19:26Z

Cython/Compiler/Nodes.py

+                    self.exception_value.calculate_constant_result()
+                    exc_val = self.exception_value.constant_result
+                    from .ExprNodes import constant_value_not_set
+                    if exc_val is None or exc_val is constant_value_not_set:


Isn't this something that ConstNode could do? That would avoid the local import. Something like ExprNode.as_exception_value(), where the default implementation would call error() and subclasses could override it.

We should also make sure that this works for enum values etc., anything that is usable as exception value.

Cython/Compiler/ExprNodes.py

scoder · 2023-10-05T06:50:12Z

Cython/Compiler/Nodes.py

-                        self.exception_value = ConstNode(
-                            self.pos, value=return_type.exception_value, type=return_type)
+                        self.exception_value = ConstNode.for_type(
+                            self.pos, value=str(return_type.exception_value), type=return_type)
            if self.exception_value:


You changed this to is (not) None in other places, so here as well? I still see a couple of other places where we use a plain if … self.exception_value …:, e.g. in the error check just a bit up.

Suggested change

if self.exception_value:

if self.exception_value is not None:

scoder · 2023-10-05T06:54:10Z

Cython/Compiler/ExprNodes.py

@@ -1311,6 +1330,29 @@ def calculate_result_code(self):
    def generate_result_code(self, code):
        pass

+    @staticmethod
+    def for_type(pos, value, type):


Might be worth adding a constant_result as well, while we're at it.

I'm slightly unclear what this means.

I'll add calculate_constant_result() to the value before returning it, but it's possible I've missed the point here.

I meant to set the attribute (explicitly) when creating the node instance. Calling calculate_constant_result() is allowed to fail, so we should not use it outside of the ConstantFolding transform.

You could pass the constant_result also as an optional argument into this method, so that callers can decide if and how to calculate it. Default should probably be constant_value_not_set. Or calculate it here based on the type and value.

This is similar to Parsing.wrap_compile_time_constant(), BTW, although that has a slightly different focus. I'm not implying that there is room for reuse, though…

scoder · 2023-10-05T07:00:09Z

Cython/Compiler/ExprNodes.py

+        result = None
+        with local_errors(ignore=True):
+            result = self.compile_time_value(env)
+        if isinstance(result, Symtab.Entry):
+            result = result.cname


compile_time_value() is used at parse time, which is long over here. This is called during the analysis phase. It seems better to use self.entry and the constant result value.

scoder · 2023-12-16T11:54:21Z

Isn't the constant result calculated already at this point?

Cython/Compiler/ExprNodes.py

scoder · 2023-12-16T14:02:36Z

Cython/Compiler/ExprNodes.py

@@ -1311,6 +1330,29 @@ def calculate_result_code(self):
    def generate_result_code(self, code):
        pass

+    @staticmethod
+    def for_type(pos, value, type):


I meant to set the attribute (explicitly) when creating the node instance. Calling calculate_constant_result() is allowed to fail, so we should not use it outside of the ConstantFolding transform.

You could pass the constant_result also as an optional argument into this method, so that callers can decide if and how to calculate it. Default should probably be constant_value_not_set. Or calculate it here based on the type and value.

This is similar to Parsing.wrap_compile_time_constant(), BTW, although that has a slightly different focus. I'm not implying that there is room for reuse, though…

Co-authored-by: scoder <stefan_ml@behnel.de>

da-woods added 2 commits September 17, 2023 14:19

Avoid using strings for exception_value of func_type

4c0d095

The main advantage is that we don't end up hugely sensitive to the exact string representation of a particular number. Fixes cython#5709

Fix Cython self-build hopefully

3f29ce4

scoder reviewed Sep 17, 2023

View reviewed changes

da-woods added 6 commits September 17, 2023 17:06

Shift the exception_value logic into ExprNode

f3f3dd1

Fix codegen when exception_value == 0

dab7347

Remove debugging code

b9fe05c

Fixup remaining tests (hopefully)

63dc932

Fix BoolNode compile-time value

9417b32

Avoid boolnode

9666e11

da-woods mentioned this pull request Oct 1, 2023

Give fused return types the correct exception value #5737

Open

scoder reviewed Oct 5, 2023

View reviewed changes

Merge branch 'master' into avoid-strs-for-functype-exception-value

30325f0

da-woods mentioned this pull request Nov 13, 2023

[BUG] Cannot put fused type pointer in return type for cdef'd function with latest cython #5816

Open

da-woods mentioned this pull request Dec 15, 2023

Use literal exception values instead of strings where possible. #5912

Merged

Comments from review

2562ad2

Assume constant result is already calculated

0ffd487

scoder reviewed Dec 16, 2023

View reviewed changes

da-woods and others added 3 commits December 16, 2023 14:54

Apply suggestions from code review

1abb981

Co-authored-by: scoder <stefan_ml@behnel.de>

More review stuff

9dd95fa

Merge branch 'master' into avoid-strs-for-functype-exception-value

e49988c

scoder approved these changes Mar 7, 2024

View reviewed changes

scoder added this to the 3.1 milestone Mar 7, 2024

scoder added cleanup Type Analysis labels Mar 7, 2024

da-woods merged commit 6aeecde into cython:master Mar 7, 2024
64 checks passed

da-woods deleted the avoid-strs-for-functype-exception-value branch March 7, 2024 18:15

da-woods restored the avoid-strs-for-functype-exception-value branch March 7, 2024 18:19

da-woods deleted the avoid-strs-for-functype-exception-value branch March 7, 2024 18:23

da-woods mentioned this pull request Apr 1, 2024

[BUG] 'except?' started failing in 3.0.10 #6122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid using strings for exception_value of func_type #5710

Avoid using strings for exception_value of func_type #5710

da-woods commented Sep 17, 2023

scoder Sep 17, 2023

da-woods Sep 17, 2023

scoder Sep 17, 2023

da-woods Sep 17, 2023

scoder Sep 17, 2023

scoder Oct 5, 2023

scoder Oct 5, 2023

da-woods Dec 16, 2023

scoder Dec 16, 2023

scoder Oct 5, 2023

scoder commented Dec 16, 2023 via email

scoder Dec 16, 2023

		@staticmethod
		def make_specific_const_node(pos, value, type):

	if self.exception_value:
	if self.exception_value is not None:

Avoid using strings for exception_value of func_type #5710

Avoid using strings for exception_value of func_type #5710

Conversation

da-woods commented Sep 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoder commented Dec 16, 2023 via email

Choose a reason for hiding this comment