[red-knot] don't include Unknown in the type for a conditionally-defined import #13563

pilleye · 2024-09-30T03:02:57Z

Summary

Fixes the bug described in #13514 where an unbound public type defaulted to the type or Unknown, whereas it should only be the type if unbound.

Test Plan

Added a new test case

crates/red_knot_python_semantic/src/types.rs

github-actions · 2024-09-30T03:19:55Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

carljm

Awesome, thank you!! Just a few comment removals and this looks good to go.

carljm · 2024-09-30T20:39:18Z

crates/red_knot_python_semantic/src/types/infer.rs

        // TODO: once we support `sys.version_info` branches,
        // we can set `--target-version=py311` in this test
        // and the inferred type will just be `BaseExceptionGroup` --Alex


We can now remove this comment, and the similar one in two below tests, because the lack of sys.version_info handling no longer results in a spurious Unknown here.

carljm · 2024-09-30T21:35:01Z

Oh, I didn't notice tests are failing. We'll need to update the tests, and I'll need to look at the resulting assertion changes to make sure we aren't getting any undesired side effects from this.

pilleye · 2024-09-30T22:09:16Z

Ah, I missed those tests on the latest version, will fix tonight.

pilleye · 2024-10-01T01:16:37Z

There seems to be a weird side-effect where we're narrowing Unbound | Unknown types to just Unknown instead of Unbound: https://github.com/pilleye/ruff/blob/pilleye/public-known/crates/red_knot_python_semantic/src/types/infer.rs#L5313

Misread the case

carljm · 2024-10-01T01:25:32Z

So one thing I'm realizing here is that we are using assert_public_ty in some tests where we really intend to be testing "what is the local type of this variable at the end of the scope" -- this was natural because those used to be the same thing, but in this PR we are definitively making them different.

So for tests like basic_for_loop, for example, what we should really do instead of using assert_public_ty is to add a reveal_type(x) as the last line in the test code, and then assert_file_diagnostics that we get the expected type revealed there, which will include Unbound. This will be a bit more verbose for now, but it'll get better in the upcoming test framework.

pilleye · 2024-10-01T01:50:22Z

Working through a global-related bugs right now.

We seem to be incorrectly shadowing global builtins with Never here: https://github.com/pilleye/ruff/blob/pilleye/public-known/crates/red_knot_python_semantic/src/types/infer.rs#L4431

This is currently narrowed to Literal[1].

carljm · 2024-10-01T02:48:16Z

Ah ok, so that's going to be due to the use of global_symbol_ty in TypeInferenceBuilder::lookup_name. We need to use a function there that does union with unbound (if unbound is possible), so that we can replace unbound with the builtin definition. So the set of functions in types.rs: global_symbol_ty, symbol_ty, symbol_ty_by_name, symbol_ty_by_id are going to have to be split between the import case and the global-lookup-within-the-module case.

