Skip to content

TypeForm: Enable by default#21262

Merged
JukkaL merged 3 commits intopython:masterfrom
davidfstr:f/typeform_complete
May 1, 2026
Merged

TypeForm: Enable by default#21262
JukkaL merged 3 commits intopython:masterfrom
davidfstr:f/typeform_complete

Conversation

@davidfstr
Copy link
Copy Markdown
Contributor

...removing the need to use --enable-incomplete-feature=TypeForm


REVIEWER NOTES:

...removing the need to use --enable-incomplete-feature=TypeForm
@github-actions

This comment has been minimized.

Comment thread test-data/unit/check-typeform.test Outdated

[case testRecognizesParameterizedTypeFormInAnnotation]
# flags: --python-version 3.14 --enable-incomplete-feature=TypeForm
# flags: --python-version 3.15
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm do we already support 3.15 enough for this to make sense? The test uses typing_extensions anyway so I'm not sure why it needs to pin a version at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test uses typing_extensions anyway so I'm not sure why it needs to pin a version at all.

If it's OK for the test to (A) continue to use typing_extensions, then I agree that the --python-version 3.15 doesn't make a lot of sense.

OTOH, if it would be more conventional in tests to (B) use the typing version, then I think --python-version 3.15 becomes necessary.

I'll take a look in the next few days to see what other tests are doing to choose whether to do (A) or (B).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't run 3.15 in CI, and may potentially not run it for another few months, so we should use 3.14 for now. It will be easy to mass update these tests to typing in the future (as we did with other type system features).

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Copy Markdown
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks! It is probably better to put this in v2.1 to be safe. I will leave this up to @JukkaL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In #20946, I kept the --python-version 3.14 flags, maybe it makes sense to keep them?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, this PR is fine.

@JukkaL
Copy link
Copy Markdown
Collaborator

JukkaL commented May 1, 2026

We'll likely have mypy 2.1 out pretty soon afterwards, so we can wait for 2.1.

@JukkaL JukkaL merged commit dd851f5 into python:master May 1, 2026
25 checks passed
@ilevkivskyi
Copy link
Copy Markdown
Member

Hm, our daily benchmark shows this PR caused 2% slow-down on self-check. Taking into account mypy itself doesn't use TypeForm at all, this is not acceptable.

I guess the slow-down is because we call try_parse_as_type_expression() too much, which btw makes two re.match() calls that may be quite expensive (because they will call into interpreted code).

I think we should only interpret an expression as type whenever a TypeForm is expected. I don't think we need to revert this yet as it is not included in 2.0, but the performance should be addressed before 2.1 (i.e. soon).

@davidfstr
Copy link
Copy Markdown
Contributor Author

I’ll take a look at the performance on selfcheck in the next few days.

The TypeForm-related code already has “cache-hit” / “cache-miss” style performance counters plus a script to interpret them, which should be useful for investigation.

@hauntsaninja hauntsaninja mentioned this pull request May 9, 2026
6 tasks
@ilevkivskyi ilevkivskyi mentioned this pull request May 9, 2026
2 tasks
@ilevkivskyi
Copy link
Copy Markdown
Member

Btw, I briefly looked at the current TypeForm logic. Initially I thought the try_parse_as_type_expression() calls during semantic analysis are some kind of performance optimization (that went wrong), but it looks like they are required because of the TypeVarLikeScope issue in type checker. @davidfstr is this right?

I don't know how hard it is, but I think the optimal way would be to fix the TypeVarLikeScope thing, and always parse expressions as types lazily. If this is too hard, some ideas to avoid too much extra work in semantic analysis:

  • re calls are actually slow (because they are not optimized by mypyc). So maybe before calling re filter some common cases, like skip empty strings, and strings of length 1 if it is not isalpha().
  • Skip parsing when assigning to an inferred variable
  • Skip parsing when assigning to a variable with a type that is known to not have TypeForm
  • Skip parsing return expression when return type annotation is missing of doesn't have TypeForm
  • Skip parsing arguments when calling a known function without TypeForm in parameter types

For the last items you don't need to do an extra visit, simply record the presence of TypeForm in type analyzer. But if something like type TypeList[T] = list[TypeForm[T]] is a valid thing per the PEP, then you will need to be careful when recording the presence of type forms (you may need to add an attribute to TypeAlias node).

@JelleZijlstra
Copy link
Copy Markdown
Member

Don't know how much it will help but I noticed an easy opportunity to get rid of one of the two regexes: #21459. Also looking into the second one a bit to see if we can get a better heuristic.

@JelleZijlstra
Copy link
Copy Markdown
Member

Rearranged the code a bit more based on some observations about strings appearing in mypy itself.

If the regression is really driven by the string matching in try_parse_as_type_expression (haven't profiled to verify), then this is going to be pretty application-dependent. Mypy might be a slightly pathological case because there are a lot of strings like "builtins.list" that look to the regex like they could be valid type expressions.

@ilevkivskyi
Copy link
Copy Markdown
Member

If the regression is really driven by the string matching

To be clear, I don't think it is the main factor, but likely a significant factor. I think it may be necessary to do most of the things I mentioned above to make the regression go away ~completely.

@davidfstr
Copy link
Copy Markdown
Contributor Author

Initially I thought the try_parse_as_type_expression() calls during semantic analysis are some kind of performance optimization (that went wrong), but it looks like they are required because of the TypeVarLikeScope issue in type checker. @davidfstr is this right?

IIRC the only reason there’s logic in the semantic analyzer (and not just only in the TypeChecker) is because string literals are sometimes type annotations and TypeChecker doesn’t have enough info to resolve all references that could occur in a string context. In particular locally defined classes are thrown out by the time TypeChecker gets to look at an expression.

I’m still tracking the task of optimizing performance related to this thread. Last week I was finishing up items for work as I was transitioning out into parental leave. Hopefully I should have some more bandwidth over the next few days.

@ilevkivskyi
Copy link
Copy Markdown
Member

Mypy might be a slightly pathological case because there are a lot of strings like "builtins.list" that look to the regex like they could be valid type expressions.

Btw if someone can benchmark this PR on something unrelated, like homeassistant or torch (the repo itself, not as 3rd party import), then we may get a better idea of the impact. If the impact on unrelated repos is <= 1% then I think we don't need to revert this for v2.1 (we will still need to profile/optimize this however).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants