-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
[red-knot] Enhancing Diagnostics for Compare Expression Inference #13787
Comments
Yes, looks great, thank you for writing this up! |
One thing to consider is whether the diagnostic we want for your example is simply:
Or a more verbose variant where we give both the inner erroring types and the full types. Pyright gives both. I think we probably should as well, because the full outer type may also be inferred and non-obvious. So maybe something like:
|
I think there’s a subtle difference in the final diagnostic I expect.
As you know, in the current implementation, literals of different types are not compared directly. First, they are converted to So, the result would be:
It may not be intuitive because the user doesn’t know where the |
Messages from other tools: TypeScript Flow (Nice! We should consider something with our new diagnostics)
Pyright
|
Thanks for reasearching this!!!!! Would it be possible to share an example of how Flow’s diagnostics could be applied to this case? I’m a little unsure about which parts of Flow’s diagnostics we should apply to. By the way, pyright doesn't show inner-type either... 😢 |
Here's a link to flow's playground What I like about flow is that it renders the longer types, including those declared outside of the message header. I would have to install their CLI to see how that looks. But I do like that they don't try to squeeze everything into the message. |
I think something like Flow's diagnostic would be great, once we have a more complete diagnostics system in place that allows formatting code frames like that; I definitely wouldn't try to implement that now. I guess the one thing it suggests is that maybe the error return value should include both the erroring types and their nodes (or at least node locations). But I also think probably we don't need to worry about that yet, either. The main thing is to change the return value to |
Weird, I definitely thought I recently saw a pyright diagnostic for a case like this, where it stacked multiple errors for the innermost vs outer types. But now I can't figure out what code example I was looking at that produced that. |
…3819) ## Summary - Refactored comparison type inference functions in `infer.rs`: Changed the return type from `Option` to `Result` to lay the groundwork for providing more detailed diagnostics. - Updated diagnostic messages. This is a small step toward improving diagnostics in the future. Please refer to #13787 ## Test Plan mdtest included! --------- Co-authored-by: Carl Meyer <carl@astral.sh>
This issue was suggested by @carljm and is related to the following threads: #13712 (comment), #13781 (comment), #13712 (comment)
Current Problems
Currently, the
infer_binary_type_comparison
,infer_lexicographic_type_comparison
, andperform_rich_comparison functions
in infer.rs return anOption<Type>
, whereNone
indicates that the comparison is not defined.However, with the addition of comparison logic for Tuple and Union types, we’ve found that using
Option
may not provide sufficient diagnostic information.For example, consider the following code:
The Python runtime provides an appropriate diagnostic message:
In contrast, our current
Option
implementation would result in:(This is just my expected result; comparison between int and str isn’t implemented yet.)
Implementation
I’m considering a straightforward change to the following structure. Any suggestions are welcome!
The text was updated successfully, but these errors were encountered: