Fix crash when function contains fused extension type #6204
Conversation
| @@ -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: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok. I'll change the message here.
No description provided.