Skip to content

fix Doesn't allow list["A|B"] #3193#3195

Open
asukaminato0721 wants to merge 4 commits intofacebook:mainfrom
asukaminato0721:3193
Open

fix Doesn't allow list["A|B"] #3193#3195
asukaminato0721 wants to merge 4 commits intofacebook:mainfrom
asukaminato0721:3193

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

Summary

Fixes #3193

binding type-argument subscripts as type expressions even in value context, so list["Tomato | Cucumber"]([t]) no longer errors and forward-ref strings get parsed.

Test Plan

add test

@meta-cla meta-cla Bot added the cla signed label Apr 21, 2026
@asukaminato0721 asukaminato0721 marked this pull request as ready for review April 21, 2026 14:03
Copilot AI review requested due to automatic review settings April 21, 2026 14:03
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 handling of generic-alias subscripts used in value context (e.g. list["A | B"]([x])) so that string type arguments are treated as forward references and parsed/bound as type expressions, matching other type checkers and resolving #3193.

Changes:

  • Adds a binder special-case to treat certain Expr::Subscript nodes as containing type expressions even in value context.
  • Adds a regression test covering list["Tomato | Cucumber"]([t]) with from __future__ import annotations.

Reviewed changes

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

File Description
pyrefly/lib/binding/expr.rs Ensures type-argument slices of certain special subscripts are bound as types in value context (enabling forward-ref parsing).
pyrefly/lib/test/typeform.rs Adds regression test for forward-ref string type arguments in generic-alias value-context usage.

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

Comment thread pyrefly/lib/binding/expr.rs
Comment thread pyrefly/lib/binding/expr.rs Outdated
@github-actions github-actions Bot added size/s and removed size/s labels Apr 21, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions github-actions Bot added size/m and removed size/s labels Apr 21, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kinto0
Copy link
Copy Markdown
Contributor

kinto0 commented Apr 24, 2026

thanks for the fix! seems targeted enough and good at first glance. assigning to @stroxler but feel free to reassign if you have too much on your plate

@github-actions github-actions Bot added size/m and removed size/m labels May 1, 2026
@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented May 1, 2026

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D103326319.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

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

