[flake8-type-checking] Add test cases for TC004

[gpilikin]

Add test cases for astral-sh#13965 (comment)

[Daverball]

LGTM

[Daverball]

Are you getting a false negative with your approach here as well or did you just copy over the snapshots from my PR?

I was kind of hoping your approach wouldn't lead to source order dependent false negatives.

[gpilikin]

I haven't dug deep into this yet - just added some test cases and ran cargo insta review.

[Daverball]

You'd need to run cargo insta test once to get updated snapshots. cargo insta review doesn't really do anything on its own without the temporary diff snapshot files.

But you can also just merge this and let the CI tell you if some of the snapshots need to be updated. (Or enable GitHub actions on your fork and trigger the CI using a force push)

[gpilikin]

Thanks, I'll try to do it later.

[gpilikin]

Hello. Ah, I see. I was doing it by creating snapshots via cargo test and then running cargo insta review. I didn’t know you could just run cargo insta test to create snapshots.

[Daverball]

That's a shame, but it's still better than without the fix, so I'll take what I can get. Maybe the source order dependency will be easier to fix as a follow-up with the additional contextual information your fix provides.

[Daverball]

Also cargo test does work, yes. But cargo insta test is more convenient, especially if there are multiple snapshots that need to be updated.

[gpilikin]

Lol)

I actually ran into this when fixing a rule related to unused imports. The solution was to search for all bindings by explicitly using the module name.
From what I understand, the imports are shadowing each other, making them harder to access.

Check the last two commits.
If I understand correctly, is this like in your PR?

[Daverball]

I think I used a similar approach in one of my attempts to fix this. I'm worried there are still other corner-cases that aren't handled correctly though, like the following example:

import a.b.c

TYPE_CHECKING = False
if TYPE_CHECKING:
    import a.b

class Foo(a.b.c.Bar):
    def baz() -> a.b.x: ...

Or the reverse or if you add more imports with different levels. Making each reference prioritize binding to the correct import for the purposes of this check is challenging.

It looks like a good start towards further improving results though.

[Daverball]

Actually, if what your fix does, is ensure that references point to the correct import, then you can probably make a much smaller patch and just iterate over all the bindings in the outermost loop, i.e. replace for binding_id in scope.binding_ids() with for _, binding_id in scope.all_bindings(), that should be faster than going from each binding back to its shadowed bindings.

[Daverball]

It might be more efficient to use the existing method on the semantic model to iterate through the shadowed bindings (unless your change makes those bindings not always shadow each other):

https://github.com/astral-sh/ruff/blob/91b7a570c2bd1c9e1cab894ded866e885f28946a/crates/ruff_python_semantic/src/model.rs#L2038-L2075

[Daverball]

Or rather as pointed out above, you might be able to revert everything and only have to replace scope.binding_ids() with scope.all_bindings().