[ty] Fix rare panic with highly cyclic TypeVar definitions (#21059)

Author: MichaReiser

Cargo.lock

@@ -146,7 +146,7 @@ regex-automata = { version = "0.4.9" }
 rustc-hash = { version = "2.0.0" }
 rustc-stable-hash = { version = "0.1.2" }
 # When updating salsa, make sure to also update the revision in `fuzz/Cargo.toml`
-salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "d38145c29574758de7ffbe8a13cd4584c3b09161", default-features = false, features = [
+salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "25b3ef146cfa2615f4ec82760bd0c22b454d0a12", default-features = false, features = [
     "compact_str",
     "macros",
     "salsa_unstable",

Cargo.toml

@@ -0,0 +1,149 @@
+# Iteration count mismatch for highly cyclic type vars
+
+Regression test for <https://github.com/astral-sh/ty/issues/1377>.
+
+The code is an excerpt from <https://github.com/Gobot1234/steam.py> that is minimal enough to
+trigger the iteration count mismatch bug in Salsa.
+
+<!-- expect-panic: execute: too many cycle iterations -->
+
+```toml
+[environment]
+extra-paths= ["/packages"]
+```
+
+`main.py`:
+
+```py
+from __future__ import annotations
+
+from typing import TypeAlias
+
+from steam.message import Message
+
+TestAlias: TypeAlias = tuple[Message]
+```
+
+`/packages/steam/__init__.py`:
+
+```py
+
+```
+
+`/packages/steam/abc.py`:
+
+```py
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import TYPE_CHECKING, Generic, Protocol
+
+from typing_extensions import TypeVar
+
+if TYPE_CHECKING:
+    from .clan import Clan
+    from .group import Group
+
+UserT = TypeVar("UserT", covariant=True)
+MessageT = TypeVar("MessageT", bound="Message", default="Message", covariant=True)
+
+class Messageable(Protocol[MessageT]): ...
+
+ClanT = TypeVar("ClanT", bound="Clan | None", default="Clan | None", covariant=True)
+GroupT = TypeVar("GroupT", bound="Group | None", default="Group | None", covariant=True)
+
+class Channel(Messageable[MessageT], Generic[MessageT, ClanT, GroupT]): ...
+
+ChannelT = TypeVar("ChannelT", bound=Channel, default=Channel, covariant=True)
+
+class Message(Generic[UserT, ChannelT]): ...
+```
+
+`/packages/steam/chat.py`:
+
+```py
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Generic, TypeAlias
+
+from typing_extensions import Self, TypeVar
+
+from .abc import Channel, ClanT, GroupT, Message
+
+if TYPE_CHECKING:
+    from .clan import Clan
+    from .message import ClanMessage, GroupMessage
+
+ChatT = TypeVar("ChatT", bound="Chat", default="Chat", covariant=True)
+MemberT = TypeVar("MemberT", covariant=True)
+
+AuthorT = TypeVar("AuthorT", covariant=True)
+
+class ChatMessage(Message[AuthorT, ChatT], Generic[AuthorT, MemberT, ChatT]): ...
+
+ChatMessageT = TypeVar("ChatMessageT", bound="GroupMessage | ClanMessage", default="GroupMessage | ClanMessage", covariant=True)
+
+class Chat(Channel[ChatMessageT, ClanT, GroupT]): ...
+
+ChatGroupTypeT = TypeVar("ChatGroupTypeT", covariant=True)
+
+class ChatGroup(Generic[MemberT, ChatT, ChatGroupTypeT]): ...
+```
+
+`/packages/steam/channel.py`:
+
+```py
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Any
+
+from .chat import Chat
+
+if TYPE_CHECKING:
+    from .clan import Clan
+
+class ClanChannel(Chat["Clan", None]): ...
+```
+
+`/packages/steam/clan.py`:
+
+```py
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, TypeVar
+
+from typing_extensions import Self
+
+from .chat import ChatGroup
+
+class Clan(ChatGroup[str], str): ...
+```
+
+`/packages/steam/group.py`:
+
+```py
+from __future__ import annotations
+
+from .chat import ChatGroup
+
+class Group(ChatGroup[str]): ...
+```
+
+`/packages/steam/message.py`:
+
+```py
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+from typing_extensions import TypeVar
+
+from .abc import BaseUser, Message
+from .chat import ChatMessage
+
+if TYPE_CHECKING:
+    from .channel import ClanChannel
+
+class GroupMessage(ChatMessage["str"]): ...
+class ClanMessage(ChatMessage["ClanChannel"]): ...
+```

crates/ty_python_semantic/resources/mdtest/regression/1377_iteration_count_mismatch.md

@@ -23,12 +23,11 @@ ty_static = { workspace = true }
 ty_vendored = { workspace = true }
 
 anyhow = { workspace = true }
-bitflags = { workspace = true }
 camino = { workspace = true }
 colored = { workspace = true }
 insta = { workspace = true, features = ["filters"] }
 memchr = { workspace = true }
-path-slash ={ workspace = true }
+path-slash = { workspace = true }
 regex = { workspace = true }
 rustc-hash = { workspace = true }
 rustc-stable-hash = { workspace = true }

crates/ty_test/Cargo.toml

@@ -180,6 +180,20 @@ snapshotting.
 At present, there is no way to do inline snapshotting or to request more granular
 snapshotting of specific diagnostics.
 
+## Expected panics
+
+It is possible to write tests that expect the type checker to panic during checking. Ideally, we'd fix those panics
+but being able to add regression tests even before is useful.
+
+To mark a test as expecting a panic, add an HTML comment like this:
+
+```markdown
+<!-- expect-panic: assertion `left == right` failed: Can't merge cycle heads -->
+```
+
+The text after `expect-panic:` is a substring that must appear in the panic message. The message is optional;
+but it is recommended to avoid false positives.
+
 ## Multi-file tests
 
 Some tests require multiple files, with imports from one file into another. For this purpose,

crates/ty_test/README.md

@@ -8,7 +8,7 @@ use parser as test_parser;
 use ruff_db::Db as _;
 use ruff_db::diagnostic::{Diagnostic, DiagnosticId, DisplayDiagnosticConfig};
 use ruff_db::files::{File, FileRootKind, system_path_to_file};
-use ruff_db::panic::catch_unwind;
+use ruff_db::panic::{PanicError, catch_unwind};
 use ruff_db::parsed::parsed_module;
 use ruff_db::system::{DbWithWritableSystem as _, SystemPath, SystemPathBuf};
 use ruff_db::testing::{setup_logging, setup_logging_with_filter};
@@ -319,6 +319,7 @@ fn run_test(
     let mut snapshot_diagnostics = vec![];
 
     let mut any_pull_types_failures = false;
+    let mut panic_info = None;
 
     let mut failures: Failures = test_files
         .iter()
@@ -338,10 +339,17 @@ fn run_test(
                     .map(|error| Diagnostic::invalid_syntax(test_file.file, error, error)),
             );
 
-            let mdtest_result = attempt_test(db, check_types, test_file, "run mdtest", None);
+            let mdtest_result = attempt_test(db, check_types, test_file);
             let type_diagnostics = match mdtest_result {
                 Ok(diagnostics) => diagnostics,
-                Err(failures) => return Some(failures),
+                Err(failures) => {
+                    if test.should_expect_panic().is_ok() {
+                        panic_info = Some(failures.info);
+                        return None;
+                    }
+
+                    return Some(failures.into_file_failures(db, "run mdtest", None));
+                }
             };
 
             diagnostics.extend(type_diagnostics);
@@ -367,22 +375,20 @@ fn run_test(
                 }));
             }
 
-            let pull_types_result = attempt_test(
-                db,
-                pull_types,
-                test_file,
-                "\"pull types\"",
-                Some(
-                    "Note: either fix the panic or add the `<!-- pull-types:skip -->` \
-                    directive to this test",
-                ),
-            );
+            let pull_types_result = attempt_test(db, pull_types, test_file);
             match pull_types_result {
                 Ok(()) => {}
                 Err(failures) => {
                     any_pull_types_failures = true;
                     if !test.should_skip_pulling_types() {
-                        return Some(failures);
+                        return Some(failures.into_file_failures(
+                            db,
+                            "\"pull types\"",
+                            Some(
+                                "Note: either fix the panic or add the `<!-- pull-types:skip -->` \
+                    directive to this test",
+                            ),
+                        ));
                     }
                 }
             }
@@ -391,6 +397,39 @@ fn run_test(
         })
         .collect();
 
