Skip to content

Comments

[red-knot] detect unreachable attribute assignments#16852

Merged
sharkdp merged 20 commits intoastral-sh:mainfrom
mtshiba:unreachable-attribute-assignments
Apr 14, 2025
Merged

[red-knot] detect unreachable attribute assignments#16852
sharkdp merged 20 commits intoastral-sh:mainfrom
mtshiba:unreachable-attribute-assignments

Conversation

@mtshiba
Copy link
Collaborator

@mtshiba mtshiba commented Mar 19, 2025

Summary

This PR closes #15967.

Attribute assignments that are statically known to be unreachable are excluded from consideration for implicit instance attribute type inference. If none of the assignments are found to be reachable, an unresolved-attribute error is reported.

Test Plan

A test case marked as TODO now work as intended, and new test cases have been added.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 19, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Mar 19, 2025
@mtshiba mtshiba force-pushed the unreachable-attribute-assignments branch from bab7385 to f04ff66 Compare March 27, 2025 04:00
@mtshiba mtshiba force-pushed the unreachable-attribute-assignments branch from 96e6334 to a5c8278 Compare March 27, 2025 04:03
@mtshiba
Copy link
Collaborator Author

mtshiba commented Mar 27, 2025

While trying to solve the issue, I noticed that the following code does not pass:

class C:
    def __init__(self):
        def closure():
            self.a: str = 1
        closure()
        [... for self.b in range(1)]
        class D:
            self.c = 1

# should be OK
C().a
C().b
C().c

It appears that the SemanticIndexBuilder::register_attribute_assignment does not take inner scopes into account.
I'm trying to fix this, but this may be an issue that should be resolved in another PR.

mtshiba added 2 commits March 31, 2025 03:49
Replace
* `AttributeAssignment::Annotated` -> `DefinitionKind::AnnotatedAssignment`
* `AttributeAssignment::Unannotated` -> `DefinitionKind::Assignment`
* `AttributeAssignment::Iterable` -> `DefinitionKind::For`
* `AttributeAssignment::ContextManager` -> `DefinitionKind::WithItem`
@mtshiba mtshiba requested a review from dhruvmanila as a code owner March 30, 2025 19:18
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 30, 2025

CodSpeed Performance Report

Merging #16852 will not alter performance

Comparing mtshiba:unreachable-attribute-assignments (e52f9d6) with main (3aa3ee8)

Summary

✅ 32 untouched benchmarks

@mtshiba
Copy link
Collaborator Author

mtshiba commented Mar 30, 2025

The commit I just made completely removes SemanticIndex::attribute_assignments/AttributeAssignment.
Attribute assignments are now stored in SemanticIndex::all_definitions.

I noticed that we can also track attribute assignments in comprehension/named/augmented assignments as well, so I will be working on that.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't finished my review but I have to head out, so submitting the comments I have so far. Thanks for your work on this!

Given the false positives observed, I'm not sure we should keep the possibly-unbound-attribute checks, but I'm still thinking about whether there is a satisfactory way to address the false positive cases. It may have some overlap with https://github.com/astral-sh/ruff/issues/15777

@mtshiba mtshiba force-pushed the unreachable-attribute-assignments branch from fe12755 to 34965a9 Compare April 3, 2025 06:02
@mtshiba
Copy link
Collaborator Author

mtshiba commented Apr 4, 2025

Support for attribute assignments in comprehension/aug-assign/named-expression is incomplete, but considering that it is still not implemented in the main branch and that it was not originally planned in this PR, this may be work that should be done in another PR.

I believe that the implementation of the feature originally planned in this PR is almost complete and ready for final review.

@mtshiba mtshiba requested a review from BurntSushi as a code owner April 11, 2025 16:16
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me! It turns ClassLiteralType::implicit_instance_attribute into even more of a monster method, but I think I'm ok with that for now: it has a clear responsibility, and the logic in it is well encapsulated; if we need to break it up for better reuse in future, we can do that.

@sharkdp if you have a chance, I'd like to make sure this looks good to you as well.

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great — thank you very much. I have just one minor comment, which I can also resolve myself before merging this.

@sharkdp sharkdp merged commit dfd8eae into astral-sh:main Apr 14, 2025
22 checks passed
@mtshiba mtshiba deleted the unreachable-attribute-assignments branch April 14, 2025 12:57
dcreager added a commit that referenced this pull request Apr 15, 2025
* main: (31 commits)
  [red-knot] Add some knowledge of `__all__` to `*`-import machinery (#17373)
  Update taiki-e/install-action digest to be7c31b (#17379)
  Update Rust crate mimalloc to v0.1.46 (#17382)
  Update PyO3/maturin-action action to v1.49.1 (#17384)
  Update Rust crate anyhow to v1.0.98 (#17380)
  dependencies: switch from `chrono` to `jiff`
  Update Rust crate bstr to v1.12.0 (#17385)
  [red-knot] Further optimize `*`-import visibility constraints (#17375)
  [red-knot] Minor 'member_lookup_with_policy' fix (#17407)
  [red-knot] Initial support for `dataclass`es (#17353)
  Sync vendored typeshed stubs (#17402)
  [red-knot] improve function/bound method type display (#17294)
  [red-knot] Move relation methods from `CallableType` to `Signature` (#17365)
  [syntax-errors] `await` outside async functions (#17363)
  [red-knot] optimize is_subtype_of for literals (#17394)
  [red-knot] add a large-union-of-string-literals benchmark (#17393)
  Update pre-commit dependencies (#17383)
  [red-knot] mypy_primer: Fail job on panic or internal errors (#17389)
  [red-knot] Document limitations of diagnostics-silencing in unreachable code (#17387)
  [red-knot] detect unreachable attribute assignments (#16852)
  ...
dhruvmanila added a commit that referenced this pull request Apr 19, 2025
## Summary

This PR is a follow-up to #16852.

Instance variables bound in comprehensions are recorded, allowing type
inference to work correctly.

This required adding support for unpacking in comprehension which
resolves #15369.

## Test Plan

One TODO in `mdtest/attributes.md` is now resolved, and some new test
cases are added.

---------

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[red-knot] Detect unreachable attributes assignments

5 participants