Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] support deferred evaluation of type expressions #13131

Merged
merged 10 commits into from
Aug 28, 2024
Merged

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Aug 28, 2024

Prototype deferred evaluation of type expressions by deferring evaluation of class bases in a stub file. This allows self-referential class definitions, as occur with the definition of str in typeshed (which inherits Sequence[str]).

@carljm carljm added the red-knot Multi-file analysis & type inference label Aug 28, 2024
Copy link

codspeed-hq bot commented Aug 28, 2024

CodSpeed Performance Report

Merging #13131 will not alter performance

Comparing lazy-bases (b9c1dd3) with main (c6023c0)

Summary

✅ 32 untouched benchmarks

Copy link
Contributor

github-actions bot commented Aug 28, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@carljm carljm marked this pull request as ready for review August 28, 2024 01:33
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Overall this looks good!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/files.rs Outdated Show resolved Hide resolved
@carljm
Copy link
Contributor Author

carljm commented Aug 28, 2024

I looked into the regression here, and it seems like it's increased Salsa overhead. This is consistent with the regression only showing up in the incremental benchmark, not the cold benchmark. I guess this is because we now double the number of tracked function memos for definitions, which are our highest-cardinality tracked struct.

@carljm
Copy link
Contributor Author

carljm commented Aug 28, 2024

I added tracking of whether a given definition has any deferred expressions, which allows us to avoid ever calling infer_deferred_types query on a definition that doesn't have any. Locally this seems to make the regression go away, will see if CodSpeed agrees.

@carljm
Copy link
Contributor Author

carljm commented Aug 28, 2024

Yay, CodSpeed agrees!

@carljm carljm merged commit 770ef2a into main Aug 28, 2024
20 checks passed
@carljm carljm deleted the lazy-bases branch August 28, 2024 18:41
carljm added a commit that referenced this pull request Sep 3, 2024
Test coverage for #13131 wasn't as good as I thought it was, because
although we infer a lot of types in stubs in typeshed, we don't check
typeshed, and therefore we don't do scope-level inference and pull all
types for a scope. So we didn't really have good test coverage for
scope-level inference in a stub. And because of this, I got the code for
supporting that wrong, meaning that if we did scope-level inference with
deferred types, we'd end up never populating the deferred types in the
scope's `TypeInference`, which causes panics like #13160.

Here I both add test coverage by running the corpus tests both as `.py`
and as `.pyi` (which reveals the panic), and I fix the code to support
deferred types in scope inference.

This also revealed a problem with deferred types in generic functions,
which effectively span two scopes. That problem will require a bit more
thought, and I don't want to block this PR on it, so for now I just
don't defer annotations on generic functions.

Fixes #13160.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants