[red-knot] Port type inference tests to new test framework #13719

Lexxxzy · 2024-10-11T18:32:18Z

Summary

Porting infer tests to new markdown tests framework.

Link to the corresponding issue: #13696

Lexxxzy · 2024-10-11T18:36:56Z

I've changed parser's CODE_RE regex, cause without that fix it was not correctly parsing code snippets with empty files, like:

```py path=package/__init__.py
```

Of course feel free to comment on this fix...

carljm

This is awesome, thank you for taking this on!

I'm going to wait for removal of the extraneous quadruple-backtick markdown fences before reviewing the tests in detail, because removing those will make them a lot easier to read :)

carljm · 2024-10-11T18:35:46Z

crates/red_knot_python_semantic/resources/mdtest/imports.md

+````markdown
+```py path=a.py
+from b import C as D; E = D
+reveal_type(E) # revealed: Literal[C]
+```
+
+```py path=b.py
+class C: pass
+```
+````


Oh, the outer markdown fenced code block is just how the mdtest README handles showing an example markdown document within a markdown README, it's not needed in the tests and should be removed here and in all other cases:

Suggested change

````markdown

```py path=a.py

from b import C as D; E = D

reveal_type(E) # revealed: Literal[C]

```

```py path=b.py

class C: pass

```

````

```py path=a.py

from b import C as D; E = D

reveal_type(E) # revealed: Literal[C]

```

```py path=b.py

class C: pass

```

It's funny that it still works either way, because of our naive regex parsing :)

I'll look at updating the README to address this potential confusion.

Fixed, my bad. Once again - thanks for clarifying

carljm · 2024-10-11T18:36:57Z

crates/red_knot_python_semantic/resources/mdtest/imports.md

+We can follow import to class:
+
+````markdown
+```py path=a.py


I would say it's probably better style in general if we don't specify the path on the "main" test file, if it isn't imported by another file and its location/name don't actually matter to the test. (This wasn't possible in the previous test format.) I don't care too much about this, though, not a blocking issue.

carljm · 2024-10-11T18:42:56Z

I've changed parser's CODE_RE regex

This fix looks good, thank you!

carljm · 2024-10-11T18:44:32Z

Also it looks like the mdformat pre-commit check is failing, you can check the ruff CONTRIBUTING doc for info on how to run pre-commit locally so you can catch those issues without having to push and wait for CI.

github-actions · 2024-10-11T18:45:54Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Lexxxzy · 2024-10-11T18:50:19Z

wait for removal of the extraneous quadruple-backtick

Oh, ok, understood, I will redo that, thanks for clarifing

Also it looks like the mdformat pre-commit check is failing, you can check the ruff CONTRIBUTING doc for info on how to run pre-commit locally so you can catch those issues without having to push and wait for CI.

Got it, totally forgot about it

Address a potential point of confusion that bit a contributor in #13719 Also remove a no-longer-accurate line about bare `error: ` assertions (which are no longer allowed) and clarify another point about which kinds of error assertions to use.

Lexxxzy · 2024-10-12T10:27:26Z

Rebased on the current main branch. Changed format of tests after new clarifications from @carljm. Also added tests for booleans infer.

I have problems with porting few tests so far:

not_literal_string, multiplied_string, multiplied_literal_string, truncated_string_literals_become_literal_string, adding_string_literals_and_literal_string, ...
In Rust's test there are formatting for variable in tests file like y = "a".repeat(TypeInferenceBuilder::MAX_STRING_LITERAL_SIZE + 1). The question is: how do I correctly insert variable in tests in mdtest the same way?
from_import_with_no_module_name and exception_handler_with_invalid_syntax

NOTE: Parsing diagnostic's error is not resolving, also already tried using full message
```py
from import bar   # error: "Expected a module name"
reveal_type(bar)  # revealed: Unknown
```

All comprehension tests (and some functions), because variables should be infered inside it's scope. Don't know how to do it in mdtest.

Lexxxzy · 2024-10-13T21:02:38Z

Most of the work is done. Now I’m just waiting for feedback on what’s finished and looking for some help with the issues I’m facing before I can keep going.

carljm

Thank you so much, this is a lot of good work! Excellent translation of the tests to the new assertion style.

A few general comments, which I commented partway through but not everywhere they occur:

Avoid TODO in test titles, and keep TODO comments minimal in length and as close as possible to the specific line/assertion that isn't quite right yet.
Avoid boilerplate phrases like "This test ensures that" or "Check that" or "We can infer the type when ..." or anything similar -- minimize the words that are not adding specific value to the test in question.
Break up into more smaller files; I commented on this throughout to try to give a clear idea of an organization that I think would work well.

I appreciate your quick work on this PR and would like to get it landed ASAP to minimize conflicts as new tests are added in other PRs! If you won't have time to update it in the next day-ish, please just let me know, I'm also happy to make the updates and get it landed!

Thanks again.

carljm · 2024-10-14T18:47:12Z

crates/red_knot_python_semantic/resources/mdtest/boolean.md

+### TODO: Not function
+
+Unknown should not be part of the type of typing.reveal_type
+
+```py
+from typing import reveal_type
+
+def f():
+    return 1
+
+a = not f
+b = not reveal_type
+
+reveal_type(a)  # revealed: Literal[False]
+# reveal_type(b)  # TODO: revealed: Literal[False]


