[ty] Ignore rejected assignments for synthesized bindings#25340
Merged
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 91.94%. The percentage of expected errors that received a diagnostic held steady at 87.09%. The number of fully passing files held steady at 92/134. |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
0 | 1 | 0 |
| Total | 0 | 1 | 0 |
Raw diff:
paasta (https://github.com/yelp/paasta)
- paasta_tools/frameworks/native_service_config.py:229:17 error[invalid-argument-type] Argument to bound method `list.append` is incorrect: Expected `dict[str, str | dict[str, int | float]]`, found `dict[str, str | dict[str, int | float] | dict[str, int | float | list[dict[str, int]]]]`
charliermarsh
force-pushed
the
charlie/reject-assignment
branch
from
f0ebbbe to
2f1767b
Compare
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
Merging this PR will improve performance by 7.69%
Performance Changes
Tip Curious why this is faster? Use the CodSpeed MCP and ask your agent. Comparing 
Footnotes
|
Member
Author
|
Hate this diff. |
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
charliermarsh
force-pushed
the
charlie/non-never
branch
from
7a2dcf7 to
5153c2b
Compare
charliermarsh
force-pushed
the
charlie/reject-assignment
branch
from
af63962 to
b01775e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Member
Author
|
I was hoping to snipe someone into it by posting about how much I didn't like the approach in Slack. |
charliermarsh
force-pushed
the
charlie/non-never
branch
2 times, most recently
from
d9f1196 to
3921ec4
Compare
charliermarsh
force-pushed
the
charlie/non-never
branch
from
3921ec4 to
1436111
Compare
charliermarsh
force-pushed
the
charlie/reject-assignment
branch
from
b01775e to
08d6118
Compare
charliermarsh
force-pushed
the
charlie/non-never
branch
from
1436111 to
58247e2
Compare
sharkdp
approved these changes
May 29, 2026
Contributor
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
Took me a while to understand, but I think it makes sense to do it this way.
Thank you.
Contributor
There was a problem hiding this comment.
This looks completely unrelated to the changes in this PR... accidentally committed?
| /// The fallback type for missing expressions/bindings/declarations or recursive type inference. | ||
| cycle_recovery: Option<Type<'db>>, | ||
|
|
||
| /// Whether refinements derived from the right-hand side of this definition were discarded. |
Contributor
There was a problem hiding this comment.
"this definition" was a bit surprising to me given that a TypeInferenceBuilder could also be created for a non-definition region. Maybe it should be clarified here ("If the type inference region refers to a definition, ...")
| } | ||
|
|
||
| fn discard_descendant_refinements_for(&mut self, definition: Definition<'db>) { | ||
| if matches!(self.region, InferenceRegion::Definition(region) if region == definition) { |
Contributor
There was a problem hiding this comment.
Nit: region is not a region
Suggested change
| if matches!(self.region, InferenceRegion::Definition(region) if region == definition) { | |
| if matches!(self.region, InferenceRegion::Definition(d) if d == definition) { |
| cycle_recovery: Option<Type<'db>>, | ||
|
|
||
| /// Whether refinements derived from the right-hand side of this definition were discarded. | ||
| discards_descendant_refinements: bool, |
Contributor
There was a problem hiding this comment.
My understanding is that this flag says: this inference builder refers to a definition which resulted in an invalid-assignment error.
If that's true, I would prefer a less cryptic name like is_definition_with_invalid_assignment or invalid_assignment_definition?
Comment on lines
+180
to
+182
| /// Such bindings describe refinement information derived from the right-hand side of their | ||
| /// enclosing assignment. If inference discards RHS-derived refinements for that assignment, its | ||
| /// synthesized descendants must not participate in place resolution either. |
Contributor
There was a problem hiding this comment.
This goes over my head. I feel like a Python code example (from the PR description or a test) with a brief explanation would be more helpful than this prose description.
| .finish_definition() | ||
| } | ||
|
|
||
| /// Returns `true` if a synthesized dictionary-key binding has been discarded. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Returns `true` if a synthesized dictionary-key binding has been discarded. | |
| /// Returns `true` if the definition refers to a dictionary-key binding that resulted in an invalid-assignment. |
Contributor
There was a problem hiding this comment.
similar here: are these changes related to the PR?
Comment on lines
+1407
to
+1408
| // The enclosing write already invalidated earlier child refinements. This | ||
| // rejected synthesized binding must not establish new RHS-derived precision. |
Contributor
There was a problem hiding this comment.
.. what? 😄
Doesn't this branch here mean that we skip a synthesized "x["a"] = 1" binding if the enclosing (real) x = {"a": 1} assignment (from which the synthesized was derived) was invalid?
Why does this refer to "invalidated earlier child refinements"? I understand that an assignment to x invalidates "earlier child refinements" like x["a"] = "foo", but how is that related to this branch?
Maybe
Suggested change
| // The enclosing write already invalidated earlier child refinements. This | |
| // rejected synthesized binding must not establish new RHS-derived precision. | |
| // This synthesized binding was derived from a dictionary assignment | |
| // `d = {...}` that was invalid. We discard it such that the lookup of the | |
| // place `d[key]` can fail. A fallback place for `d[key]` can then be looked | |
| // up on `d` directly. |
charliermarsh
force-pushed
the
charlie/reject-assignment
branch
from
e4ee3c2 to
d12b7e2
Compare
charliermarsh
force-pushed
the
charlie/reject-assignment
branch
from
d12b7e2 to
9984e4e
Compare
carljm
added a commit
that referenced
this pull request
Jun 1, 2026
* main: [`pydocstyle`] Improve discoverability of rules enabled for each convention (#24973) [ty] Deduplicate retained use-def place states (#25450) [ty] reduce features of low-level crates depended on by `ty_python_semantic` (#25524) [ty] Fix narrowing enum literal unions by member identity (#25520) [ty] Test tagged union narrowing for named tuples (#25519) [ty] Disallow file-system access in `ty_python_core` (#25518) [ty] Nominal Tagged Union Narrowing (#24916) Commit `scripts/uv.lock` (#25517) Fix potential index out of range in `LineIndex` computation (#25492) [ty] Sync vendored typeshed stubs (#25514) [ty] Add disjointness for protocol method members (#25315) [ty] Use compact sets for more immutable fields (#25476) [ty] Derive `Default` for `FunctionDecoratorInference` (#25482) [ty] Ignore rejected assignments for synthesized bindings (#25340) [ty] Handle cycles in function decorator inference (#25475) docs: fix typo `bin/active` → `bin/activate` in tutorial (#25473) [ty] Narrow bound method overloads by receiver (#24707)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prior to this change, we were retaining "known key" bindings from the right-hand side of an assignment even when the enclosing assignment was rejected. For example:
So we're respecting the new type information from
x = {"a": "bad"}... But an invalid assignment shouldn't contribute new type information!In other words, we want to ignore the known-key bindings that we get from the dict assignment when the overall assignment is invalid.
Doing so is pretty ugly because the index is built solely from syntax (e.g., we create definitions from
x = {"a": "bad"}), but we're trying to reject those definitions later during inference, using information we don't have at the time of definition creation. I tried a variety of approaches but this ended up being the cheapest and least invasive thing I could find.