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 crash when function contains fused extension type #6204

Merged
merged 1 commit into from
May 21, 2024
Merged

Fix crash when function contains fused extension type #6204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion Cython/Compiler/PyrexTypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5042,7 +5042,7 @@ def best_match(arg_types, functions, pos=None, env=None, args=None):
if len(candidates) == 1:
return candidates[0][0]
elif len(candidates) == 0:
if pos is not None:
if pos is not None and errors:
Copy link
Contributor

Choose a reason for hiding this comment

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

You've possibly looked into this more than me, but I wonder if this should end up showing a "no suitable method found" error?

(I'm also not quite sure what the point of [1 for func, e in errors if e == errmsg] below... that looks like it could be simplified).

Otherwise PR looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this should end up showing a "no suitable method found" error?

No idea. Broken parts of the cythonized code already contains errors - see test part of the PR. Honestly, I tried to grep the code and I was not able to find any test with this error message:

matus@dev:~/dev/cython/tests$ grep -RI 'no suitable method found' *
matus@dev:~/dev/cython/tests$ grep -RI 'suitable method found' *
matus@dev:~/dev/cython/tests$ grep -RI 'suitable method' *
matus@dev:~/dev/cython/tests$ grep -RI 'suitable' *

For me adding additional error is not adding any value to the case I found compiler crashing.

(I'm also not quite sure what the point of

When I reviewed the code, I agree. It does not make sense. the part [1 for func, e in errors if e == errmsg] because at least first item in errors list will match yielding [1] list in worst case. Maybe I am missing something though. It is difficult to say because I didn't found any test covering it...

Copy link
Contributor

Choose a reason for hiding this comment

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

Broken parts of the cythonized code already contains errors [...] For me adding additional error is not adding any value to the case I found compiler crashing.

Yes - agree with this.

I think this change is fine then. Avoiding the crash is definitely an improvement

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not make sense. the part [1 for func, e in errors if e == errmsg] because at least first item in errors list will match

I can also only guess, but the intention might have been to avoid dropping the concrete error message in the case that the same error message simply appeared more than once. That's not entirely unrealistic, given that some error messages refer simply to the number of arguments. I agree that the code is useless as it stands. I've changed it here, currently testing that it doesn't break anything:
scoder@ee1e885

Copy link
Contributor

Choose a reason for hiding this comment

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

It turns out that the effect is the other way round: we lose concrete error messages if I make the code meaningful as I suggested. This test now produces "no suitable method found" instead of "Call with wrong number of arguments (expected 0, got 2)":
https://github.com/cython/cython/blob/9e81f994aee2a391db8c2bc9c9b9e79fff74e1ea/tests/errors/cpp_no_constructor.pyx

Arguably, that's more correct than the previous message since we're expecting 0 or 1 arguments, not exactly 0. What do you think, should we change this or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably, that's more correct than the previous message since we're expecting 0 or 1 arguments, not exactly 0. What do you think, should we change this or not?

e.g. #3235 (comment) - we've definitely had cases before where the exact output chosen and the more general output would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll change the message here.

func, errmsg = errors[0]
if len(errors) == 1 or [1 for func, e in errors if e == errmsg]:
error(pos, errmsg)
Expand Down
10 changes: 10 additions & 0 deletions tests/errors/fused_types.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ cdef void contents_unfindable1(cython.integral x):
z: floating = 1 # note: cdef variables also fail with an error but not by the time this test aborts
sz = sizeof(floating)

cdef fused fused3:
Foo
cython.int

cdef cython.integral func_with_fused_extension(fused3 foo):
return x

func_with_fused_extension(5)

_ERRORS = u"""
11:15: fused_type does not take keyword arguments
Expand All @@ -112,4 +120,6 @@ _ERRORS = u"""
87:16: Type cannot be specialized since it is not a fused argument to this function
87:16: Type cannot be specialized since it is not a fused argument to this function
87:16: Type cannot be specialized since it is not a fused argument to this function
93:5: Return type is a fused type that cannot be determined from the function arguments
96:0: Invalid use of fused types, type cannot be specialized
"""
Loading