Skip to content
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

C416 incorrectly flags dict comprehension based on dict with tuple keys #13625

Open
henribru opened this issue Oct 4, 2024 · 3 comments
Open
Labels
rule Implementing or modifying a lint rule type-inference Requires more advanced type inference.

Comments

@henribru
Copy link

henribru commented Oct 4, 2024

https://play.ruff.rs/4d78b865-9778-415b-9303-377a3fdff673

d1 = {(1, 2): 3, (3, 4): 5}
d2 = {x: y for x, y in d1}

C416 suggests changing d2 to dict(d1), which is incorrect because this will just copy d1.

This seems like an intractable problem to solve in general because you'd have to know whether the thing being iterated over is a dict. In a case like this I suppose it could be done with local type inference, though. SIM118 mentions acting differently based on whether it can statically know something to be a dict, so I assume that's something Ruff is already capable of.

The fix is thankfully already marked as unsafe for a different reason, so this isn't a huge problem. But it seems worth documenting this case as another reason for marking it as unsafe.

@zanieb
Copy link
Member

zanieb commented Oct 4, 2024

Ah the classic case where the dict keys are tuples. I think we could do better here with type inference, yeah.

@zanieb zanieb added rule Implementing or modifying a lint rule type-inference Requires more advanced type inference. labels Oct 4, 2024
@zanieb
Copy link
Member

zanieb commented Oct 4, 2024

This is sort of a false positive of the rule not a fix safety problem though.

@henribru
Copy link
Author

henribru commented Oct 4, 2024

This is sort of a false positive of the rule not a fix safety problem though.

Well, sure, but in general the only way to avoid false positives for this is to only flag it if you can statically determine that it's not a dict, right? But that would make the rule apply much more rarely, so the more pragmatic solution seems like keeping the fix as unsafe and avoiding flagging it if you can statically determine that it's a dict. Not sure if Ruff generally prefers correctness or more broadly applicable rules, though.

zanieb added a commit that referenced this issue Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule type-inference Requires more advanced type inference.
Projects
None yet
Development

No branches or pull requests

2 participants