I'd prefer to keep the error type named IterateError because I then don't have to guess what the error type is named: try_iterate -> IterateError, try_call -> CallError.

blegh, I much prefer IterationError 😄 because "iterate" just can't be used as a noun adjunct in the same way as "call" or "bool" (because it's a verb rather than a noun -- "call" can be both a verb or a noun depending on context, which is why that feels OK to me, but that's not true for "iterate")

I don't feel too strongly though. I can change this back if you strongly dislike the new name, or if others agree with you :-)

It's too much a detail to feel strongly about it but I'm interested in other opinions as well

I think I prefer the old message because it's more on the point
(Overall, messages should be as concise as possible).

I do think that we want to add a help text saying because Literal[1] doesn't have an __iter__ method and because Literal[1] doesn't have a __getitem__ method once our diagnostic system supports it.

I agree with all that. I actually think for all of these diagnostics, the top-line error message should probably just be either "Object of type is not iterable" or "Object of type may not be iterable".

Again, though, this PR isn't really about trying to get to perfect diagnostics right now -- its aim is really to distinguish between the different error cases and make sure that we're propagating the necessary information into `IterationError`` so that we can easily turn these into beautifully presented diagnostics when our infrastructure's there.

Overall, I think I'd prefer to leave this as-is for now. Otherwise it's the only IterationError diagnostic message that doesn't given an explanation, which seems inconsistent 😄 and I think long-term, the explanation probably shouldn't be part of the top-line message for any of these diagnostics

I added a TODO comment in df75629 saying that we probably don't want all the information in one long sentence. Though I think the TODO comment also applies to lots of our other diagnostics right now!

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.

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_semantic stops testing a markdown file as soon as it finds a failing snapshot in that markdown file. That meant that I had to run cargo test -p red_knot_python_semantic; cargo insta review several times before all snapshots for for.md were 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 :-)

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 ;)