I would probably keep those functions as is, and add a new global_symbol_lookup function that a) still inserts Type::Unbound in case unbound is possible, and b) should also use only definitions, not declared types, even if there are declarations (so it's like just the second case in symbol_ty_by_id). (We may want a new test to verify (b) as well.) We may not even need global_symbol_lookup to be a function; I think the only callsite will be in TypeInferenceBuilder::lookup_name, so it can probably just be inlined there, since it shouldn't be more than a couple lines.

Let me know if any of that doesn't make sense! Thanks for working through this. I knew the first version of this PR was too simple to be true 😆

pilleye · 2024-10-01T05:39:07Z

Haha the first version seemed suspiciously easy

I think I got the tests working, but definitely feels way jankier than I would want. Would appreciate comments!

I think trying to genericize symbol_ty_by_id adds unnecessary complexity, so I might just inline that code as you said.

carljm

Nice work! You did what I asked for yesterday, but I now realize that in some cases I asked for the wrong thing 😆 Sorry about that! I've talked this over with the team (by which I mean @AlexWaygood in this case) and I think we've got a clearer plan in mind now; let me know if any of the comments are not clear.

Thanks for all your work on this! You definitely didn't pick an easy first task :)

crates/red_knot_python_semantic/src/types/infer.rs

carljm · 2024-10-01T18:32:10Z

crates/red_knot_python_semantic/src/types/infer.rs

+    }
+
+    #[test]
+    fn resolve_unannotated_unbound_public() -> anyhow::Result<()> {


nit: it's only maybe unbound

Suggested change

fn resolve_unannotated_unbound_public() -> anyhow::Result<()> {

fn resolve_unannotated_maybe_unbound_public() -> anyhow::Result<()> {

carljm · 2024-10-01T18:32:32Z

crates/red_knot_python_semantic/src/types/infer.rs

@@ -4158,6 +4159,40 @@ mod tests {
        Ok(())
    }

+    #[test]
+    fn resolve_unbound_public() -> anyhow::Result<()> {


Suggested change

fn resolve_unbound_public() -> anyhow::Result<()> {

fn resolve_maybe_undeclared_maybe_unbound_public() -> anyhow::Result<()> {

carljm · 2024-10-01T18:34:04Z

crates/red_knot_python_semantic/src/types/infer.rs

+        assert_public_ty(&db, "src/a.py", "r", "Literal[2]");
+        assert_public_ty(&db, "src/a.py", "s", "Literal[5]");


This is one of those cases where part of the whole purpose of the test is actually to show that we know which names might be unbound, so we should really update the test such that it doesn't use (only)assert_public_ty, because it's really not the public type we want to assert on.

I earlier suggested an approach using reveal_type, and I think that's actually a fine option in the tests below where you use it.

I realized a simpler approach in some of these tests (particularly the global-scope ones) might be to keep these assert_public_ty but add another helper assert_may_be_unbound (which uses the use-def map directly) and explicitly assert here that r and s may be unbound. This way we actually get both the assertion on the public type, and an assertion that locally we do know the name might be unbound.

~~Is there a way to add public type tests to the markdown tests introduced in #13719?~~

Never mind, figured it out with importing and revealing the type.

You can import it (since tests can be multi-file), or refer to the name as a global from within a nested function. Import probably makes it clearer what's going on?

How do I run the mdtests? cargo test inside red_knot_python_semantic and red_knot_test both don't seem to run it.

cargo test inside red_knot_python_semantic (or cargo test -p red_knot_python_semantic from the workspace) should run the mdtests, and do for me. How are you concluding that they aren't running?

Hmm, they only run when I run cargo test mdtest inside red_knot_python_semantic.

Without mdtest: https://gist.github.com/pilleye/7454e668bc19f9ca1969e0765a82a301
With mdtest: https://gist.github.com/pilleye/3e9842e37a911eb88f6ac26c281bb0dc

That's weird -- I'm not enough of a cargo test expert to explain this discrepancy :) For me they definitely run with just cargo test... In general my experience has been that cargo test runs all tests it discovers, both unit tests and integration tests. Not sure what might cause that to not be happening for you. mdtest is just a normal Rust integration test.

crates/red_knot_python_semantic/src/types/infer.rs

crates/red_knot_python_semantic/src/types.rs

carljm · 2024-10-14T18:42:02Z

Hi @pilleye! I'd love to get this landed soon since it affects other work we want to do. Really appreciate your work on it so far, and if you don't have time to wrap it up that's no problem, we can pick it up. Let me know if we should go ahead and do that!

pilleye · 2024-10-15T19:06:04Z

Ah sorry, I've just been a bit busy the past week with other work, I'll try to get it in a ready-to-review state tonight.

Uses `Type::Never` instead of `Type::Unknown` for the case where a publicly available variable is unbound. In the case where it is unbound, we want a union of its actual type with `Never` instead of `Unbound`, because the `Unbound` case will cause runtime error.

pilleye · 2024-10-15T21:29:04Z

I'm stuck on a couple tests.

I also have another question, why don't we use symbol_ty(...) in infer.rs for this line:

let member_ty = module_ty.member(...)

carljm · 2024-10-15T23:11:43Z

I'm stuck on a couple tests.

Taking a look! Thanks for all your work on this. If the changes to get everything passing look reasonably small, I might just go ahead and push them and land this, in the interest of efficiency, hope that's OK with you.

I also have another question, why don't we use symbol_ty(...) in infer.rs for this line:
let member_ty = module_ty.member(...)

We kinda do use global_symbol_ty, because that's the entire implementation of Type::member for Type::Module types. It's just a layer of abstraction, since semantically what is happening here is an attribute access on a module object, and we model attribute accesses via Type::member.

pilleye · 2024-10-16T02:08:19Z

If the changes to get everything passing look reasonably small, I might just go ahead and push them and land this, in the interest of efficiency, hope that's OK with you.

Taking one more stab at it tonight, but feel free to finish it if it unblocks y'all.

pilleye requested review from carljm, MichaReiser and AlexWaygood as code owners September 30, 2024 03:02

pilleye force-pushed the pilleye/public-known branch 2 times, most recently from 4fcbb88 to 2436cb2 Compare September 30, 2024 03:05

pilleye changed the title ~~[red-knot] don't include Unknown in the type for a conditionally-defined import #13514~~ [red-knot] don't include Unknown in the type for a conditionally-defined import Sep 30, 2024

pilleye commented Sep 30, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

AlexWaygood added the red-knot Multi-file analysis & type inference label Sep 30, 2024

pilleye force-pushed the pilleye/public-known branch 2 times, most recently from 4e1fcc7 to b67b0b3 Compare September 30, 2024 20:35

carljm approved these changes Sep 30, 2024

View reviewed changes

pilleye force-pushed the pilleye/public-known branch from b67b0b3 to 4b3b6d7 Compare October 1, 2024 01:46

pilleye force-pushed the pilleye/public-known branch 2 times, most recently from d1c8432 to 2acaeef Compare October 1, 2024 01:55

pilleye force-pushed the pilleye/public-known branch from b389d98 to 4f8c324 Compare October 1, 2024 05:41

carljm requested changes Oct 1, 2024

View reviewed changes

pilleye force-pushed the pilleye/public-known branch from 4f8c324 to b19c32f Compare October 2, 2024 00:18

pilleye marked this pull request as draft October 2, 2024 00:18

MichaReiser mentioned this pull request Oct 8, 2024

[red-knot] remove Type::Unbound #13671

Open

pilleye force-pushed the pilleye/public-known branch from b19c32f to 7932d6c Compare October 15, 2024 19:10

pilleye force-pushed the pilleye/public-known branch 4 times, most recently from a18ae67 to a7f6b6c Compare October 15, 2024 20:47

pilleye force-pushed the pilleye/public-known branch from a7f6b6c to 77a9bff Compare October 15, 2024 21:28

pilleye marked this pull request as ready for review October 15, 2024 21:29

pilleye added 2 commits October 15, 2024 17:30

fix incorrect path

73ae646

add file diagnostic on never type

acbcb1e

pilleye added 5 commits October 15, 2024 22:19

undo changes to may be unbound

5a904dd

simplify contains_unbound

1e05e27

cargo fmt

e77b701

undo unbound to never changes in local tests

8baf3ae

don't fallback to unknown type with declarations

2cb2945

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] don't include Unknown in the type for a conditionally-defined import #13563

[red-knot] don't include Unknown in the type for a conditionally-defined import #13563

pilleye commented Sep 30, 2024

github-actions bot commented Sep 30, 2024 •

edited

Loading

carljm left a comment

carljm Sep 30, 2024

carljm commented Sep 30, 2024

pilleye commented Sep 30, 2024

pilleye commented Oct 1, 2024 •

edited

Loading

carljm commented Oct 1, 2024 •

edited

Loading

pilleye commented Oct 1, 2024 •

edited

Loading

carljm commented Oct 1, 2024 •

edited by AlexWaygood

Loading

pilleye commented Oct 1, 2024

carljm left a comment

carljm Oct 1, 2024

carljm Oct 1, 2024

carljm Oct 1, 2024

pilleye Oct 15, 2024 •

edited

Loading

carljm Oct 15, 2024

pilleye Oct 15, 2024

carljm Oct 15, 2024

pilleye Oct 15, 2024

carljm Oct 15, 2024

carljm commented Oct 14, 2024

pilleye commented Oct 15, 2024

pilleye commented Oct 15, 2024

carljm commented Oct 15, 2024

pilleye commented Oct 16, 2024

	fn resolve_unannotated_unbound_public() -> anyhow::Result<()> {
	fn resolve_unannotated_maybe_unbound_public() -> anyhow::Result<()> {

	fn resolve_unbound_public() -> anyhow::Result<()> {
	fn resolve_maybe_undeclared_maybe_unbound_public() -> anyhow::Result<()> {

		assert_public_ty(&db, "src/a.py", "r", "Literal[2]");
		assert_public_ty(&db, "src/a.py", "s", "Literal[5]");

[red-knot] don't include Unknown in the type for a conditionally-defined import #13563

Are you sure you want to change the base?

[red-knot] don't include Unknown in the type for a conditionally-defined import #13563

Conversation

pilleye commented Sep 30, 2024

Summary

Test Plan

github-actions bot commented Sep 30, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Sep 30, 2024

pilleye commented Sep 30, 2024

pilleye commented Oct 1, 2024 • edited Loading

carljm commented Oct 1, 2024 • edited Loading

pilleye commented Oct 1, 2024 • edited Loading

carljm commented Oct 1, 2024 • edited by AlexWaygood Loading

pilleye commented Oct 1, 2024

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilleye Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Oct 14, 2024

pilleye commented Oct 15, 2024

pilleye commented Oct 15, 2024

carljm commented Oct 15, 2024

pilleye commented Oct 16, 2024

github-actions bot commented Sep 30, 2024 •

edited

Loading

`ruff-ecosystem` results

pilleye commented Oct 1, 2024 •

edited

Loading

carljm commented Oct 1, 2024 •

edited

Loading

pilleye commented Oct 1, 2024 •

edited

Loading

carljm commented Oct 1, 2024 •

edited by AlexWaygood

Loading

pilleye Oct 15, 2024 •

edited

Loading