From adbf05802a391de7fa86bd29da77fe48a86814f3 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 24 Oct 2025 18:30:54 +0200 Subject: [PATCH] [ty] Fix rare panic with highly cyclic `TypeVar` definitions (#21059) --- Cargo.lock | 7 +- Cargo.toml | 2 +- .../1377_iteration_count_mismatch.md | 149 ++++++++++++++++++ crates/ty_test/Cargo.toml | 3 +- crates/ty_test/README.md | 14 ++ crates/ty_test/src/lib.rs | 98 +++++++++--- crates/ty_test/src/parser.rs | 114 +++++++++----- fuzz/Cargo.toml | 2 +- 8 files changed, 318 insertions(+), 71 deletions(-) create mode 100644 crates/ty_python_semantic/resources/mdtest/regression/1377_iteration_count_mismatch.md diff --git a/Cargo.lock b/Cargo.lock index 8a6afb3be2..11897e189c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3563,7 +3563,7 @@ checksum = "28d3b2b1366ec20994f1fd18c3c594f05c5dd4bc44d8bb0c1c632c8d6829481f" [[package]] name = "salsa" version = "0.24.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=d38145c29574758de7ffbe8a13cd4584c3b09161#d38145c29574758de7ffbe8a13cd4584c3b09161" +source = "git+https://github.com/salsa-rs/salsa.git?rev=25b3ef146cfa2615f4ec82760bd0c22b454d0a12#25b3ef146cfa2615f4ec82760bd0c22b454d0a12" dependencies = [ "boxcar", "compact_str", @@ -3587,12 +3587,12 @@ dependencies = [ [[package]] name = "salsa-macro-rules" version = "0.24.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=d38145c29574758de7ffbe8a13cd4584c3b09161#d38145c29574758de7ffbe8a13cd4584c3b09161" +source = "git+https://github.com/salsa-rs/salsa.git?rev=25b3ef146cfa2615f4ec82760bd0c22b454d0a12#25b3ef146cfa2615f4ec82760bd0c22b454d0a12" [[package]] name = "salsa-macros" version = "0.24.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=d38145c29574758de7ffbe8a13cd4584c3b09161#d38145c29574758de7ffbe8a13cd4584c3b09161" +source = "git+https://github.com/salsa-rs/salsa.git?rev=25b3ef146cfa2615f4ec82760bd0c22b454d0a12#25b3ef146cfa2615f4ec82760bd0c22b454d0a12" dependencies = [ "proc-macro2", "quote", @@ -4521,7 +4521,6 @@ name = "ty_test" version = "0.0.0" dependencies = [ "anyhow", - "bitflags 2.9.4", "camino", "colored 3.0.0", "insta", diff --git a/Cargo.toml b/Cargo.toml index 22f4a5cbfd..301efbd180 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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", diff --git a/crates/ty_python_semantic/resources/mdtest/regression/1377_iteration_count_mismatch.md b/crates/ty_python_semantic/resources/mdtest/regression/1377_iteration_count_mismatch.md new file mode 100644 index 0000000000..600e408331 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/regression/1377_iteration_count_mismatch.md @@ -0,0 +1,149 @@ +# Iteration count mismatch for highly cyclic type vars + +Regression test for . + +The code is an excerpt from that is minimal enough to +trigger the iteration count mismatch bug in Salsa. + + + +```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"]): ... +``` diff --git a/crates/ty_test/Cargo.toml b/crates/ty_test/Cargo.toml index 670c46cc91..271ca8f084 100644 --- a/crates/ty_test/Cargo.toml +++ b/crates/ty_test/Cargo.toml @@ -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 } diff --git a/crates/ty_test/README.md b/crates/ty_test/README.md index 3db1832231..ecf4614d94 100644 --- a/crates/ty_test/README.md +++ b/crates/ty_test/README.md @@ -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 + +``` + +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, diff --git a/crates/ty_test/src/lib.rs b/crates/ty_test/src/lib.rs index f66eaaf26a..9460992312 100644 --- a/crates/ty_test/src/lib.rs +++ b/crates/ty_test/src/lib.rs @@ -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 `` \ - 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 `` \ + 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") + .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 + test_file: &'a TestFile, +) -> Result> 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, } - }) + } } diff --git a/crates/ty_test/src/parser.rs b/crates/ty_test/src/parser.rs index 12ad3b8039..bc039eece6 100644 --- a/crates/ty_test/src/parser.rs +++ b/crates/ty_test/src/parser.rs @@ -9,6 +9,7 @@ use anyhow::bail; use ruff_db::system::{SystemPath, SystemPathBuf}; use rustc_hash::FxHashMap; +use crate::config::MarkdownTestConfig; use ruff_index::{IndexVec, newtype_index}; use ruff_python_ast::PySourceType; use ruff_python_trivia::Cursor; @@ -16,8 +17,6 @@ use ruff_source_file::{LineIndex, LineRanges, OneIndexed}; use ruff_text_size::{TextLen, TextRange, TextSize}; use rustc_stable_hash::{FromStableHash, SipHasher128Hash, StableSipHasher128}; -use crate::config::MarkdownTestConfig; - /// Parse the Markdown `source` as a test suite with given `title`. pub(crate) fn parse<'s>(title: &'s str, source: &'s str) -> anyhow::Result> { let parser = Parser::new(title, source); @@ -145,13 +144,17 @@ impl<'m, 's> MarkdownTest<'m, 's> { pub(super) fn should_snapshot_diagnostics(&self) -> bool { self.section .directives - .contains(MdtestDirectives::SNAPSHOT_DIAGNOSTICS) + .has_directive_set(MdtestDirective::SnapshotDiagnostics) + } + + pub(super) fn should_expect_panic(&self) -> Result, ()> { + self.section.directives.get(MdtestDirective::ExpectPanic) } pub(super) fn should_skip_pulling_types(&self) -> bool { self.section .directives - .contains(MdtestDirectives::PULL_TYPES_SKIP) + .has_directive_set(MdtestDirective::PullTypesSkip) } } @@ -495,6 +498,7 @@ impl<'s> Parser<'s> { fn parse_impl(&mut self) -> anyhow::Result<()> { const SECTION_CONFIG_SNAPSHOT: &str = "snapshot-diagnostics"; const SECTION_CONFIG_PULLTYPES: &str = "pull-types:skip"; + const SECTION_CONFIG_EXPECT_PANIC: &str = "expect-panic"; const HTML_COMMENT_ALLOWLIST: &[&str] = &["blacken-docs:on", "blacken-docs:off"]; const CODE_BLOCK_END: &[u8] = b"```"; const HTML_COMMENT_END: &[u8] = b"-->"; @@ -506,16 +510,47 @@ impl<'s> Parser<'s> { memchr::memmem::find(self.cursor.as_bytes(), HTML_COMMENT_END) { let html_comment = self.cursor.as_str()[..position].trim(); - if html_comment == SECTION_CONFIG_SNAPSHOT { - self.process_mdtest_directive(MdtestDirective::SnapshotDiagnostics)?; - } else if html_comment == SECTION_CONFIG_PULLTYPES { - self.process_mdtest_directive(MdtestDirective::PullTypesSkip)?; - } else if !HTML_COMMENT_ALLOWLIST.contains(&html_comment) { - bail!( - "Unknown HTML comment `{html_comment}` -- possibly a typo? \ + let (directive, value) = match html_comment.split_once(':') { + Some((directive, value)) => { + (directive.trim(), Some(value.trim().to_string())) + } + None => (html_comment, None), + }; + + match directive { + SECTION_CONFIG_SNAPSHOT => { + anyhow::ensure!( + value.is_none(), + "The `{SECTION_CONFIG_SNAPSHOT}` directive does not take a value." + ); + self.process_mdtest_directive( + MdtestDirective::SnapshotDiagnostics, + None, + )?; + } + SECTION_CONFIG_PULLTYPES => { + anyhow::ensure!( + value.is_none(), + "The `{SECTION_CONFIG_PULLTYPES}` directive does not take a value." + ); + self.process_mdtest_directive( + MdtestDirective::PullTypesSkip, + None, + )?; + } + SECTION_CONFIG_EXPECT_PANIC => { + self.process_mdtest_directive(MdtestDirective::ExpectPanic, value)?; + } + _ => { + if !HTML_COMMENT_ALLOWLIST.contains(&html_comment) { + bail!( + "Unknown HTML comment `{html_comment}` -- possibly a typo? \ (Add to `HTML_COMMENT_ALLOWLIST` if this is a false positive)" - ); + ); + } + } } + self.cursor.skip_bytes(position + HTML_COMMENT_END.len()); } else { bail!("Unterminated HTML comment."); @@ -646,7 +681,7 @@ impl<'s> Parser<'s> { level: header_level.try_into()?, parent_id: Some(parent), config: self.sections[parent].config.clone(), - directives: self.sections[parent].directives, + directives: self.sections[parent].directives.clone(), }; if !self.current_section_files.is_empty() { @@ -793,7 +828,11 @@ impl<'s> Parser<'s> { Ok(()) } - fn process_mdtest_directive(&mut self, directive: MdtestDirective) -> anyhow::Result<()> { + fn process_mdtest_directive( + &mut self, + directive: MdtestDirective, + value: Option, + ) -> anyhow::Result<()> { if self.current_section_has_config { bail!( "Section config to enable {directive} must come before \ @@ -814,7 +853,7 @@ impl<'s> Parser<'s> { at most once.", ); } - current_section.directives.add_directive(directive); + current_section.directives.add_directive(directive, value); Ok(()) } @@ -833,12 +872,14 @@ impl<'s> Parser<'s> { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] enum MdtestDirective { /// A directive to enable snapshotting diagnostics. SnapshotDiagnostics, /// A directive to skip pull types. PullTypesSkip, + + ExpectPanic, } impl std::fmt::Display for MdtestDirective { @@ -846,40 +887,31 @@ impl std::fmt::Display for MdtestDirective { match self { MdtestDirective::SnapshotDiagnostics => f.write_str("snapshotting diagnostics"), MdtestDirective::PullTypesSkip => f.write_str("skipping the pull-types visitor"), + MdtestDirective::ExpectPanic => f.write_str("expect test to panic"), } } } -bitflags::bitflags! { - /// Directives that can be applied to a Markdown test section. - #[derive(Default, Debug, Clone, Copy, PartialEq, Eq)] - pub(crate) struct MdtestDirectives: u8 { - /// We should snapshot diagnostics for this section. - const SNAPSHOT_DIAGNOSTICS = 1 << 0; - /// We should skip pulling types for this section. - const PULL_TYPES_SKIP = 1 << 1; - } +/// The directives applied to a Markdown test section. +#[derive(Default, Debug, Clone, PartialEq, Eq)] +pub(crate) struct MdtestDirectives { + directives: FxHashMap>, } impl MdtestDirectives { - const fn has_directive_set(self, directive: MdtestDirective) -> bool { - match directive { - MdtestDirective::SnapshotDiagnostics => { - self.contains(MdtestDirectives::SNAPSHOT_DIAGNOSTICS) - } - MdtestDirective::PullTypesSkip => self.contains(MdtestDirectives::PULL_TYPES_SKIP), - } + fn has_directive_set(&self, directive: MdtestDirective) -> bool { + self.directives.contains_key(&directive) } - fn add_directive(&mut self, directive: MdtestDirective) { - match directive { - MdtestDirective::SnapshotDiagnostics => { - self.insert(MdtestDirectives::SNAPSHOT_DIAGNOSTICS); - } - MdtestDirective::PullTypesSkip => { - self.insert(MdtestDirectives::PULL_TYPES_SKIP); - } - } + fn get(&self, directive: MdtestDirective) -> Result, ()> { + self.directives + .get(&directive) + .map(|s| s.as_deref()) + .ok_or(()) + } + + fn add_directive(&mut self, directive: MdtestDirective, value: Option) { + self.directives.insert(directive, value); } } diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 24034b4854..82c9ae3be6 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -30,7 +30,7 @@ ty_python_semantic = { path = "../crates/ty_python_semantic" } ty_vendored = { path = "../crates/ty_vendored" } libfuzzer-sys = { git = "https://github.com/rust-fuzz/libfuzzer", default-features = false } -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",