Ok, I looked into the diagnostics situation, and it is going to need some attention here. A bit of background will be needed to understand the issue.

We have type inference queries at several granularities: expression (but only some "standalone" expressions, so we don't create too many Salsa ingredients), definition (that is, a value assigned to some name), and scope. We use the definition granularity queries to do lazy type inference of names from scopes we aren't actually checking (e.g. imported libraries). When we are actually checking a file, we run the scope-level inference queries for every scope in the file, and it is those queries where we aggregate and emit diagnostics.

So the intent when we run scope-level inference is that as we walk the AST, anywhere we hit a node that is a definition of a name, we use self.infer_definition to find that Definition by key, query its types, and then merge those types and diagnostics into the scope-level TypeInference. What this achieves is that we never double-infer the same types (even if we first lazily query the type of a name from a scope, and then later do full scope type inference on that scope), because we always use the (Salsa-cached) infer_definition_types query whenever we hit a definition.

What is currently happening in this PR is that when we find an unpacked with statement in scope-level inference, we don't eagerly run infer_definition_types for the definitions of the unpack-target names at all. This is not good, because it means we are implicitly depending on a later load of one of those names to actually run infer_definition_types, go through the Unpacker, and catch any unpack errors.

We should add a test like this to demonstrate this:

class ContextManager:
    def __enter__(self) -> tuple[int, str]:
        return (1, "a")

    def __exit__(self, exc_type, exc_value, traceback) -> None:
        pass

with ContextManager() as (a, b, c):
    pass

This should emit an error about trying to unpack two elements into three, but currently in this PR it does not.

The fix is that this PR needs to update TypeInferenceBuilder::infer_with_statement, and currently doesn't; there's even a relevant TODO statement there. In the case where the target is not a simple Name, we are currently just inferring the type of the context expression (just once! which is why you currently can't make duplicate diagnostics happen, regardless of first) and then we visit the optional-vars, and nothing in that visit will ever look up any of the definitions or run self.infer_definition() on them.

The right fix for this involves fixing astral-sh/ty#185, since this duplicate-definition thing is already broken for for statements, as we discovered yesterday. But that fix is not going to be simple, and I don't think we should try to fix it in this PR.

Instead, I think the goal for this PR should be to emulate the current handling of for statements, and be just as broken as for in this regard, and then the fix for astral-sh/ty#185 will fix them both in the same way. That will mean, in this PR, updating TypeInferenceBuilder::infer_with_statement to look a lot more like TypeInferenceBuilder::infer_for_statement, and use TypeInferenceBuilder::infer_target.

Hopefully that all made sense! If not, feel free to ask questions, here or in Discord. Or if it doesn't make sense and you don't have time to wrap your head around it and update the PR, that's totally fine too! Just let me know and one of us can pick it up.

@ericmarkmartin Awesome work! This is a really great first contribution. Thank you for taking this on and also surfacing an important issue related to the unpacking diagnostics.