Let's consolidate the stuff that's about the TODO to just the one line where it's relevant, that's not the main point of this test.

Suggested change

### TODO: Not function

Unknown should not be part of the type of typing.reveal_type

```py

from typing import reveal_type

def f():

return 1

a = not f

b = not reveal_type

reveal_type(a) # revealed: Literal[False]

# reveal_type(b) # TODO: revealed: Literal[False]

### Not function

```py

from typing import reveal_type

def f():

return 1

a = not f

b = not reveal_type

reveal_type(a) # revealed: Literal[False]

# TODO Unknown should not be part of the type of typing.reveal_type

# reveal_type(b) # revealed: Literal[False]

carljm · 2024-10-14T21:35:24Z

crates/red_knot_python_semantic/resources/mdtest/boolean.md

+reveal_type(f)  # revealed: Literal[False]
+```
+
+## Comparison


I'd like to break up this large file a bit, lets move out this whole section into a set of files, e.g. comparison/integers.md, comparison/non_boolean_returns.md, comparison/strings.md, comparison/unsupported.md. There's probably more that we could break out as well, but that's probably good enough for now.

carljm · 2024-10-14T21:38:20Z

crates/red_knot_python_semantic/resources/mdtest/classes.md

+
+## Cyclical class definition
+
+Python supports classes that can reference themselves in their base class definitions. Although it may seem unusual, such a structure is not uncommon, particularly in type hinting systems like `typeshed`, where base classes can be self-referential: `class str(Sequence[str]): ...`.


Let's generally hard-wrap lines at 80 columns in text, and also edit this text a bit for clarity/conciseness:

Suggested change

Python supports classes that can reference themselves in their base class definitions. Although it may seem unusual, such a structure is not uncommon, particularly in type hinting systems like `typeshed`, where base classes can be self-referential: `class str(Sequence[str]): ...`.

In type stubs, classes can reference themselves in their base class definitions. For example, in `typeshed`, we have `class str(Sequence[str]): ...`.

carljm · 2024-10-14T21:42:10Z

crates/red_knot_python_semantic/resources/mdtest/boolean.md

+NOTE: `j = "ab" < "ab_cd"` is a very cornercase test ensuring we're not comparing the interned salsa symbols, which compare by order of declaration.
+
+```py
+def str_instance() -> str: ...
+a = "abc" == "abc"
+b = "ab_cd" <= "ab_ce"
+c = "abc" in "ab cd"
+d = "" not in "hello"
+e = "--" is "--"
+f = "A" is "B"
+g = "--" is not "--"
+h = "A" is not "B"
+i = str_instance() < "..."
+j = "ab" < "ab_cd"


nit: I'd rather put notes like this as Python comments right next to the relevant line

Suggested change

NOTE: `j = "ab" < "ab_cd"` is a very cornercase test ensuring we're not comparing the interned salsa symbols, which compare by order of declaration.

```py

def str_instance() -> str: ...

a = "abc" == "abc"

b = "ab_cd" <= "ab_ce"

c = "abc" in "ab cd"

d = "" not in "hello"

e = "--" is "--"

f = "A" is "B"

g = "--" is not "--"

h = "A" is not "B"

i = str_instance() < "..."

j = "ab" < "ab_cd"

```py

def str_instance() -> str: ...

a = "abc" == "abc"

b = "ab_cd" <= "ab_ce"

c = "abc" in "ab cd"

d = "" not in "hello"

e = "--" is "--"

f = "A" is "B"

g = "--" is not "--"

h = "A" is not "B"

