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] implement basic call expression inference #13164

Merged
merged 6 commits into from
Aug 30, 2024
Merged

[red-knot] implement basic call expression inference #13164

merged 6 commits into from
Aug 30, 2024

Conversation

chriskrycho
Copy link
Contributor

Summary

Adds basic support for inferring the type resulting from a call expression. This only works for the result of call expressions; it performs no inference on parameters. It also intentionally does nothing with class instantiation, __call__ implementors, or lambdas.

Test Plan

Adds a test that it infers the right thing!

This only works for the *result* of call expressions; it performs no
inference on parameters.
Copy link
Contributor

github-actions bot commented Aug 30, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good! Some things we should adjust in the implementation.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
Comment on lines +2840 to +2857
#[test]
fn basic_call_expression() -> anyhow::Result<()> {
let mut db = setup_db();

db.write_dedented(
"src/a.py",
"
def get_int() -> int:
return 42

x = get_int()
",
)?;

assert_public_ty(&db, "src/a.py", "x", "int");

Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fantastic, this is exciting to see happen! Unlocks so much new capability.

- Move inference on the call expression type to `Type::call`, matching
  what we do for `Type::member`.
- Add a diagnostic for the case that the type is in fact not callable.
  Test it, noting that there is a separate issue around creating the
  same diagnostic more than once because it is emitted as a side effect
  of inference.
- Leave correct breadcrumbs for future implementation in `Type::call`.
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!!

crates/red_knot_python_semantic/src/types.rs Show resolved Hide resolved
@carljm
Copy link
Contributor

carljm commented Aug 30, 2024

Looks like we need to adjust the expected diagnostics in the benchmark.

Copy link

codspeed-hq bot commented Aug 30, 2024

CodSpeed Performance Report

Merging #13164 will degrade performances by 14.73%

Comparing chriskrycho:rk-call-expression-inference (4c52a81) with main (a73bebc)

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

Benchmark main chriskrycho:rk-call-expression-inference Change
👁 red_knot_check_file[incremental] 2.4 ms 2.8 ms -14.73%

@carljm
Copy link
Contributor

carljm commented Aug 30, 2024

Looks like this CodSpeed regression is real, but it's expected; we are now looking up the return type of every function called (and apparently doing it twice.) Fixing the double inference part should reduce the size of the regression somewhat.

Now we do not call (or emit diagnostics!) twice for call expressions on
the RHS of an assignment. The issue here was not the call expression but
the assignment, and had existed for some time, but this makes for a much
cleaner landing!

Co-authored-by: Carl Meyer <carl@astral.sh>
@carljm
Copy link
Contributor

carljm commented Aug 30, 2024

On closer look, I think a lot of the regression (and why it shows up only in incremental benchmark) is that annotations are deferred in stubs, and now we look up a lot of return types from stubs, which causes us to trigger the infer_deferred_types query on a lot more function definitions, creating more memoized query results that Salsa has to re-validate in an incremental re-check. I still think this is expected and there's not a lot we can do about it, short of finding ways to make Salsa more efficient on new revisions when there are a lot of ingredients to re-validate.

@carljm carljm merged commit 28ab5f4 into astral-sh:main Aug 30, 2024
20 checks passed
@chriskrycho chriskrycho deleted the rk-call-expression-inference branch August 30, 2024 21:15
@dhruvmanila dhruvmanila added the red-knot Multi-file analysis & type inference label Sep 5, 2024
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.

4 participants