[ty] Add property test generators for variable-length tuples #18901
Conversation
dcreager
added
internal
|
MichaReiser
added
testing
There was a problem hiding this comment.
Looks good! But I am seeing some property test failures here (see below). We need to either fix the underlying problems, or create issues for them and move those property tests into the "flaky" section (but that would be less preferable). I think some of these are not OK to move into "flaky" (subtyping transitivity in particular.)
Repro: QUICKCHECK_TESTS=1000000 cargo test -p ty_python_semantic -- --ignored types::property_tests::stable
(Running with a million iterations like this is pretty slow, but is the standard we need to hit in order to not get flaky CI failures on the property tests.)
Failures:
failures:
---- types::property_tests::stable::all_type_pairs_are_assignable_to_their_union stdout ----
thread 'types::property_tests::stable::all_type_pairs_are_assignable_to_their_union' panicked at /Users/carlmeyer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (VariableLengthTuple([], Intersection { pos: [None, SubclassOfAbcClass("ABC")], neg: [] }, []), Union([FixedLengthTuple([]), VariableLengthTuple([AlwaysTruthy], KnownClassInstance(Str), [])]))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- types::property_tests::stable::all_fully_static_type_pairs_are_subtype_of_their_union stdout ----
thread 'types::property_tests::stable::all_fully_static_type_pairs_are_subtype_of_their_union' panicked at /Users/carlmeyer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (VariableLengthTuple([], BytesLiteral("\0"), []), Union([FixedLengthTuple([]), AlwaysTruthy]))
---- types::property_tests::stable::subtype_of_is_transitive stdout ----
thread 'types::property_tests::stable::subtype_of_is_transitive' panicked at /Users/carlmeyer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (FixedLengthTuple([]), VariableLengthTuple([], BuiltinClassLiteral("bool"), []), VariableLengthTuple([BuiltinInstance("type")], Intersection { pos: [], neg: [] }, []))
---- types::property_tests::stable::subtype_of_implies_not_disjoint_from stdout ----
thread 'types::property_tests::stable::subtype_of_implies_not_disjoint_from' panicked at /Users/carlmeyer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (Intersection { pos: [FixedLengthTuple([])], neg: [VariableLengthTuple([], BytesLiteral(""), [SubclassOfAbcClass("ABC")])] }, VariableLengthTuple([], VariableLengthTuple([], KnownClassInstance(Int), [Never]), []))
---- types::property_tests::stable::subtype_of_is_antisymmetric stdout ----
thread 'types::property_tests::stable::subtype_of_is_antisymmetric' panicked at /Users/carlmeyer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (VariableLengthTuple([], FixedLengthTuple([]), [FixedLengthTuple([])]), VariableLengthTuple([], FixedLengthTuple([]), []))
failures:
types::property_tests::stable::all_fully_static_type_pairs_are_subtype_of_their_union
types::property_tests::stable::all_type_pairs_are_assignable_to_their_union
types::property_tests::stable::subtype_of_implies_not_disjoint_from
types::property_tests::stable::subtype_of_is_antisymmetric
types::property_tests::stable::subtype_of_is_transitive
|
(Based on my experience debugging these property test failures recently, I recommend starting with the |
Ha! I ran it yesterday with around 500,000 runs and didn't see any failures. I ran it this morning to reproduce your results and got a failure with the default of 100 😂 |
* main: [ty] Fix false positives when subscripting an object inferred as having an `Intersection` type (#18920) [`flake8-use-pathlib`] Add autofix for `PTH202` (#18763) [ty] Add relative import completion tests [ty] Clarify what "cursor" means [ty] Add a cursor test builder [ty] Enforce sort order of completions (#18917) [formatter] Fix missing blank lines before decorated classes in .pyi files (#18888) Apply fix availability and applicability when adding to `DiagnosticGuard` and remove `NoqaCode::rule` (#18834) py-fuzzer: allow relative executable paths (#18915) [ty] Change `environment.root` to accept multiple paths (#18913) [ty] Rename `src.root` setting to `environment.root` (#18760) Use file path for detecting package root (#18914) Consider virtual path for various server actions (#18910) [ty] Introduce `UnionType::try_from_elements` and `UnionType::try_map` (#18911) [ty] Support narrowing on `isinstance()`/`issubclass()` if the second argument is a dynamic, intersection, union or typevar type (#18900) [ty] Add decorator check for implicit attribute assignments (#18587) [`ruff`] Trigger `RUF037` for empty string and byte strings (#18862) [ty] Avoid duplicate diagnostic in unpacking (#18897) [`pyupgrade`] Extend version detection to include `sys.version_info.major` (`UP036`) (#18633) [`ruff`] Frozen Dataclass default should be valid (`RUF009`) (#18735)
|
Okay it's been running on a loop for around 15 minutes now and I'm not seeing any more proptest failures. |
CodSpeed WallTime Performance ReportMerging #18901 will not alter performanceComparing Summary
|
|
Are the new primer diagnostics here expected? |
My hope is they're related to the test failure that I just fixed |
looks like we still have a few new ones |
| // If the variable-length portion is Never, it can only be instantiated with zero elements. | ||
| // That means this isn't a variable-length tuple after all! | ||
| if variable.is_never() { | ||
| return TupleSpec::Fixed(FixedLengthTupleSpec::from_elements( | ||
| prefix.into_iter().chain(suffix), | ||
| )); | ||
| } |
There was a problem hiding this comment.
this is a great observation! 😃
|
|
||
| fn suffix_elements( | ||
| &self, | ||
| ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ { |
There was a problem hiding this comment.
nit: this is quite a complicated type to repeat twice in the same file. Maybe introduce a type alias?
There was a problem hiding this comment.
Unfortunately it's not a type, it's an impl trait(s), so I don't think I can make an alias for it.
There was a problem hiding this comment.
you could maybe just use the concrete type instead of the trait, and then you can alias it? This seems to compile fine:
diff --git a/crates/ty_python_semantic/src/types/tuple.rs b/crates/ty_python_semantic/src/types/tuple.rs
index d19666fcd2..5d264a35f5 100644
--- a/crates/ty_python_semantic/src/types/tuple.rs
+++ b/crates/ty_python_semantic/src/types/tuple.rs
@@ -154,6 +154,8 @@ impl<'db> TupleType<'db> {
}
}
+type TupleElementIterator<'a, 'db> = std::iter::Copied<std::slice::Iter<'a, Type<'db>>>;
+
/// A fixed-length tuple spec.
///
/// Tuple specs are used for more than just `tuple` instances, so they allow `Never` to appear as a
@@ -179,9 +181,7 @@ impl<'db> FixedLengthTupleSpec<'db> {
&self.0
}
- pub(crate) fn elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ pub(crate) fn elements(&self) -> TupleElementIterator<'_, 'db> {
self.0.iter().copied()
}
@@ -372,9 +372,7 @@ impl<'db> VariableLengthTupleSpec<'db> {
})
}
- fn prefix_elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ fn prefix_elements(&self) -> TupleElementIterator<'_, 'db> {
self.prefix.iter().copied()
}
@@ -409,9 +407,7 @@ impl<'db> VariableLengthTupleSpec<'db> {
)
}
- fn suffix_elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ fn suffix_elements(&self) -> TupleElementIterator<'_, 'db> {
self.suffix.iter().copied()
}There was a problem hiding this comment.
I mildly prefer the impl trait, since I don't like encoding concrete types into the function signature if we don't need to name them for some other reason. But I can make this change if you feel strongly.
There was a problem hiding this comment.
I guess you could do something ✨fancy✨ like this, but it probably isn't worth it just to make some type signatures less complicated 😆
diff --git a/crates/ty_python_semantic/src/types/tuple.rs b/crates/ty_python_semantic/src/types/tuple.rs
index d19666fcd2..5c1c7b09e1 100644
--- a/crates/ty_python_semantic/src/types/tuple.rs
+++ b/crates/ty_python_semantic/src/types/tuple.rs
@@ -154,6 +154,16 @@ impl<'db> TupleType<'db> {
}
}
+pub(crate) trait TupleElementIterator<'db>:
+ ExactSizeIterator<Item = Type<'db>> + DoubleEndedIterator
+{
+}
+
+impl<'db, T> TupleElementIterator<'db> for T where
+ T: ExactSizeIterator<Item = Type<'db>> + DoubleEndedIterator
+{
+}
+
/// A fixed-length tuple spec.
///
/// Tuple specs are used for more than just `tuple` instances, so they allow `Never` to appear as a
@@ -179,9 +189,7 @@ impl<'db> FixedLengthTupleSpec<'db> {
&self.0
}
- pub(crate) fn elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ pub(crate) fn elements(&self) -> impl TupleElementIterator<'db> + '_ {
self.0.iter().copied()
}
@@ -372,9 +380,7 @@ impl<'db> VariableLengthTupleSpec<'db> {
})
}
- fn prefix_elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ fn prefix_elements(&self) -> impl TupleElementIterator<'db> + '_ {
self.prefix.iter().copied()
}
@@ -409,9 +415,7 @@ impl<'db> VariableLengthTupleSpec<'db> {
)
}
- fn suffix_elements(
- &self,
- ) -> impl DoubleEndedIterator<Item = Type<'db>> + ExactSizeIterator + '_ {
+ fn suffix_elements(&self) -> impl TupleElementIterator<'db> + '_ {
self.suffix.iter().copied()
}Feel free to leave things as they are, I don't feel strongly!
crates/ty_python_semantic/resources/mdtest/type_compendium/tuple.md
Outdated
Show resolved
Hide resolved
Most of them look like new true positives! This one is a false positive: There are a bunch of unannotated assignments to a variable in an |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
| ## Truthiness | ||
|
|
||
| The truthiness of the empty tuple is `False`: | ||
| The truthiness of the empty tuple is `False`. |
There was a problem hiding this comment.
I think it would be worth a note here like this:
| The truthiness of the empty tuple is `False`. | |
| The truthiness of the empty tuple is `False`. | |
| (TODO: this is only true as long as we assume that a subclass of `tuple` doesn't override `__bool__` or `__len__` -- we should enforce this.) |
There was a problem hiding this comment.
There is a note at the bottom of the section mentioning this (along with a test that shows we currently allow a subclass that overrides __bool__). Is that sufficient, or do you think it needs a note here too?
There was a problem hiding this comment.
(I'm also planning on tackling this this week or next as part of my tuple subclasses/NamedTuples work)
There was a problem hiding this comment.
Oh that's probably sufficient if we have it mentioned somewhere
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Add property test generators for the new variable-length tuples. This covers homogeneous tuples as well.
The property tests did their job! This identified several fixes we needed to make to various type property methods.
cf #18600 (comment)