tornado (https://github.com/tornadoweb/tornado)
- ERROR tornado/routing.py:296:9-15: Expected a type form, got instance of `Literal['Rule']` [not-a-type]
- ERROR tornado/routing.py:298:26-35: Expected a type form, got instance of `Literal['Matcher']` [not-a-type]
- ERROR tornado/routing.py:299:26-35: Expected a type form, got instance of `Literal['Matcher']` [not-a-type]
- ERROR tornado/routing.py:300:26-35: Expected a type form, got instance of `Literal['Matcher']` [not-a-type]
- ERROR tornado/routing.py:349:55-64: Argument `dict[str, Any] | str | Unknown` is not assignable to parameter `target_kwargs` with type `dict[str, Any] | None` in function `Rule.__init__` [bad-argument-type]
+ ERROR tornado/routing.py:349:55-64: Argument `dict[str, Any] | str | Any` is not assignable to parameter `target_kwargs` with type `dict[str, Any] | None` in function `Rule.__init__` [bad-argument-type]
- ERROR tornado/routing.py:349:55-64: Argument `dict[str, Any] | str | Unknown` is not assignable to parameter `name` with type `str | None` in function `Rule.__init__` [bad-argument-type]
+ ERROR tornado/routing.py:349:55-64: Argument `dict[str, Any] | str | Any` is not assignable to parameter `name` with type `str | None` in function `Rule.__init__` [bad-argument-type]
- ERROR tornado/routing.py:351:33-38: Argument `dict[str, Any] | str | Unknown` is not assignable to parameter `matcher` with type `Matcher` in function `Rule.__init__` [bad-argument-type]
+ ERROR tornado/routing.py:351:33-38: Argument `Matcher | dict[str, Any] | str | Any` is not assignable to parameter `matcher` with type `Matcher` in function `Rule.__init__` [bad-argument-type]
- ERROR tornado/routing.py:351:33-38: Argument `dict[str, Any] | str | Unknown` is not assignable to parameter `target_kwargs` with type `dict[str, Any] | None` in function `Rule.__init__` [bad-argument-type]
+ ERROR tornado/routing.py:351:33-38: Argument `Matcher | dict[str, Any] | str | Any` is not assignable to parameter `target_kwargs` with type `dict[str, Any] | None` in function `Rule.__init__` [bad-argument-type]
- ERROR tornado/routing.py:351:33-38: Argument `dict[str, Any] | str | Unknown` is not assignable to parameter `name` with type `str | None` in function `Rule.__init__` [bad-argument-type]
+ ERROR tornado/routing.py:351:33-38: Argument `Matcher | dict[str, Any] | str | Any` is not assignable to parameter `name` with type `str | None` in function `Rule.__init__` [bad-argument-type]
- ERROR tornado/routing.py:353:49-53: Argument `Rule | tuple[str | Unknown, Any] | tuple[str | Unknown, Any, dict[str, Any]] | tuple[str | Unknown, Any, dict[str, Any], str] | Unknown` is not assignable to parameter `rule` with type `Rule` in function `RuleRouter.process_rule` [bad-argument-type]

altair (https://github.com/vega/altair)
- ERROR altair/datasets/_cache.py:153:61-71: Expected a type form, got instance of `Literal['_Dataset']` [not-a-type]
- ERROR altair/datasets/_cache.py:153:73-83: Expected a type form, got instance of `Literal['Metadata']` [not-a-type]
- ERROR altair/datasets/_cache.py:225:62-72: Expected a type form, got instance of `Literal['_Dataset']` [not-a-type]
- ERROR altair/datasets/_cache.py:225:74-85: Expected a type form, got instance of `Literal['_FlSchema']` [not-a-type]

discord.py (https://github.com/Rapptz/discord.py)
- ERROR discord/app_commands/translator.py:119:61-85: Expected a type form, got instance of `Literal['Command[Any, ..., Any]']` [not-a-type]
- ERROR discord/app_commands/translator.py:119:87-100: Expected a type form, got instance of `Literal['ContextMenu']` [not-a-type]

django-test-migrations (https://github.com/wemake-services/django-test-migrations)
- ERROR django_test_migrations/db/backends/registry.py:16:10-37: Expected a type form, got instance of `Literal['BaseDatabaseConfiguration']` [not-a-type]

prefect (https://github.com/PrefectHQ/prefect)
- ERROR src/prefect/deployments/schedules.py:14:53-69: Expected a type form, got instance of `Literal['SCHEDULE_TYPES']` [not-a-type]
- ERROR src/prefect/server/database/orm_models.py:1510:38-54: Expected a type form, got instance of `Literal['sa.Column[Any]']` [not-a-type]

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

Primer Diff Classification

✅ 5 improvement(s) | 5 project(s) total | +2, -16 errors

5 improvement(s) across tornado, altair, discord.py, django-test-migrations, prefect.

Project Verdict Changes Error Kinds Root Cause
tornado ✅ Improvement +2, -7 bad-argument-type, not-a-type ensure_expr()
altair ✅ Improvement -4 not-a-type ensure_type_impl()
discord.py ✅ Improvement -2 not-a-type pyrefly/lib/binding/expr.rs
django-test-migrations ✅ Improvement -1 not-a-type ensure_type_impl()
prefect ✅ Improvement -2 not-a-type ensure_type_impl()
Detailed analysis

✅ Improvement (5)

tornado (+2, -7)

Net change: -7 errors (all false positives) +2 errors (confirmed by pyright, real type-checking difficulty with tuple union unpacking). This is a clear improvement. The core fix correctly handles forward references in type subscripts per https://typing.readthedocs.io/en/latest/spec/annotations.html#string-annotations. The 2 new errors are more accurate versions of previously-existing errors (replacing Unknown with Any).
Attribution: The change to ensure_expr() in pyrefly/lib/binding/expr.rs (the new Expr::Subscript arm) now binds type-argument subscripts as type expressions even in value context. This correctly resolves forward-reference strings like "Rule" and "Matcher" inside Union["Rule", ...] and tuple[Union[str, "Matcher"], Any], eliminating the 4 not-a-type false positives and the 3 cascade bad-argument-type errors with Unknown. The 2 new bad-argument-type errors are the same underlying issue but with correct type resolution (Any instead of Unknown), now matching pyright's behavior.

altair (-4)

All four removed errors are false positives that pyrefly was incorrectly reporting. The expressions dict["_Dataset", "Metadata"] and dict["_Dataset", "_FlSchema"] on lines 153 and 225 use string forward references as type arguments to the dict generic alias in default parameter values. These strings refer to TypeAlias definitions (_Dataset on line 51, _FlSchema on line 52) and an imported type (Metadata imported on line 36). At runtime, dict["_Dataset", "Metadata"] creates a types.GenericAlias object, and the strings serve as forward references to types not yet available at runtime (they're defined under TYPE_CHECKING). Pyrefly was incorrectly treating these string subscript arguments as runtime Literal string values rather than recognizing them as type-level forward references. The PR fix in pyrefly/lib/binding/expr.rs correctly identifies that subscript arguments to known generic types (like dict) should be bound as type expressions, enabling forward-reference string parsing. This allows pyrefly to resolve "_Dataset" to the _Dataset type alias and "Metadata" / "_FlSchema" to their respective types, eliminating the false not-a-type errors.
Attribution: The change in pyrefly/lib/binding/expr.rs in the new Expr::Subscript arm of ensure_expr is directly responsible. Previously, subscript slices in value context were always bound as value expressions, so dict["_Dataset", "Metadata"] treated the strings as literal values. The new code checks if the subscript's base is a known generic type (matching against SpecialExport::BuiltinsDict, SpecialExport::TypingMapping, etc.) and if so, binds the slice as a type expression via ensure_type_impl(). This allows forward-reference strings like "_Dataset" and "Metadata" to be correctly parsed as type forms.

discord.py (-2)

The removed errors were false positives. On line 119, Union['Command[Any, ..., Any]', 'ContextMenu'] appears as the second type argument to TranslationContext[...]. The strings 'Command[Any, ..., Any]' and 'ContextMenu' are forward references — they are explicitly quoted because Command and ContextMenu are only imported under TYPE_CHECKING (lines 31-32) and would not be available at runtime. Note that while from __future__ import annotations is present on line 25, it affects annotations in function signatures and variable annotations by deferring their evaluation, but does not affect runtime expressions like type alias assignments on line 118-120. The explicit quoting here is necessary precisely because this is a runtime expression and the names aren't available at runtime. Pyrefly was incorrectly treating these quoted strings as Literal string instances rather than recognizing them as forward type references within the Union[...] type expression context, leading to the not-a-type errors. The PR fix in pyrefly/lib/binding/expr.rs correctly identifies that subscript slices of known generic type forms (like Union) should be bound as type expressions, enabling proper forward-reference resolution of string arguments.
Attribution: The change in pyrefly/lib/binding/expr.rs in the Expr::Subscript arm of ensure_expr is directly responsible. Previously, subscript slices in value context were always bound as value expressions (ensure_expr). The new code checks whether the subscript's base is a special export like Union, Optional, BuiltinsList, etc., and if so, binds the slice as a type expression (ensure_type_impl) instead. This means Union['Command[Any, ..., Any]', 'ContextMenu'] now has its string arguments parsed as forward references rather than treated as literal string instances.

django-test-migrations (-1)

This is a clear false positive removal. type['BaseDatabaseConfiguration'] is standard Python typing — a forward reference inside a generic type subscript. The old behavior incorrectly treated the string as a Literal value instead of parsing it as a type reference. The PR correctly fixes this by ensuring type-argument subscripts are bound as type expressions, allowing forward-reference strings to be resolved properly.
Attribution: The change in pyrefly/lib/binding/expr.rs that adds handling for Expr::Subscript in value context is responsible. Specifically, the new code checks if the subscript's value is a special export (including SpecialExport::BuiltinsType and SpecialExport::TypingType) and, if so, binds the slice as a type expression via self.[ensure_type_impl()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/binding/expr.rs) instead of as a regular expression. This means type['BaseDatabaseConfiguration'] now correctly parses the string 'BaseDatabaseConfiguration' as a forward reference type form rather than treating it as a literal string value.

prefect (-2)

Both errors were false positives where pyrefly failed to recognize forward-reference strings inside generic type subscripts as type expressions. The PR correctly fixes this by binding type-argument slices as type expressions when the subscript base is a known generic type, per https://typing.readthedocs.io/en/latest/spec/annotations.html#string-annotations.
Attribution: The change in pyrefly/lib/binding/expr.rs adds a new Expr::Subscript arm that detects when a subscript's base is a known generic/special export (like Union, Optional, Iterable, list, etc.) and binds the slice as a type expression via ensure_type_impl() instead of a value expression. This causes forward-reference strings in type argument positions to be correctly parsed as type forms, eliminating the false not-a-type errors.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (5 LLM)

Copy link
Copy Markdown
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'll try to get this merged

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.

Doesn't allow list["A|B"]

4 participants