i = str_instance() < "..."

# ensure we're not comparing the interned salsa symbols, which compare by order of declaration.

j = "ab" < "ab_cd"

carljm · 2024-10-14T21:43:02Z

crates/red_knot_python_semantic/resources/mdtest/boolean.md

+TODO: `d = 5 < object()` should be `Unknown` but we don't check if __lt__ signature is valid for right operand type.
+
+```py
+a = 1 in 7      # error: "Operator `in` is not supported for types `Literal[1]` and `Literal[7]`"
+b = 0 not in 10 # error: "Operator `not in` is not supported for types `Literal[0]` and `Literal[10]`"
+c = object() < 5 # error: "Operator `<` is not supported for types `object` and `Literal[5]`"
+d = 5 < object()
+
+reveal_type(a)  # revealed: bool
+reveal_type(b)  # revealed: bool
+reveal_type(c)  # revealed: Unknown
+reveal_type(d)  # revealed: bool


Same here:

Suggested change

TODO: `d = 5 < object()` should be `Unknown` but we don't check if __lt__ signature is valid for right operand type.

```py

a = 1 in 7 # error: "Operator `in` is not supported for types `Literal[1]` and `Literal[7]`"

b = 0 not in 10 # error: "Operator `not in` is not supported for types `Literal[0]` and `Literal[10]`"

c = object() < 5 # error: "Operator `<` is not supported for types `object` and `Literal[5]`"

d = 5 < object()

reveal_type(a) # revealed: bool

reveal_type(b) # revealed: bool

reveal_type(c) # revealed: Unknown

reveal_type(d) # revealed: bool

```py

a = 1 in 7 # error: "Operator `in` is not supported for types `Literal[1]` and `Literal[7]`"

b = 0 not in 10 # error: "Operator `not in` is not supported for types `Literal[0]` and `Literal[10]`"

c = object() < 5 # error: "Operator `<` is not supported for types `object` and `Literal[5]`"

d = 5 < object()

reveal_type(a) # revealed: bool

reveal_type(b) # revealed: bool

reveal_type(c) # revealed: Unknown

# TODO should be `Unknown` but we don't yet check if `__lt__` signature is valid for right operand

reveal_type(d) # revealed: bool

carljm · 2024-10-14T22:33:06Z

crates/red_knot_python_semantic/resources/mdtest/strings.md

+reveal_type(a)  # revealed: str
+```
+
+## Subscript


These belong in subscript/string.md

carljm · 2024-10-14T22:33:33Z

crates/red_knot_python_semantic/resources/mdtest/strings.md

+reveal_type(a)  # revealed: str
+```
+
+## Bytes


Bytes are a separate type from strings, this should go in bytes.md or literal/bytes.md

carljm · 2024-10-14T22:34:45Z

crates/red_knot_python_semantic/resources/mdtest/variables.md

+reveal_type(x)  # revealed: Unbound
+```
+
+### Annotation only transparent to local inference


All the tests from here onward should go in shadowing.md along with some other tests I commented above.

carljm · 2024-10-14T22:35:13Z

crates/red_knot_python_semantic/resources/mdtest/variables.md

