Skip to content

fix TypeVar bound to type[X] resolves as object #3066#3068

Open
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:3066
Open

fix TypeVar bound to type[X] resolves as object #3066#3068
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:3066

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

@asukaminato0721 asukaminato0721 commented Apr 8, 2026

Summary

Fixes #3066

The bug was in quantified attribute lookup: a TypeVar bound like type[ShellComplete] was converted into a class-object base, but the helper only recognized instance-style bases and fell back to object.

Test Plan

add test

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

Diff from mypy_primer, showing the effect of this PR on open source code:

bokeh (https://github.com/bokeh/bokeh)
- ERROR src/bokeh/core/has_props.py:108:28-40: Object of class `object` has no attribute `__name__` [missing-attribute]

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

Primer Diff Classification

✅ 1 improvement(s) | 1 project(s) total | -1 errors

1 improvement(s) across bokeh.

Project Verdict Changes Error Kinds Root Cause
bokeh ✅ Improvement -1 missing-attribute quantified_bound_class()
Detailed analysis

✅ Improvement (1)

bokeh (-1)

This is a clear improvement. The TypeVar C = TypeVar('C', bound=type['HasProps']) means cls is always a subtype of type['HasProps'] — i.e., it is a class object (an instance of type or a subclass thereof). All instances of type have __name__ as a built-in attribute. The error message indicates pyrefly was resolving the type of cls to object rather than correctly understanding the type['HasProps'] bound. This likely means pyrefly was not properly handling the type[X] construct in TypeVar bounds — possibly unwrapping or degrading it incorrectly. The PR fix allows pyrefly to correctly recognize that C bound to type['HasProps'] means cls is a class object with all attributes of type, including __name__, so cls.__name__ is now correctly recognized as valid.
Attribution: The change in pyrefly/lib/alt/attr.rs in the quantified_bound_class() method added a new match arm for AttributeBase1::ClassObject(class) that returns Some(class.[class_type()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/attr.rs).clone()) instead of falling through to None (which would default to object). This fix ensures that when a TypeVar has a bound like type[HasProps], attribute lookups resolve against the actual class type rather than object. The test case test_bounded_typevar_specific_type_attribute_access in pyrefly/lib/test/generic_restrictions.rs confirms this is the intended behavior.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (1 LLM)

@asukaminato0721 asukaminato0721 marked this pull request as ready for review April 8, 2026 01:28
Copilot AI review requested due to automatic review settings April 8, 2026 01:28
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes pyrefly’s attribute lookup for TypeVars whose bound is a parameterized type[...] (e.g. TypeVar(..., bound=type[C])), addressing issue #3066 where such bounds previously degraded to object and caused spurious “no attribute” errors.

Changes:

  • Add a regression test asserting cls.name is accessible when T is bounded to type[ShellComplete].
  • Update attribute-bound handling to preserve the underlying class when the bound normalizes to a class-object base.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pyrefly/lib/test/generic_restrictions.rs Adds a regression testcase for TypeVar bounded to type[SpecificClass] attribute access.
pyrefly/lib/alt/attr.rs Adjusts quantified_bound_class to treat ClassObject bounds as the underlying class instead of falling back to object.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2078 to +2080
// Bounds like `type[C]` become class-object bases; keep their underlying class
// instead of falling back to `object`.
AttributeBase1::ClassObject(class) => Some(class.class_type().clone()),
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Handling AttributeBase1::ClassObject by returning its underlying ClassType will cause TypeVars with bounds like type[C] to use instance attribute lookup on C (via AttributeBase1::Quantified), which changes method binding and visibility rules. This can incorrectly allow calling instance methods as if they were bound (e.g. def f[T: type[C]](cls: T): cls.inst()), and can expose instance-only attributes that should be rejected on class objects. Consider propagating a class-object base for type[C] bounds (e.g. producing AttributeBase1::ClassObject(ClassBase::Quantified(...)) / using get_bounded_quantified_class_attribute) so class-object attribute semantics are preserved while still allowing cls.name.

Suggested change
// Bounds like `type[C]` become class-object bases; keep their underlying class
// instead of falling back to `object`.
AttributeBase1::ClassObject(class) => Some(class.class_type().clone()),
// Do not erase class-object semantics for bounds like `type[C]` by
// returning the underlying `ClassType`, since that would cause
// instance-style attribute lookup on `C`.
AttributeBase1::ClassObject(_) => None,

Copilot uses AI. Check for mistakes.
Comment on lines +268 to +274

T = TypeVar("T", bound=type[ShellComplete])

def add_completion_class(cls: T, label: str | None = None) -> T:
if label is None:
assert_type(cls.name, str)
label = cls.name
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

This regression test covers class-body attribute access (cls.name) for T bounded to type[ShellComplete], but it doesn’t assert the rest of the class-object lookup semantics (notably that instance methods remain unbound and instance-only attributes remain not-visible on the class). Given the attr-lookup change, please extend this testcase (or add a companion one) to ensure that cls does not typecheck as a ShellComplete instance (e.g., calling an instance method without providing self should still error, and accessing an attribute only initialized in __init__ should still be rejected).

Suggested change
T = TypeVar("T", bound=type[ShellComplete])
def add_completion_class(cls: T, label: str | None = None) -> T:
if label is None:
assert_type(cls.name, str)
label = cls.name
def __init__(self) -> None:
self.instance_only = "instance"
def instance_method(self) -> str:
return self.instance_only
T = TypeVar("T", bound=type[ShellComplete])
def add_completion_class(cls: T, label: str | None = None) -> T:
if label is None:
assert_type(cls.name, str)
label = cls.name
cls.instance_method() # E: Missing argument `self`
assert_type(cls.instance_only, str) # E: assert_type(Unknown, str) failed # E: Attribute `instance_only` of class `ShellComplete` is not visible on the class

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeVar bound to type[X] resolves as object

2 participants