Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[red-knot] Minor: fix Literal[True] <: int #14177

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Nov 7, 2024

Summary

Minor fix to Type::is_subtype_of to make sure that Boolean literals are subtypes of int, to match runtime semantics.

Found this while doing some property-testing experiments.

Test Plan

New unit test.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Nov 7, 2024
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!

@sharkdp sharkdp force-pushed the david/fix-bool-literal-subtype-int branch from 74ba518 to 38d93ea Compare November 7, 2024 22:15
@sharkdp sharkdp merged commit 2624249 into main Nov 7, 2024
20 checks passed
@sharkdp sharkdp deleted the david/fix-bool-literal-subtype-int branch November 7, 2024 22:23
Copy link
Contributor

github-actions bot commented Nov 7, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

sharkdp added a commit that referenced this pull request Dec 3, 2024
## Summary

This PR adds a new `property_tests` module with quickcheck-based tests
that verify certain properties of types. The following properties are
currently checked:

* `is_equivalent_to`:
  * is reflexive: `T` is equivalent to itself
* `is_subtype_of`:
  * is reflexive: `T` is a subtype of `T`
* is antisymmetric: if `S <: T` and `T <: S`, then `S` is equivalent to
`T`
  * is transitive: `S <: T` & `T <: U` => `S <: U`
* `is_disjoint_from`:
  * is irreflexive: `T` is not disjoint from `T`
  * is symmetric: `S` disjoint from `T` => `T` disjoint from `S`
* `is_assignable_to`:
  * is reflexive
* `negate`:
  * is an involution: `T.negate().negate()` is equivalent to `T`

There are also some tests that validate higher-level properties like:

* `S <: T` implies that `S` is not disjoint from `T`
* `S <: T` implies that `S` is assignable to `T`
* A singleton type must also be single-valued

These tests found a few bugs so far:

- #14177 
- #14195 
- #14196 
- #14210
- #14731

Some additional notes:

- Quickcheck-based property tests are non-deterministic and finding
counter-examples might take an arbitrary long time. This makes them bad
candidates for running in CI (for every PR). We can think of running
them in a cron-job way from time to time, similar to fuzzing. But for
now, it's only possible to run them locally (see instructions in source
code).
- Some tests currently find false positive "counterexamples" because our
understanding of equivalence of types is not yet complete. We do not
understand that `int | str` is the same as `str | int`, for example.
These tests are in a separate `property_tests::flaky` module.
- Properties can not be formulated in every way possible, due to the
fact that `is_disjoint_from` and `is_subtype_of` can produce false
negative answers.
- The current shrinking implementation is very naive, which leads to
counterexamples that are very long (`str & Any & ~tuple[Any] &
~tuple[Unknown] & ~Literal[""] & ~Literal["a"] | str & int & ~tuple[Any]
& ~tuple[Unknown]`), requiring the developer to simplify manually. It
has not been a major issue so far, but there is a comment in the code
how this can be improved.
- The tests are currently implemented using a macro. This is a single
commit on top which can easily be reverted, if we prefer the plain code
instead. With the macro:
  ```rs
  // `S <: T` implies that `S` can be assigned to `T`.
  type_property_test!(
      subtype_of_implies_assignable_to, db,
forall types s, t. s.is_subtype_of(db, t) => s.is_assignable_to(db, t)
  );
  ```
  without the macro:
  ```rs
  /// `S <: T` implies that `S` can be assigned to `T`.
  #[quickcheck]
  fn subtype_of_implies_assignable_to(s: Ty, t: Ty) -> bool {
      let db = get_cached_db();
  
      let s = s.into_type(&db);
      let t = t.into_type(&db);
  
      !s.is_subtype_of(&*db, t) || s.is_assignable_to(&*db, t)
  }
  ```

## Test Plan

```bash
while cargo test --release -p red_knot_python_semantic --features property_tests types::property_tests; do :; done
```
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants