[red-knot] Allow ellipsis default params in stub functions #17243
Conversation
|
AlexWaygood
added
ty
MatthewMckee4
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
MatthewMckee4
changed the title
| }) | ||
| } | ||
|
|
||
| fn in_function_overload_or_abstract(&self) -> bool { |
There was a problem hiding this comment.
nit: maybe call this in_overloaded_or_abstract_function?
| fn in_function_overload_or_abstract(&self) -> bool { | |
| fn in_overloaded_or_abstract_function(&self) -> bool { |
There was a problem hiding this comment.
I'm not sure about this naming change, but I'm also not sure about the semantics of the function in the first place. I think being "in an overloaded function" suggests that it would include the main implementation (which is not decorated with @overload), but that shouldn't be included here. Have to go at the moment but would like to look at this a bit more.
There was a problem hiding this comment.
I'll test to see if this surprises an error of an ellipsis in the main overload
There was a problem hiding this comment.
it doesn't, but i also see thats not really what you were referring to
There was a problem hiding this comment.
Ok sorry for the cryptic note here, I was on a plane yesterday and the flight attendant was literally asking me to put away my computer as I was submitting that comment 😆
There were three things I wanted to look at here:
-
Do we handle generic methods correctly (not just methods of generic classes, which you already added a test for)? The answer is yes, we do, but I'll push a couple tests for that as well.
-
I wanted to clarify my understanding of how this same code works for both default values and for return statements inside the function, since default values are inferred in the outer scope, not the function body scope. But what I realized looking at it more closely is that function parameter definitions are handled when inferring the function body scope, so we are actually in the function body scope for all call sites of these functions. I think I might add doc comments to these methods clarifying their usage, but otherwise I think this looks good.
-
The question of how to name this function. I think the name (and doc comment) should be clear that it returns true for overloads, not for overloaded functions, since the latter could be read to mean the main implementation function. Thus I wouldn't favor the name change suggested above, and I'm not coming up with anything I like significantly better than the current name.
There was a problem hiding this comment.
3. The question of how to name this function. I think the name (and doc comment) should be clear that it returns true for overloads, not for overloaded functions, since the latter could be read to mean the main implementation function. Thus I wouldn't favor the name change suggested above, and I'm not coming up with anything I like significantly better than the current name.
I do understand what you're saying here... in_function_decorated_with_abstractmethod_or_overload? But it's obviously awfully verbose.
in_stublike_function?
There was a problem hiding this comment.
I'm ok with the very verbose name if you like it better, we won't have tons of call sites. I think we could also go with just in_function_overload_or_abstractmethod if you like that better? Tbh I don't know what alternatives to suggest since I'm not clear on precisely what it is you dislike about the current name :)
There was a problem hiding this comment.
I don't like the current name because it doesn't make grammatical sense -- you can have a "function overload", sure, but you can't have a "function abstract" (it would be "abstract function")
in_overload_or_abstractmethod or similar would work fine for me!
There was a problem hiding this comment.
But I think the current name is perfectly grammatical, it just needs to be read with different grouping :) It's "in (function overload) or (abstractmethod)", not "in function (overload or abstractmethod)"
There was a problem hiding this comment.
Ah sorry, I think we were talking at cross purposes here? I didn't realise the name had already changed. The original name of this function in the first version of this PR was in_function_overload_or_abstract(), and that's the version of this PR that my original comment here was attached to. The name of the function in the merged version of the PR is in_function_overload_or_abstractmethod(), which, yes, I agree is perfectly grammatical and much better!
crates/red_knot_python_semantic/resources/mdtest/function/parameters.md
Outdated
Show resolved
Hide resolved
| }) | ||
| } | ||
|
|
||
| fn in_function_overload_or_abstract(&self) -> bool { |
There was a problem hiding this comment.
Ok sorry for the cryptic note here, I was on a plane yesterday and the flight attendant was literally asking me to put away my computer as I was submitting that comment 😆
There were three things I wanted to look at here:
-
Do we handle generic methods correctly (not just methods of generic classes, which you already added a test for)? The answer is yes, we do, but I'll push a couple tests for that as well.
-
I wanted to clarify my understanding of how this same code works for both default values and for return statements inside the function, since default values are inferred in the outer scope, not the function body scope. But what I realized looking at it more closely is that function parameter definitions are handled when inferring the function body scope, so we are actually in the function body scope for all call sites of these functions. I think I might add doc comments to these methods clarifying their usage, but otherwise I think this looks good.
-
The question of how to name this function. I think the name (and doc comment) should be clear that it returns true for overloads, not for overloaded functions, since the latter could be read to mean the main implementation function. Thus I wouldn't favor the name change suggested above, and I'm not coming up with anything I like significantly better than the current name.
* main: (42 commits) [playground] New default program (#17277) [red-knot] Add `--python-platform` CLI option (#17284) [red-knot] Allow ellipsis default params in stub functions (#17243) [red-knot] Fix stale syntax errors in playground (#17280) Update Rust crate clap to v4.5.35 (#17273) Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061) [ci] Fix pattern for code changes (#17275) [`airflow`] Update oudated `AIR301`, `AIR302` rules (#17123) [docs] fix formatting of "See Style Guide" link (#17272) [red-knot] Support stub packages (#17204) ruff_annotate_snippets: address unused code warnings [red-knot] Add a couple more tests for `*` imports (#17270) [red-knot] Add 'Format document' to playground (#17217) Update actions/setup-node action to v4.3.0 (#17259) Update actions/upload-artifact action to v4.6.2 (#17261) Update actions/download-artifact action to v4.2.1 (#17258) Update actions/setup-python action to v5.5.0 (#17260) Update actions/cache action to v4.2.3 (#17256) Update Swatinem/rust-cache action to v2.7.8 (#17255) Update actions/checkout action to v4.2.2 (#17257) ...
* main: (222 commits) [playground] New default program (#17277) [red-knot] Add `--python-platform` CLI option (#17284) [red-knot] Allow ellipsis default params in stub functions (#17243) [red-knot] Fix stale syntax errors in playground (#17280) Update Rust crate clap to v4.5.35 (#17273) Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061) [ci] Fix pattern for code changes (#17275) [`airflow`] Update oudated `AIR301`, `AIR302` rules (#17123) [docs] fix formatting of "See Style Guide" link (#17272) [red-knot] Support stub packages (#17204) ruff_annotate_snippets: address unused code warnings [red-knot] Add a couple more tests for `*` imports (#17270) [red-knot] Add 'Format document' to playground (#17217) Update actions/setup-node action to v4.3.0 (#17259) Update actions/upload-artifact action to v4.6.2 (#17261) Update actions/download-artifact action to v4.2.1 (#17258) Update actions/setup-python action to v5.5.0 (#17260) Update actions/cache action to v4.2.3 (#17256) Update Swatinem/rust-cache action to v2.7.8 (#17255) Update actions/checkout action to v4.2.2 (#17257) ...
Summary
Fixes #17234
Test Plan
Add tests to functions/paremeters.md