Skip to content

Comments

[red-knot] Use Unknown | T_inferred for undeclared public symbols#15674

Merged
sharkdp merged 13 commits intomainfrom
david/union-of-unknown-and-inferred-type
Jan 24, 2025
Merged

[red-knot] Use Unknown | T_inferred for undeclared public symbols#15674
sharkdp merged 13 commits intomainfrom
david/union-of-unknown-and-inferred-type

Conversation

@sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Jan 22, 2025 •
edited
Loading

Summary

Use Unknown | T_inferred as the type for undeclared public symbols.

Test Plan

  • Updated existing tests
  • New test for external __slots__ modifications.
  • New tests for external modifications of public symbols.

@sharkdp sharkdp added the ty Multi-file analysis & type inference label Jan 22, 2025
@github-actions

This comment was marked as resolved.

Sign in to view
sharkdp
sharkdp commented Jan 22, 2025
View reviewed changes
crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md

a = NonCallable()
# error: "Object of type `NonCallable` is not callable"
# error: "Object of type `Unknown | Literal[1]` is not callable (due to union element `Literal[1]`)"
Copy link
Contributor Author

@sharkdp sharkdp Jan 22, 2025

Choose a reason for hiding this comment

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

Another instance of the callable problem. Notice how the usefulness of the error message degrades.

Copy link
Contributor

@carljm carljm Jan 22, 2025

Choose a reason for hiding this comment

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

Yes, this is really begging for nested diagnostics (so you get both "Object of type NonCallable is not callable" and nested details explaining why.)

We could easily adjust the handling so you still get "Object of type NonCallable is not callable" here, but then you lose the details of which union element failed callability.

Even without nested diagnostics we could still provide all of the information here in a new error, it just requires more and more bespoke error-cases handling in the __call__ lookup code. (We would need to match on the case that callability of __call__ failed due to a union, and then write a new custom error message for that case which mentions both the outer NonCallable type and the details of why its __call__ isn't callable.) Nested diagnostics would just let us handle this kind of situation more generically, with less special casing.

cc @BurntSushi @MichaReiser re diagnostics considerations

sharkdp
sharkdp commented Jan 22, 2025
View reviewed changes
carljm
carljm reviewed Jan 22, 2025
View reviewed changes
crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md

a = NonCallable()
# error: "Object of type `NonCallable` is not callable"
# error: "Object of type `Unknown | Literal[1]` is not callable (due to union element `Literal[1]`)"
Copy link
Contributor

@carljm carljm Jan 22, 2025

Choose a reason for hiding this comment

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

Yes, this is really begging for nested diagnostics (so you get both "Object of type NonCallable is not callable" and nested details explaining why.)

We could easily adjust the handling so you still get "Object of type NonCallable is not callable" here, but then you lose the details of which union element failed callability.

Even without nested diagnostics we could still provide all of the information here in a new error, it just requires more and more bespoke error-cases handling in the __call__ lookup code. (We would need to match on the case that callability of __call__ failed due to a union, and then write a new custom error message for that case which mentions both the outer NonCallable type and the details of why its __call__ isn't callable.) Nested diagnostics would just let us handle this kind of situation more generically, with less special casing.

cc @BurntSushi @MichaReiser re diagnostics considerations

AlexWaygood
AlexWaygood reviewed Jan 22, 2025
View reviewed changes
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.

Thank you for this! This makes it clear what the impact of this change would be. I agree that the most unintuitive thing is that we'd infer Unknown unions when local scopes reference types from enclosing scopes:

X = 1

def foo():
    reveal_type(X)  # Unknown | Literal[1]  :(

This is probably more correct, though? Because X might be mutated by other modules "monkey-patching" this module's globals.

@sharkdp sharkdp force-pushed the david/union-of-unknown-and-inferred-type branch from cd9230f to c19648a Compare January 23, 2025 12:52
@sharkdp sharkdp mentioned this pull request Jan 23, 2025
Merged
sharkdp added a commit that referenced this pull request Jan 23, 2025
@sharkdp
[red-knot] Make infer.rs unit tests independent of public symbol in…
fc2ebea 
…ference (#15690)

## Summary

Make the remaining `infer.rs` unit tests independent from public symbol
type inference decisions (see upcoming change in #15674).

## Test Plan

- Made sure that the unit tests actually fail if one of the
  `assert_type` assertions is changed.
@sharkdp sharkdp force-pushed the david/union-of-unknown-and-inferred-type branch from c19648a to ded8625 Compare January 23, 2025 13:32
@sharkdp sharkdp mentioned this pull request Jan 23, 2025
Merged
sharkdp added a commit that referenced this pull request Jan 23, 2025
@sharkdp
[red-knot] MDTests: Do not depend on precise public-symbol type infer…
15394a8 
…ence (#15691)

## Summary

Another small PR to focus #15674 solely on the relevant changes. This
makes our Markdown tests less dependent on precise types of public
symbols, without actually changing anything semantically in these tests.

Best reviewed using ignore-whitespace-mode.

## Test Plan

Tested these changes on `main` and on the branch from #15674.
@sharkdp sharkdp force-pushed the david/union-of-unknown-and-inferred-type branch from ded8625 to 026dbd3 Compare January 23, 2025 13:54
@sharkdp sharkdp force-pushed the david/union-of-unknown-and-inferred-type branch from 026dbd3 to 5bf3c9b Compare January 23, 2025 15:05
@sharkdp sharkdp marked this pull request as ready for review January 23, 2025 15:10
@sharkdp sharkdp requested a review from MichaReiser as a code owner January 23, 2025 15:10
sharkdp
sharkdp commented Jan 23, 2025
View reviewed changes
carljm
carljm approved these changes Jan 23, 2025
View reviewed 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.

Thank you!

I would like to understand where Unknown is coming from in the cases I commented inline; otherwise this looks good to go.

@sharkdp sharkdp merged commit 1feb3cf into main Jan 24, 2025
21 checks passed
@sharkdp sharkdp deleted the david/union-of-unknown-and-inferred-type branch January 24, 2025 11:47
dcreager added a commit that referenced this pull request Jan 24, 2025
@dcreager
Merge branch 'main' into dcreager/terminal-visibility
b9f91fb 
* main:
  Add `check` command (#15692)
  [red-knot] Use itertools to clean up `SymbolState::merge` (#15702)
  [red-knot] Add `--ignore`, `--warn`, and `--error` CLI arguments (#15689)
  Use `uv init --lib` in tutorial (#15718)
  [red-knot] Use `Unknown | T_inferred` for undeclared public symbols (#15674)
  [`ruff`] Parenthesize fix when argument spans multiple lines for `unnecessary-round` (`RUF057`) (#15703)
  [red-knot] Rename `TestDbBuilder::typeshed` to `.custom_typeshed` (#15712)
  Honor banned top level imports by TID253 in PLC0415.  (#15628)
  Apply `AIR302`-context check only in `@task` function (#15711)
  [`airflow`] Update `AIR302` to check for deprecated context keys (#15144)
  Remove test rules from JSON schema (#15627)
  Add two missing commits to changelog (#15701)
  Fix grep for version number in docker build (#15699)
  Bump version to 0.9.3 (#15698)
  Preserve raw string prefix and escapes (#15694)
  [`flake8-pytest-style`] Rewrite references to `.exception` (`PT027`) (#15680)
@sharkdp sharkdp mentioned this pull request Jan 27, 2025
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@AlexWaygood AlexWaygood AlexWaygood left review comments

@MichaReiser MichaReiser Awaiting requested review from MichaReiser

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sharkdp @carljm @MichaReiser @AlexWaygood