Skip to content

[ty] fix bug in string annotations and clean up diagnostics#22913

Merged
Gankra merged 2 commits intomainfrom
gankra/fix-str2
Jan 28, 2026
Merged

[ty] fix bug in string annotations and clean up diagnostics#22913
Gankra merged 2 commits intomainfrom
gankra/fix-str2

Conversation

@Gankra
Copy link
Contributor

@Gankra Gankra commented Jan 28, 2026

This multiplication was always using only using the larger sub-ast size value, which didn't do anything unsound but made sub-string annotations become exhausted on files 32x smaller than expected (16k nodes instead of 512k nodes).

Also I decided to do a cleanup pass on the diagnostics to make them more precise and helpful.

@Gankra Gankra added parser Related to the parser ty Multi-file analysis & type inference labels Jan 28, 2026
@Gankra
Copy link
Contributor Author

Gankra commented Jan 28, 2026

thanks to @MeGaGiGaGon for catching this!

Comment on lines 65 to 67
NodeIndexError::TooNested => "too many levels of nested string annotations, remove the redundant nested quotes".to_owned(),
NodeIndexError::OverflowedIndices => {
"file too long for string annotations, either break up the file or don't use string annotations".to_owned()
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
NodeIndexError::TooNested => "too many levels of nested string annotations, remove the redundant nested quotes".to_owned(),
NodeIndexError::OverflowedIndices => {
"file too long for string annotations, either break up the file or don't use string annotations".to_owned()
NodeIndexError::TooNested => "too many levels of nested string annotations; remove the redundant nested quotes".to_owned(),
NodeIndexError::OverflowedIndices => {
"file too long for string annotations: either break up the file or don't use string annotations".to_owned()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh, what's the logic for colon vs semicolon, to you?

Copy link
Member

Choose a reason for hiding this comment

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

I would usually use a semicolon for two statements that are independent and sit alongside each other, but a colon for two statements where the second directly follows from the first.

In this specific context... I can't really put a finger on why a colon feels better for me in the second case while a semicolon feels better in the first case 😆 I think either is perfectly grammatical. The second one feels slightly more imperative to me (not sure why), which I think is why I'd go for a colon there? But as I say, I think both are fine here

Comment on lines 70 to 76
"file too long for nested string annotations, remove the redundant nested quotes".to_owned()
}
NodeIndexError::ExhaustedSubIndices => {
"string annotation is too long, consider introducing type aliases to simplify".to_owned()
}
NodeIndexError::ExhaustedSubSubIndices => {
"nested string annotation is too long, remove the redundant nested quotes".to_owned()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"file too long for nested string annotations, remove the redundant nested quotes".to_owned()
}
NodeIndexError::ExhaustedSubIndices => {
"string annotation is too long, consider introducing type aliases to simplify".to_owned()
}
NodeIndexError::ExhaustedSubSubIndices => {
"nested string annotation is too long, remove the redundant nested quotes".to_owned()
"file too long for nested string annotations; remove the redundant nested quotes".to_owned()
}
NodeIndexError::ExhaustedSubIndices => {
"string annotation is too long: consider introducing type aliases to simplify".to_owned()
}
NodeIndexError::ExhaustedSubSubIndices => {
"nested string annotation is too long; remove the redundant nested quotes".to_owned()

@Gankra Gankra merged commit 2c902c7 into main Jan 28, 2026
44 of 45 checks passed
@Gankra Gankra deleted the gankra/fix-str2 branch January 28, 2026 15:55
carljm added a commit that referenced this pull request Jan 30, 2026
* main: (76 commits)
  [ty] Improve the check for `NewType`s with generic bases (#22961)
  [ty] Ban legacy `TypeVar` bounds or constraints from containing type variables (#22949)
  Bump the typing conformance suite pin (#22960)
  [ty] Emit an error if a TypeVarTuple is used to subscript `Generic` or `Protocol` without being unpacked (#22952)
  [ty] Reduce false positives when subscripting classes generic over `TypeVarTuple`s (#22950)
  [ty] Detect invalid attempts to subclass `Protocol[]` and `Generic[]` simultaneously (#22948)
  Fix suppression indentation matching (#22903)
  Remove hidden `--output-format` warning (#22944)
  [ty] Validate signatures of dataclass `__post_init__` methods (#22730)
  [ty] extend special-cased `numbers` diagnostic to `invalid-argument-type` errors (#22938)
  [ty] Avoid false positive for `not-iterable` with no-positive intersection types (#22089)
  [ty] Preserve pure negation types in descriptor protocol (#22907)
  [ty] add special-case diagnostic for `numbers` module (#22931)
  [ty] Move the location of more `invalid-overload` diagnostics (#22933)
  [ty] Fix unary and comparison operators for TypeVars with union bounds (#22925)
  [ty] Rule Selection: ignore/warn/select all rules (unless subsequently overriden) (#22832)
  [ty] Fix TypedDict construction from existing TypedDict values (#22904)
  [ty] fix bug in string annotations and clean up diagnostics (#22913)
  [ty] Improve support for goto-type, goto-declaration, hover, and highlighting of string annotations (#22878)
  [ty] Rename old typing imports to new on `unresolved-reference`. (#22827)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parser Related to the parser ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments