[red-knot] Rewrite Type::try_iterate() to improve type inference and diagnostic messages#16321
Conversation
AlexWaygood
requested review from
MichaReiser,
carljm and
sharkdp
as code owners
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
2 times, most recently
from
85abfd5 to
2c8bab5
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
The error messages look great. Although they're all rather long now. It makes me wonder if now is the right time to improve them or if we should just wait for when we have notes (or another mechanism to explain why a certain diagnostic was created) to make the improvement.
Either way. I'm concerned about having 10 IterateError variants. It doesn't seem to scale well and I'm somewhat convinced that we'll have to retain even more information for diagnostics in the future (e.g. the range where the __getitem__ and __iterate__ method are defined).
This makes me believe that we should not distinguish between all those variants in IterateError but instead redo some of the iterate logic in the IterateError::report_diagnostic.
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
2 times, most recently
from
1bce75e to
c08746f
Compare
Thanks!
Yeah. The main reasons why I wanted to do something now were:
I looked again following your comments, and managed to reduce it to 4 variants without sacrificing the precision of our type inference or our diagnostic messages. |
...ntic/resources/mdtest/snapshots/unpacking.md_-_Unpacking_-_Right_hand_side_not_iterable.snap
Show resolved
Hide resolved
|
|
||
| ## With non-callable iterator | ||
|
|
||
| <!-- snapshot-diagnostics --> |
There was a problem hiding this comment.
I'm curious to hear your take on this and how you decided to use snapshoting because it's not clear to me when we want to use snapshot tests. I've found updating the messages very painful when working on unsupported-bool-conversion but having the assertions (and messages) inline is significantely more readable and avoids bugs slipping through by accepting the snapshot tests.
I think it's a failure if we start using snapshot tests everywhere (or for a large majority of tests) because the experience is than very close to what we have from Ruff. Instead, we should use them mainly to validate that the diagnostic ranges are correct.
There was a problem hiding this comment.
It's a great question.
Overall I feel like mdtest (as it is currently -- obviously we can keep improving it!) is fantastic in a lot of ways:
- It allows us to write tests very concisely and declaratively
- It's very easy to make lots of assertions on which types are revealed in which cases
- It's very easy to make lots of assertions that "this Python code leads us to emit an error on that line with error code X".
What I think mdtest isn't very well suited to right now is making assertions on the full error message:
- It's very tedious to have to go through all locations and update the asserted error message if you make a change to the error message
- Even if you do make an assertion on the full error message, you don't see the full details of how the diagnostic will be rendered to the user. And as we've been discussing, that seems pretty relevant here, since cramming all the information into one sentence obviously isn't the ideal way of presenting these diagnostics.
- [This one is very fixable] I was pretty surprised that my changes didn't initially lead to any tests failing, even though I had changed a bunch of error messages. That's because currently if the error message is "Object X is not iterable because foo bar baz" and the assertion is
error: [not-iterable] "Object X is not iterable", the assertion will pass, since mdtest just checks whether the actual error message contains the asserted errror message (it doesn't check that the asserted error message is a fullmatch).
So using snapshots felt like a good fit for a lot of these tests, where what we really want to test is how the information is reported to the user, and where we expect that the presentation of the diagnostics will continue to change in the future. However, I do agree that it's not ideal:
- It would be so much nicer with inline snapshots; having them far away from the Python snippets makes it much harder to tell whether the snapshot is correct or incorrect
- It's much easier to just blindly accept the snapshot without checking that it's correct
- Our current snapshotting setup for mdtest isn't ideal.
cargo test -p red_knot_python_semanticstops testing a markdown file as soon as it finds a failing snapshot in that markdown file. That meant that I had to runcargo test -p red_knot_python_semantic; cargo insta reviewseveral times before all snapshots forfor.mdwere updated, which was pretty frustrating.
All told, I'm also not sure that using this much snapshotting is a great idea right now. I'm happy to switch it to standard mdtest assertions on full diagnostic messages if you'd prefer it :-)
There was a problem hiding this comment.
Thanks for the detailed explanation. I'd probably create one snapshot test for each unique diagnostic rendering variant. All tests that only enforce type inference feature should not use snaphshot testing. I leave it up to you to assess whether that's the case.
I suspect that @carljm has opinions on this as well ;)
There was a problem hiding this comment.
I'd probably create one snapshot test for each unique diagnostic rendering variant.
That makes sense to me. I did a quickish audit and removed a few hundred lines of snapshots. We're pretty much there on this PR now, I think.
There was a problem hiding this comment.
I don't think I have a lot to add here. As far as the current state, I think I basically agree with what Micha suggests above.
This thread did prompt me to add a few thoughts over on https://github.com/astral-sh/ruff/issues/16255
Regarding messages in # error: comments being a "contains" check rather than a "full match", that was intentional, the idea being to reduce update pain by only asserting the critical information that a certain test wants to ensure is present in the message. I'm not sure this works out in practice, though; we almost always use the full message, and it reads pretty oddly if you don't. So I'd be OK changing this to full match.
There was a problem hiding this comment.
Regarding messages in
# error:comments being a "contains" check rather than a "full match", that was intentional, the idea being to reduce update pain by only asserting the critical information that a certain test wants to ensure is present in the message. I'm not sure this works out in practice, though; we almost always use the full message, and it reads pretty oddly if you don't. So I'd be OK changing this to full match.
Yeah, I remember the reasons for the decision! But I also agree that it hasn't proven useful in practice. And as I say, it actually confused me briefly while working on this PR -- so I'd also favour changing it now to require a full match.
There was a problem hiding this comment.
Hmm, I tried out making this change and a surprising number of tests failed. I guess we use this feature more than I thought.
It's probably worth revisiting this once we have support for subdiagnostics/notes. Once those are implemented, we'll generally have shorter summary lines in our diagnostics, so it'll be less cumbersome to enforce a fullmatch on the summary line as part of mdtest.
…d diagnostic messages
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
2 times, most recently
from
4d26391 to
a9b984c
Compare
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
from
a9b984c to
2f53378
Compare
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks great to me, thank you! I think the number of variants of IterationErrorKind is reasonable, and the current PR does a good job of deferring any extra work that is solely for diagnostic purposes into the path where there definitely is a diagnostic to report (mostly by wrapping the underlying call error in the iteration error, which seems like a good pattern to me.)
|
|
||
| ## With non-callable iterator | ||
|
|
||
| <!-- snapshot-diagnostics --> |
There was a problem hiding this comment.
I don't think I have a lot to add here. As far as the current state, I think I basically agree with what Micha suggests above.
This thread did prompt me to add a few thoughts over on https://github.com/astral-sh/ruff/issues/16255
Regarding messages in # error: comments being a "contains" check rather than a "full match", that was intentional, the idea being to reduce update pain by only asserting the critical information that a certain test wants to ensure is present in the message. I'm not sure this works out in practice, though; we almost always use the full message, and it reads pretty oddly if you don't. So I'd be OK changing this to full match.
...snapshots/for.md_-_For_loops_-_Possibly_unbound_`__iter__`_and_bad_`__getitem__`_method.snap
Outdated
Show resolved
Hide resolved
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
from
55aa839 to
90016d9
Compare
AlexWaygood
force-pushed
the
alex/bad-iter-msg-2
branch
from
90016d9 to
31f4ebe
Compare
* main: (38 commits) [red-knot] Use arena-allocated association lists for narrowing constraints (#16306) [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321) Add issue templates (#16213) Normalize inconsistent markdown headings in docstrings (#16364) [red-knot] Better diagnostics for method calls (#16362) [red-knot] Add argfile and windows glob path support (#16353) [red-knot] Handle pipe-errors gracefully (#16354) Rename `venv-path` to `python` (#16347) [red-knot] Fixup some formatting in `infer.rs` (#16348) [red-knot] Restrict visibility of more things in `class.rs` (#16346) [red-knot] Add diagnostic for class-object access to pure instance variables (#16036) Add `per-file-target-version` option (#16257) [PLW1507] Mark fix unsafe (#16343) [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326) Extract class and instance types (#16337) Re-order changelog entries for 0.9.7 (#16344) [red-knot] Add support for `@classmethod`s (#16305) Update Salsa (#16338) Update Salsa part 1 (#16340) Upgrade Rust toolchain to 1.85.0 (#16339) ...
3 participants
Summary
This PR rewrites
Type::try_iterate()to greatly improve the quality of our type inference for edge cases involving iterables, and greatly improve the quality of our diagnostics.IterateErroris renamed toIterationError, andis expanded to have 10(!) inner variants. (Following code review, I've managed to reduce this to 4 inner variants.)Fixes #16272. Fixes #16123. Helps a lot with #13989.
Note: some of the diagnostic messages are now somewhat verbose. In the long run, I think we could probably change a lot of them to be diagnostics with concise messages but with several notes attached to them.
Test Plan
Several new mdtests, and lots of diagnostic snapshots