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] infer_symbol_public_type infers union of all definitions #11669

Merged
merged 6 commits into from
Jun 3, 2024

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Jun 1, 2024

Summary

Rename infer_symbol_type to infer_symbol_public_type, and allow it to work on symbols with more than one definition. For now, use the most cautious/sound inference, which is the union of all definitions. We can prune this union more in future by eliminating definitions if we can show that they can't be visible (this requires both that the symbol is definitely later reassigned, and that there is no intervening call/import that might be able to see the over-written definition).

Test Plan

Added a test showing inference of union from multiple definitions.

@carljm carljm added the red-knot Multi-file analysis & type inference label Jun 1, 2024
@carljm carljm requested a review from AlexWaygood June 1, 2024 01:56
@carljm carljm requested a review from MichaReiser as a code owner June 1, 2024 01:56
Copy link
Contributor

github-actions bot commented Jun 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/red_knot/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot/src/types/infer.rs Outdated Show resolved Hide resolved
@@ -16,21 +16,30 @@ use crate::{FileId, Name};

// FIXME: Figure out proper dead-lock free synchronisation now that this takes `&db` instead of `&mut db`.
#[tracing::instrument(level = "trace", skip(db))]
pub fn infer_symbol_type(db: &dyn SemanticDb, symbol: GlobalSymbolId) -> QueryResult<Type> {
pub fn infer_symbol_public_type(db: &dyn SemanticDb, symbol: GlobalSymbolId) -> QueryResult<Type> {
Copy link
Member

Choose a reason for hiding this comment

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

The distinction between public and non public types is unclear to me, also how we guarantee that you can't call this function for a local symbol. But that's something we can tackle independently.

It may help to add some documentation to the query to explain the distinction and for what this query should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a docstring to the method that replaces the comment I'd written internally, and tries to explain what the "public type" is. Let me know if it's not clear.

The "public type" is relevant for any symbol that can be seen from outside its own scope: this includes all public symbols, and it also includes all "cellvars" (function-internal symbols that are used by a nested function.)

We may need to refine our terminology here, because this means that even symbols inside a function (which I wouldn't call "public symbols" in terms of cross-module dependency tracking) still have a "public type" which is relevant to type checking.

I don't think we need to "guarantee that you can't call this function" for any symbol. This function shouldn't ever be needed in type inference for purely-local symbols (where every use of the symbol is at a specific known point in control flow and uses only definitions reachable from there), but that's a matter for validating correctness of type inference; this function can return a reasonable result for any symbol (it's just the union of all definitions.)

carljm added 2 commits June 3, 2024 15:58
* main: (25 commits)
  Isolate non-breaking whitespace indentation test case (#11721)
  Generator should add a newline before type statement (#11720)
  Remove less used parser dependencies (#11718)
  Use string expression for parsing type annotation (#11717)
  Re-order lexer methods (#11716)
  Maintain synchronicity between the lexer and the parser (#11457)
  Update NPM Development dependencies (#11713)
  Update pre-commit dependencies (#11712)
  Update cloudflare/wrangler-action action to v3.6.1 (#11709)
  Update dependency monaco-editor to ^0.49.0 (#11710)
  Update Rust crate tracing-tree to v0.3.1 (#11703)
  Update Rust crate libcst to v1.4.0 (#11707)
  Update Rust crate itertools to 0.13.0 (#11706)
  Update Rust crate insta to v1.39.0 (#11705)
  Update Rust crate proc-macro2 to v1.0.85 (#11700)
  Update Rust crate toml to v0.8.13 (#11702)
  Update Rust crate strum_macros to v0.26.3 (#11701)
  Update UP035 for Python 3.13 and the latest version of typing_extensions (#11693)
  Add RDJson support. (#11682)
  [`pyupgrade`] Write empty string in lieu of panic (#11696)
  ...
@carljm carljm merged commit b02d3f3 into main Jun 3, 2024
19 checks passed
@carljm carljm deleted the cjm/cfg1 branch June 3, 2024 23:27
carljm added a commit that referenced this pull request Jun 3, 2024
* main:
  [red-knot] infer_symbol_public_type infers union of all definitions (#11669)
carljm added a commit that referenced this pull request Jun 3, 2024
* cjm/cfg2:
  [red-knot] infer_symbol_public_type infers union of all definitions (#11669)
carljm added a commit that referenced this pull request Jun 3, 2024
* cjm/cfg3: (29 commits)
  [red-knot] infer_symbol_public_type infers union of all definitions (#11669)
  review comments
  review comments
  review comments
  Isolate non-breaking whitespace indentation test case (#11721)
  Generator should add a newline before type statement (#11720)
  Remove less used parser dependencies (#11718)
  Use string expression for parsing type annotation (#11717)
  Re-order lexer methods (#11716)
  Maintain synchronicity between the lexer and the parser (#11457)
  Update NPM Development dependencies (#11713)
  Update pre-commit dependencies (#11712)
  Update cloudflare/wrangler-action action to v3.6.1 (#11709)
  Update dependency monaco-editor to ^0.49.0 (#11710)
  Update Rust crate tracing-tree to v0.3.1 (#11703)
  Update Rust crate libcst to v1.4.0 (#11707)
  Update Rust crate itertools to 0.13.0 (#11706)
  Update Rust crate insta to v1.39.0 (#11705)
  Update Rust crate proc-macro2 to v1.0.85 (#11700)
  Update Rust crate toml to v0.8.13 (#11702)
  ...
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.

Nice! Sorry for the slow review

crates/red_knot/src/types/infer.rs Show resolved Hide resolved
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.

3 participants