+reveal_type(x)  # revealed: Literal[1, 2]
+```
+
+## Assignment


These could go in assignment.md

carljm · 2024-10-14T22:35:28Z

crates/red_knot_python_semantic/resources/mdtest/variables.md

@@ -0,0 +1,134 @@
+# Variables
+
+## Union resolution


This test can go with some others in conditional/if_statement.md

Lexxxzy · 2024-10-14T23:50:19Z

would like to get it landed ASAP

All clear. Will try to push fixes as soon as possible

Lexxxzy · 2024-10-15T11:27:29Z

@carljm

A few general comments, which I commented partway through but not everywhere they occur

I tried to follow the style you described. Please take a look at the tests with the new structure. Sorry for all the issues!

…ype guards (#13758) ## Summary - Fix a bug with `… is not …` type guards. Previously, in an example like ```py x = [1] y = [1] if x is not y: reveal_type(x) ``` we would infer a type of `list[int] & ~list[int] == Never` for `x` inside the conditional (instead of `list[int]`), since we built a (negative) intersection with the type of the right hand side (`y`). However, as this example shows, this assumption can only be made for singleton types (types with a single inhabitant) such as `None`. - Add support for `… is …` type guards. closes #13715 ## Test Plan Moved existing `narrow_…` tests to Markdown-based tests and added new ones (including a regression test for the bug described above). Note that will create some conflicts with #13719. I tried to establish the correct organizational structure as proposed in #13719 (comment)

sharkdp · 2024-10-15T13:28:26Z

@Lexxxzy I already ported the two narrowing-related tests to Markdown while fixing a bug in the narrowing logic (#13758). Let me know if you need help resolving the conflicts here.

Except multiplied_string, multiplied_literal_string, truncated_string_literals_become_literal_string, adding_string_literals_and_literal_string

Except scope tests (unbound_function_local, implicit_global_in_function, conditionally_global_or_builtin, nonlocal_name_reference, nonlocal_name_reference_multi_level, nonlocal_name_reference_skips_class_scope, nonlocal_name_reference_skips_annotation_only_assignment)

Except exception_handler_with_invalid_syntax

Lexxxzy · 2024-10-15T14:54:37Z

@sharkdp Thanks for letting me know! I rebased on the current main, but somehow subscript/string.md - Subscript on strings - Function return test started to fail. Now it says that revealed type is @Todo, and not str. Can somebody check what went wrong? Maybe it is because of #5fa82f (Sync vendored typeshed stubs) commit?

Lexxxzy · 2024-10-15T14:58:16Z

Can somebody check what went wrong?

Yep, I checked, actually on @sharkdp pr merge commit (74bf4b) test is running fine, but on 5fa82f it started to fail

AlexWaygood · 2024-10-15T15:01:14Z

Can somebody check what went wrong?

Yep, I checked, actually on @sharkdp pr merge commit (74bf4b) test is running fine, but on 5fa82f it started to fail

Yes -- unfortunately I had to change the assertion the test was making as part of that PR (#13753). This is because typeshed now declares str.__getitem__ to be an overloaded function, and we don't understand overloaded functions yet.

AlexWaygood · 2024-10-15T15:02:33Z

This is the specific commit in that PR where I tweaked the test @Lexxxzy: 8ec3b30. You'll need to change the assertion to @Todo in your mdtest port of the test as well.

Sorry for the bother!

Lexxxzy · 2024-10-15T15:08:38Z

You'll need to change the assertion to @todo in your mdtest port of the test as well.

Ok, i see. Thanks for the info!

carljm

This is great!! Thank you so much for the quick and thorough work. I'm going to make a few minor cosmetic tweaks, push those, wait for CI, and then merge.

AlexWaygood · 2024-10-15T19:09:45Z

Thanks so much for taking this on @Lexxxzy! A big first contribution, and a really helpful one for us!!

Lexxxzy · 2024-10-15T19:41:39Z

Big thanks @carljm for reviewing my first PR to ruff! Learned a lot about that project while porting tests

Lexxxzy requested review from carljm, MichaReiser and AlexWaygood as code owners October 11, 2024 18:32

carljm requested changes Oct 11, 2024

View reviewed changes

carljm mentioned this pull request Oct 11, 2024

[red-knot] clarify mdtest README #13720

Merged

carljm added the red-knot Multi-file analysis & type inference label Oct 11, 2024

Lexxxzy force-pushed the type-infer-tests branch from 6114ee7 to 6ac74d3 Compare October 12, 2024 10:10

Lexxxzy force-pushed the type-infer-tests branch from 6ac74d3 to 5215c97 Compare October 12, 2024 10:31

Lexxxzy marked this pull request as draft October 12, 2024 10:46

Lexxxzy marked this pull request as ready for review October 13, 2024 21:04

Lexxxzy requested a review from carljm October 13, 2024 21:04

Lexxxzy mentioned this pull request Oct 14, 2024

[red-knot] mdtest doesn't spot a new Markdown test file has been added until you force a rebuild of the crate being tested #13732

Closed

sharkdp mentioned this pull request Oct 14, 2024

[red-knot] feat: Inference for BytesLiteral comparisons #13746

Merged

carljm reviewed Oct 14, 2024

View reviewed changes

sharkdp mentioned this pull request Oct 15, 2024

[red knot] Fix narrowing for '… is not …' type guards, add '… is …' type guards #13758

Merged

Lexxxzy force-pushed the type-infer-tests branch from 2cdafef to 44d6372 Compare October 15, 2024 11:22

Lexxxzy requested a review from carljm October 15, 2024 11:45

[red-knot] Fixed bug in test parser

59bed7e

Lexxxzy added 16 commits October 15, 2024 17:39

Added follow import tests and one more example to infer numbers

0e016ea

[red-knot] Adds & fixes to tests after review

6387b08

[red-knot] Added tests for boolean inference

a0a2679

[red-knot] Added test for string inference

26f0c19

Except multiplied_string, multiplied_literal_string, truncated_string_literals_become_literal_string, adding_string_literals_and_literal_string

[red-knot] Added more import infer tests

261021f

[red-knot] Tests for class inference

a65eff2

[red-knot] Conditionals inference tests

0fbaab9

[red-knot] Loops inference tests

7325805

[red-knot] More tests for numbers inference

591fa95

[red-knot] Variables inference tests

5e0e3c9

[red-knot] Added pattern matching tests

3fa2f39

[red-knot] Exception inference tests

3df08cc

Except exception_handler_with_invalid_syntax

[red-knot] Removed unused import

bab99fd

[red-knot] Porting to new structure after remarks

ec0be47

[red-knot] Added collections infer tests

1b657b2

Lexxxzy force-pushed the type-infer-tests branch from 9f27116 to 1b657b2 Compare October 15, 2024 14:50

[red-knot] Fix test after rebase

83dc1bf

carljm approved these changes Oct 15, 2024

View reviewed changes

mostly cosmetic adjustments

b340e2a

carljm merged commit d774807 into astral-sh:main Oct 15, 2024
20 checks passed

pilleye mentioned this pull request Oct 15, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Port type inference tests to new test framework #13719

[red-knot] Port type inference tests to new test framework #13719

Lexxxzy commented Oct 11, 2024 •

edited

Loading

Lexxxzy commented Oct 11, 2024

carljm left a comment

carljm Oct 11, 2024 •

edited

Loading

Lexxxzy Oct 12, 2024

carljm Oct 11, 2024

Lexxxzy Oct 12, 2024

carljm commented Oct 11, 2024

carljm commented Oct 11, 2024

github-actions bot commented Oct 11, 2024 •

edited

Loading

Lexxxzy commented Oct 11, 2024

Lexxxzy commented Oct 12, 2024 •

edited

Loading

Lexxxzy commented Oct 13, 2024

carljm left a comment

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

carljm Oct 14, 2024

Lexxxzy commented Oct 14, 2024

Lexxxzy commented Oct 15, 2024

sharkdp commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024

AlexWaygood commented Oct 15, 2024

AlexWaygood commented Oct 15, 2024 •

edited

Loading

Lexxxzy commented Oct 15, 2024

carljm left a comment

AlexWaygood commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024


		## Cyclical class definition

		Python supports classes that can reference themselves in their base class definitions. Although it may seem unusual, such a structure is not uncommon, particularly in type hinting systems like `typeshed`, where base classes can be self-referential: `class str(Sequence[str]): ...`.

	Python supports classes that can reference themselves in their base class definitions. Although it may seem unusual, such a structure is not uncommon, particularly in type hinting systems like `typeshed`, where base classes can be self-referential: `class str(Sequence[str]): ...`.
	In type stubs, classes can reference themselves in their base class definitions. For example, in `typeshed`, we have `class str(Sequence[str]): ...`.

[red-knot] Port type inference tests to new test framework #13719

[red-knot] Port type inference tests to new test framework #13719

Conversation

Lexxxzy commented Oct 11, 2024 • edited Loading

Summary

Lexxxzy commented Oct 11, 2024

carljm left a comment

Choose a reason for hiding this comment

carljm Oct 11, 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

carljm commented Oct 11, 2024

carljm commented Oct 11, 2024

github-actions bot commented Oct 11, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Lexxxzy commented Oct 11, 2024

Lexxxzy commented Oct 12, 2024 • edited Loading

Lexxxzy commented Oct 13, 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

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

Choose a reason for hiding this comment

Lexxxzy commented Oct 14, 2024

Lexxxzy commented Oct 15, 2024

sharkdp commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024

AlexWaygood commented Oct 15, 2024

AlexWaygood commented Oct 15, 2024 • edited Loading

Lexxxzy commented Oct 15, 2024

carljm left a comment

Choose a reason for hiding this comment

AlexWaygood commented Oct 15, 2024

Lexxxzy commented Oct 15, 2024

Lexxxzy commented Oct 11, 2024 •

edited

Loading

carljm Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

`ruff-ecosystem` results

Lexxxzy commented Oct 12, 2024 •

edited

Loading

AlexWaygood commented Oct 15, 2024 •

edited

Loading