Skip to content

Conversation

@carljm
Copy link
Contributor

@carljm carljm commented Oct 7, 2025

Summary

Typevar attributes (bound/constraints/default) can be either lazily evaluated or eagerly evaluated. Currently they are lazily evaluated for PEP 695 typevars, and eager for legacy and synthetic typevars. #20598 will make them lazy also for legacy typevars, and the ecosystem report on that PR surfaced the issue fixed here (because legacy typevars are much more common in the ecosystem than PEP 695 typevars.)

Applying a transform to a typevar (normalization, materialization, or mark-inferable) will reify all lazy attributes and create a new typevar with eager attributes. In terms of Salsa identity, this transformed typevar will be considered different from the original typevar, whether or not the attributes were actually transformed.

In general, this is not a problem, since all typevars in a given generic context will be transformed, or not, together.

The exception to this was implicit-self vs explicit Self annotations. The typevar we created for implicit self was created initially using inferable typevars, whereas an explicit Self annotation is initially non-inferable, then transformed via mark-inferable when accessed as part of a function signature. If the containing class (which becomes the upper bound of Self) is generic, and has e.g. a lazily-evaluated default, then the explicit-Self annotation will reify that default in the upper bound, and the implicit-self would not, leading them to be treated as different typevars, and causing us to fail to solve a call to a method such as def method(self) -> Self correctly.

The fix here is to treat implicit-self more like explicit-Self, initially creating it as non-inferable and then using the mark-inferable transform on it. This is less efficient, but restores the invariant that all typevars in a given generic context are transformed together, or not, fixing the bug.

In the improved-constraint-solver work, the separation of typevars into "inferable" and "non-inferable" is expected to disappear, along with the mark-inferable transform, which would render both this bug and the fix moot. So this fix is really just temporary until that lands.

There is a performance regression, but not a huge one: 1-2% on most projects, 5% on one outlier. This seems acceptable, given that it should be fully recovered by removing the mark-inferable transform.

Test Plan

Added mdtests that failed before this change.

@carljm carljm force-pushed the cjm/self-lazy-default branch from 9617d66 to a2396cd Compare October 7, 2025 20:48
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@github-actions

This comment was marked as outdated.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 7, 2025

CodSpeed Performance Report

Merging #20754 will degrade performances by 4.96%

Comparing cjm/self-lazy-default (a70eb5e) with main (ff386b4)

Summary

❌ 1 (👁 1) regression
✅ 20 untouched
⏩ 30 skipped1

Benchmarks breakdown

Mode Benchmark BASE HEAD Change
👁 WallTime small[freqtrade] 4.9 s 5.1 s -4.96%

Footnotes

  1. 30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@carljm carljm force-pushed the cjm/self-lazy-default branch from a2396cd to 78a377c Compare October 7, 2025 23:51
@carljm carljm marked this pull request as ready for review October 8, 2025 00:00
@carljm carljm force-pushed the cjm/self-lazy-default branch from 78a377c to ae1e527 Compare October 8, 2025 00:18
@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Oct 8, 2025
Copy link
Member

@dcreager dcreager left a comment

Choose a reason for hiding this comment

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

Agreed that the performance regression is acceptable seeing as how it's temporary. #20677 should land soon; I'm just tracking down some missing pieces in overload resolution.

@carljm carljm enabled auto-merge (squash) October 8, 2025 01:35
@carljm carljm merged commit 5d3a35e into main Oct 8, 2025
38 checks passed
@carljm carljm deleted the cjm/self-lazy-default branch October 8, 2025 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ecosystem-analyzer ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants