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] type narrowing #12706

Merged
merged 4 commits into from
Aug 16, 2024
Merged

[red-knot] type narrowing #12706

merged 4 commits into from
Aug 16, 2024

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Aug 6, 2024

Extend the UseDefMap to also track which constraints (provided by e.g. if tests) apply to each visible definition.

Uses a custom BitSet and BitSetArray to track which constraints apply to which definitions, while keeping data inline as much as possible.

@carljm carljm added the red-knot Multi-file analysis & type inference label Aug 6, 2024
@carljm carljm changed the title [red-knot] [WIP] initial narrowing work [red-knot] [WIP] type narrowing Aug 8, 2024
Copy link

codspeed-hq bot commented Aug 8, 2024

CodSpeed Performance Report

Merging #12706 will degrade performances by 13.78%

Comparing cjm/narrow (15885c6) with main (a9847af)

Summary

❌ 2 (👁 2) regressions
✅ 30 untouched benchmarks

Benchmarks breakdown

Benchmark main cjm/narrow Change
👁 red_knot_check_file[cold] 51 ms 59.2 ms -13.78%
👁 red_knot_check_file[incremental] 2.7 ms 3 ms -9.39%

@carljm carljm changed the title [red-knot] [WIP] type narrowing [red-knot] type narrowing Aug 15, 2024
Copy link
Contributor

github-actions bot commented Aug 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@carljm carljm marked this pull request as ready for review August 15, 2024 03:25
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This is great work and the documentation is excellent.

We may be able to remove quiet a bit of code from BitSet by using smallvec (if we want?) and we probably clone the narrowing constraints on every query run which we should avoid ;)

crates/red_knot_python_semantic/src/semantic_index.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/semantic_index.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved
@carljm
Copy link
Contributor Author

carljm commented Aug 16, 2024

@MichaReiser Thanks for the excellent review! I think I've addressed all the comments. Lots of changes, so you may want to take another look.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I only skimmed through. Thanks for addressing all the feedback and we're down to a 13% perf regression :)

}

/// Merge two [`SymbolState`] and return the result.
pub(super) fn merge(a: &SymbolState, b: &SymbolState) -> SymbolState {
Copy link
Member

Choose a reason for hiding this comment

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

I do think we could make use of it but it would probably complicate the logic a bit because you need two versions of push and a into_iter version for constraints

For example, couldn't we avoid cloning the constraints and instead take them right from the symbol state?

@carljm
Copy link
Contributor Author

carljm commented Aug 16, 2024

Hmm, I made a reply comment about the "taking ownership in merge" thing but now I don't see it in the UI anywhere. Anyway the upshot was that you're right, I was able to do that; it didn't help the benchmarks but I left it in anyway. Going to go ahead and merge this, but if you see anything in those latest changes that looks dumb, don't hesitate to comment and I'll fix!

@carljm carljm merged commit 6359e55 into main Aug 16, 2024
20 checks passed
@carljm carljm deleted the cjm/narrow branch August 16, 2024 23:34
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