[red-knot] improve function/bound method type display #17294
Conversation
mishamsk
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
|
|
What does Red Knot say in this case? def foo(): ...
a = foo
reveal_type(a) # ?If the message is also |
it does... I am conflicted. I agree that it may be confusing, and that's probably the reason why pyright/mypy do not carry on the name from the definition, they just have a different default text for reveal_type which always includes the binding. But on the other hand, this alternative approach correctly leads to the original definition, which may be an interesting way to triage binding source ;-) I'll wait for more opinions and will do a second take based on all of the feedback |
|
I'd prefer if we could use |
|
I don't think it's confusing; it's an accurate reflection of runtime behavior: >>> def foo(): ...
...
>>> a = foo
>>> a.__name__
'foo'
>>> a
<function foo at 0x105212840>Functions carry their original name-at-definition with them, even if reassigned to a different name. |
This could work for functions, and I don't mind it, but it won't give us this property universally. How would we display the type of a bound method? We want to show the signature without |
|
Here's a proposal: regular function: It's less explicit than if we include the text "bound method", but I think it's pretty intuitive, and it is valid Python syntax? (edit: well I guess not quite, Also, a nit: PR title claims it changes class literal display, too, but the PR does not. The |
That does seem to be a very good choice. It reminds me of Kotlin's extension functions: interface A<T> { val v: T }
fun A<Int>.foo() = println("Int: $v")
fun A<String>.foo() = println("String: $v")
That's true. Perhaps a better suggestion would be to change the entire message as a whole: reveal_type(a) # Revealed type for variable `a` is `def foo() -> Unknown`
# ^^^^^^^^^^^^ Customized for each kind of expression |
Not a strong preference but another possibility is to use
I'd be fine to include the expression (similar to Pyright) but that doesn't seem related to this PR. We should avoid "variable" because |
Agreed on both counts.
I think this was already acknowledged by the "customized for each kind of expression" note. But we should discuss the design of this feature in its own issue, not here.
I don't like this because it wrongly suggests that |
|
I agree with Carl rgd self. How about this: regular function: ? Rgd @MichaReiser point about being able to render as markdown. If we really want to make it work, we would need to do: |
mishamsk
changed the title
I'm personally fine with this option, too. I don't think I have a clear understanding of how far we can stretch away from "valid Python syntax" and still get nice rendering. Perhaps @MichaReiser or @dhruvmanila can offer more details here. I don't think we want to require fully valid Python syntax; adding a trailing |
|
The lsp specification is pretty vague but I expect that most lsp do syntax highlighting similar to what you get in markdown files. That means incomplete syntax is fine. The part I'm more worried about are unions over functions because a def keyword in those positions is simply invalid. The easiest way to test this is to make the change and change the markup renditions in on hover to use a python code block (there are other types that use <> syntax that prevent us from switching to python just yet) |
|
I actually think we could probably even eliminate the But I'm not sure how any of these would look if syntax-highlighted as Python inside a union (separated by |
|
@MichaReiser @carljm screenshots! so the suggested and as Carl said, dots don't influence rendering at all: bound methods are not all black and white either, so not too bad IMHO: and unions too! So all in all I suggest moving forward with this:
let me know if any objections left. Also - I assume I am NOT changing the code-fence language, Micha will do it at some point. Correct? |
|
Thanks for doing this experimentation to drive this forward! I think my remaining concern here is just about ambiguity of unions and intersection display; isn't it possible that two different types could both be rendered as Perhaps this is not a problem we need to deal with in the function-literal display, but rather in the union/intersection display by wrapping some types with possibly-ambiguous representations in extra parentheses? Other than this, I'm fine with the proposed display. |
ah, well, I faked the display when doing screenshots (just hardcoded two unioned signatures for a single callable). So seems like I can indeed proceed as unions/intersections are already handled. I’ll wait for formal ok from @MichaReiser and will go ahead and update all mdtests on top of most recent main |
mishamsk
force-pushed
the
17238-red-knot-improve-function-and-class-literal-type-display
branch
from
6bf5904 to
7f28d16
Compare
|
Sounds good to me. |
|
What's our plans for overloads? Neither mypy nor pyright has what I would consider a user-friendly display for overloads: from typing import overload
@overload
def f(x: str) -> int: ...
@overload
def f(x: int) -> str: ...
def f(x: int | str) -> int | str:
if isinstance(x, int):
return "foo"
return 42
reveal_type(f)Mypy says: Pyright says: What if instead of trying to cram the whole signature(s) into the top-level message, we displayed the signature as a subdiagnostic? That might work well for single-signature functions and multi-signature (overloaded) functions. It would also more clearly distinguish function-literal types from abstract For a single-signature function we could say: And for an overloaded function: |
I think we could do this for I think that we should do something similar to mypy and pyright, where we just show the signatures of all the overloads. It's a lot of information, but that's just because there's a lot of information there! I don't have strong feelings about the exact notation. |
|
Fair enough! I guess I'd still be interested in possibly special-casing |
I'm still not sure we should be reporting the whole signature in the default display... it makes error messages like this very long. To me, the message here was great as it was for a top-line summary. The signature is important in order to understand why the object is not assignable to parameter 2, but that information should be presented as a subdiagnostic IMO. Should we consider falling back to a more concise display if the signature is very long, like this? And then always reporting the full signature in a subdiagnostic where it's relevant to the reason the diagnostic was emitted? Or do we just need to be more careful about using the full display in top-line diagnostic messages like this in the future? |
|
@AlexWaygood For Pyright, this is perhaps not a problem, since if a user needed to see all signatures of a function, they would likely hover over its name rather than using |
I think this might be an interesting path to explore for hover implementation as we have the flexibility there i.e., using multi-line signature. Unions might be an issue here though.
Similarly, for hover implementation, we could split the parameter list across multiple lines similar to what |
I think we could consider this, but I don't think it should block this PR. For short signatures (and many are) it's clearly better to just get all the information at once, so I think that should be our starting point, and we can consider layering on something more complex for long signatures as a follow-up. |
mishamsk
force-pushed
the
17238-red-knot-improve-function-and-class-literal-type-display
branch
from
7f28d16 to
324ac30
Compare
|
well, this was much more painful than I expected. Also, noticed how weird the there are a couple of other rough edges, like those pointed out by @AlexWaygood but I'd rather merge this one and then do an incremental change than re-applying everything from scratch. |
There was a problem hiding this comment.
I think in 9/10 cases I'm seeing, this is a clear improvement. There are some issues I think we need to iron out, commented inline. I'm ok with doing them as follow-ups if necessary to avoid accumulating merge conflicts here, but some of them would be pretty hi-pri follow-ups (e.g. the display of function literals inside Literal[...] when in unions, the very weird-looking display when the self-type of a bound method is a function literal).
| @@ -1584,7 +1584,7 @@ Some attributes are special-cased, however: | |||
|
|
|||
| ```py | |||
| reveal_type(f.__get__) # revealed: <method-wrapper `__get__` of `f`> | |||
| reveal_type(f.__call__) # revealed: <bound method `__call__` of `Literal[f]`> | |||
| reveal_type(f.__call__) # revealed: bound method def f() -> Unknown.__call__(*args: Any, **kwargs: Any) -> Any | |||
There was a problem hiding this comment.
This looks like it needs some attention. It seems like we are emitting the representation of the function literal f as the bound self type for the __call__ method, but that ends up looking quite odd. I think it could work but we need to parenthesize at the very least.
| reveal_type(f.__call__) # revealed: bound method def f() -> Unknown.__call__(*args: Any, **kwargs: Any) -> Any | |
| reveal_type(f.__call__) # revealed: bound method (def f() -> Unknown).__call__(*args: Any, **kwargs: Any) -> Any |
It also looks like we have the wrong signature for __call__ here, but that's a separate issue, not part of this PR.
There was a problem hiding this comment.
thanks a lot for spotting, I think I missed this one yesterday. As-in it was replaced by my script and I haven't noticed it.
I'll look into this as part of this PR as it seems odd.
There was a problem hiding this comment.
had to add new variant of method wrapper, but it turned out to be very simple and cause no other changes. The code with these method wrappers screams to be abstracted as they all seem very similar, but a tolerable duplication for now IMHO
crates/red_knot_python_semantic/resources/mdtest/call/getattr_static.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/call/getattr_static.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/conditional.md
Outdated
Show resolved
Hide resolved
.../resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_invalid_`__getitem__`_methods.snap
Outdated
Show resolved
Hide resolved
update display per discussion update snapshots fix after pre-commit not so helpful "fix" fix hover tests
mishamsk
force-pushed
the
17238-red-knot-improve-function-and-class-literal-type-display
branch
from
84e9c0f to
45d2ad7
Compare
* main: (31 commits) [red-knot] Add some knowledge of `__all__` to `*`-import machinery (#17373) Update taiki-e/install-action digest to be7c31b (#17379) Update Rust crate mimalloc to v0.1.46 (#17382) Update PyO3/maturin-action action to v1.49.1 (#17384) Update Rust crate anyhow to v1.0.98 (#17380) dependencies: switch from `chrono` to `jiff` Update Rust crate bstr to v1.12.0 (#17385) [red-knot] Further optimize `*`-import visibility constraints (#17375) [red-knot] Minor 'member_lookup_with_policy' fix (#17407) [red-knot] Initial support for `dataclass`es (#17353) Sync vendored typeshed stubs (#17402) [red-knot] improve function/bound method type display (#17294) [red-knot] Move relation methods from `CallableType` to `Signature` (#17365) [syntax-errors] `await` outside async functions (#17363) [red-knot] optimize is_subtype_of for literals (#17394) [red-knot] add a large-union-of-string-literals benchmark (#17393) Update pre-commit dependencies (#17383) [red-knot] mypy_primer: Fail job on panic or internal errors (#17389) [red-knot] Document limitations of diagnostics-silencing in unreachable code (#17387) [red-knot] detect unreachable attribute assignments (#16852) ...
Summary
todo_type!now statically checks for no parens in the message to avoid issues between debug & release build testsMdtests are not updated yet per discussion. Here is the current knot vs. pyright vs. mypy:
Test Plan
many mdtests are changing