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

[red-knot] assert that we don't create multiple definitions for one node #13085

Closed
carljm opened this issue Aug 23, 2024 · 3 comments · Fixed by #13214
Closed

[red-knot] assert that we don't create multiple definitions for one node #13085

carljm opened this issue Aug 23, 2024 · 3 comments · Fixed by #13214
Assignees
Labels
red-knot Multi-file analysis & type inference

Comments

@carljm
Copy link
Contributor

carljm commented Aug 23, 2024

I don't think this should happen, and if it does that suggests a bug. But a quick attempt to add the assert caused a panic, so apparently we are already relying on this. We should figure out why/where, fix it, and add the assert.

@carljm carljm added the red-knot Multi-file analysis & type inference label Aug 23, 2024
@carljm
Copy link
Contributor Author

carljm commented Aug 23, 2024

See #13075 (comment) for previous attempt to add the assert.

@dylwil3
Copy link
Collaborator

dylwil3 commented Aug 24, 2024

It looks like it's only one corpus example that panics with the new debug assertion (commenting it out makes the test pass):

async def foo():
    l = {k: v async for k, v in gen()}
    return [i for i in l]

A more minimal example that causes a panic is this:

{i: j for i, j in gen}

In each case the repeated definition is the Comprehension.

All of the following cause the test to fail for the same reason:

{i: j for i, j in gen}
{i for i, j in gen}
{0: 0 for i,j in gen}
{0 for i,j in gen}
[i for i,j in gen]
[j for i,j in gen]
[0 for i,j in gen]

These cases pass:

{x[0]: x[1] for x in gen}
{0: 0 for x in gen}

So it seems like issue is the tuple of elements... and indeed, if you remove the debug_assert_eq and inspect how many definitions get added for

[_ for a,b,c,d,e in gen]

you'll find it's 5.

But that's as far as I got and I should probably go to sleep 💤

@carljm
Copy link
Contributor Author

carljm commented Aug 24, 2024

That's good data; looks like the problem is we don't handle unpacking in comprehension iteration vars.

@dhruvmanila dhruvmanila self-assigned this Sep 2, 2024
dhruvmanila added a commit that referenced this issue Sep 4, 2024
## Summary

Part of #13085, this PR updates the comprehension definition to handle
multiple targets.

## Test Plan

Update existing semantic index test case for comprehension with multiple
targets. Running corpus tests shouldn't panic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants