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.

Sign up for GitHub

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

Jump to bottom

Union rules trigger duplicates on nested violations #6285

Closed
charliermarsh opened this issue Aug 2, 2023 · 0 comments · Fixed by #6399
Closed

Union rules trigger duplicates on nested violations #6285

charliermarsh opened this issue Aug 2, 2023 · 0 comments · Fixed by #6399
Assignees
@charliermarsh
Labels
bug Something isn't working

Comments

@charliermarsh
Copy link
Member

In expression.rs, we have some logic to avoid checking nested Union operators, since our Union checks operate recursively (and so re-checking nested Union operators leads to duplicate diagnostics).

These checks don't quite work in .pyi files due to the way we process deferred nodes. We don't store the current expression stack when snapshotting the SemanticModel, because that would require us to store all expressions in an IndexVec. This is a known source of bugs.

We can store the expressions in an IndexVec, like we do for statements, but it will definitely hurt performance and increase memory usage.

As an example, if this is put in a .pyi file:

from typing import Union

Union[int, Union[int, int]]

You'll hit duplicate violations:

foo.pyi:3:22: PYI016 Duplicate union member `int`
foo.pyi:3:27: PYI016 Duplicate union member `int`
foo.pyi:3:27: PYI016 Duplicate union member `int`
@charliermarsh charliermarsh added the bug Something isn't working label Aug 2, 2023
This was referenced Aug 2, 2023
Merged
Merged
@charliermarsh charliermarsh self-assigned this Aug 7, 2023
@charliermarsh charliermarsh mentioned this issue Aug 7, 2023
Merged
charliermarsh added a commit that referenced this issue Aug 7, 2023
@charliermarsh
Extend nested union detection to handle bitwise or Union expressions (
26098b8 
#6399)

## Summary

We have some logic in the expression analyzer method to avoid
re-checking the inner `Union` in `Union[Union[...]]`, since the methods
that analyze `Union` expressions already recurse. Elsewhere, we have
logic to avoid re-checking the inner `|` in `int | (int | str)`, for the
same reason.

This PR unifies that logic into a single method _and_ ensures that, just
as we recurse over both `Union` and `|`, we also detect that we're in
_either_ kind of nested union.

Closes #6285.

## Test Plan

Added some new snapshots.
durumu pushed a commit to durumu/ruff that referenced this issue Aug 12, 2023
@charliermarsh @durumu
Extend nested union detection to handle bitwise or Union expressions (
4924553 
astral-sh#6399)

## Summary

We have some logic in the expression analyzer method to avoid
re-checking the inner `Union` in `Union[Union[...]]`, since the methods
that analyze `Union` expressions already recurse. Elsewhere, we have
logic to avoid re-checking the inner `|` in `int | (int | str)`, for the
same reason.

This PR unifies that logic into a single method _and_ ensures that, just
as we recurse over both `Union` and `|`, we also detect that we're in
_either_ kind of nested union.

Closes astral-sh#6285.

## Test Plan

Added some new snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Extend nested union detection to handle bitwise or Union expressions astral-sh/ruff
1 participant
@charliermarsh