+    match panic_info {
+        Some(panic_info) => {
+            let expected_message = test
+                .should_expect_panic()
+                .expect("panic_info is only set when `should_expect_panic` is `Ok`");
+
+            let message = panic_info
+                .payload
+                .as_str()
+                .unwrap_or("Box<dyn Any>")
+                .to_string();
+
+            if let Some(expected_message) = expected_message {
+                assert!(
+                    message.contains(expected_message),
+                    "Test `{}` is expected to panic with `{expected_message}`, but panicked with `{message}` instead.",
+                    test.name()
+                );
+            }
+        }
+        None => {
+            if let Ok(message) = test.should_expect_panic() {
+                if let Some(message) = message {
+                    panic!(
+                        "Test `{}` is expected to panic with `{message}`, but it didn't.",
+                        test.name()
+                    );
+                }
+                panic!("Test `{}` is expected to panic but it didn't.", test.name());
+            }
+        }
+    }
+
     if test.should_skip_pulling_types() && !any_pull_types_failures {
         let mut by_line = matcher::FailuresByLine::default();
         by_line.push(
@@ -596,17 +635,32 @@ fn create_diagnostic_snapshot(
 ///
 /// If a panic occurs, a nicely formatted [`FileFailures`] is returned as an `Err()` variant.
 /// This will be formatted into a diagnostic message by `ty_test`.
-fn attempt_test<'db, T, F>(
+fn attempt_test<'db, 'a, T, F>(
     db: &'db Db,
     test_fn: F,
-    test_file: &TestFile,
-    action: &str,
-    clarification: Option<&str>,
-) -> Result<T, FileFailures>
+    test_file: &'a TestFile,
+) -> Result<T, AttemptTestError<'a>>
 where
     F: FnOnce(&'db dyn ty_python_semantic::Db, File) -> T + std::panic::UnwindSafe,
 {
-    catch_unwind(|| test_fn(db, test_file.file)).map_err(|info| {
+    catch_unwind(|| test_fn(db, test_file.file))
+        .map_err(|info| AttemptTestError { info, test_file })
+}
+
+struct AttemptTestError<'a> {
+    info: PanicError,
+    test_file: &'a TestFile,
+}
+
+impl AttemptTestError<'_> {
+    fn into_file_failures(
+        self,
+        db: &Db,
+        action: &str,
+        clarification: Option<&str>,
+    ) -> FileFailures {
+        let info = self.info;
+
         let mut by_line = matcher::FailuresByLine::default();
         let mut messages = vec![];
         match info.location {
@@ -652,8 +706,8 @@ where
         by_line.push(OneIndexed::from_zero_indexed(0), messages);
 
         FileFailures {
-            backtick_offsets: test_file.backtick_offsets.clone(),
+            backtick_offsets: self.test_file.backtick_offsets.clone(),
             by_line,
         }
-    })
+    }
 }