From ed90e39537e1b802ef743a06de821a550fb012b7 Mon Sep 17 00:00:00 2001 From: Gustavo Ocanto Date: Wed, 21 Oct 2026 17:14:45 +0800 Subject: [PATCH 1/4] Adopt t3code's lint rules: ban-ts-comment and six anti-slop rules The bundled policy now requires a reason on @ts-ignore, @ts-expect-error and @ts-nocheck (typescript/ban-ts-comment, allow-with-description). New anti-slop rules, off by default like the rest: - no-test-in-loop: no describe/it/test inside a for loop; use .each. - no-unreasoned-test-skip: a skipped or todo test links its issue. - no-low-value-comments: dividers, step labels, unlinked TODOs, JSDoc types in TypeScript, empty JSDoc tags, commented-out code, and comments that restate the next name. - no-hookless-use-function: a use* function in a React file calls a hook. - no-real-time-wait-in-tests: no promise-timer or new-Promise sleeps in tests unless the file uses fake timers. - no-polling-loop-in-tests: no loop that spins on a tick or sleep until a predicate holds. no-ambient-nondeterminism also catches process.hrtime.bigint(), and no longer reports values passed to mocks (mockReturnValue, vi.fn, ...). --- README.md | 2 +- crates/lint/policy/oxlintrc.json | 8 + crates/lint/src/rules/anti_slop/mod.rs | 20 +- .../anti_slop/no_ambient_nondeterminism.rs | 48 +++- .../anti_slop/no_hookless_use_function.rs | 131 +++++++++ .../rules/anti_slop/no_low_value_comments.rs | 251 ++++++++++++++++++ .../src/rules/anti_slop/no_module_mocking.rs | 30 +-- .../anti_slop/no_polling_loop_in_tests.rs | 183 +++++++++++++ .../anti_slop/no_real_time_wait_in_tests.rs | 64 +++++ .../src/rules/anti_slop/no_test_in_loop.rs | 73 +++++ .../anti_slop/no_unreasoned_test_skip.rs | 93 +++++++ crates/lint/src/rules/anti_slop/shared/mod.rs | 7 + .../src/rules/anti_slop/shared/testing.rs | 70 +++++ .../lint/src/rules/anti_slop/shared/timers.rs | 141 ++++++++++ crates/lint/src/rules/anti_slop/tests/mod.rs | 23 +- .../tests/no_ambient_nondeterminism.rs | 52 ++++ .../tests/no_hookless_use_function.rs | 65 +++++ .../anti_slop/tests/no_low_value_comments.rs | 101 +++++++ .../tests/no_polling_loop_in_tests.rs | 80 ++++++ .../tests/no_real_time_wait_in_tests.rs | 72 +++++ .../rules/anti_slop/tests/no_test_in_loop.rs | 59 ++++ .../tests/no_unreasoned_test_skip.rs | 74 ++++++ crates/lint/tests/linter.rs | 34 +++ 23 files changed, 1641 insertions(+), 40 deletions(-) create mode 100644 crates/lint/src/rules/anti_slop/no_hookless_use_function.rs create mode 100644 crates/lint/src/rules/anti_slop/no_low_value_comments.rs create mode 100644 crates/lint/src/rules/anti_slop/no_polling_loop_in_tests.rs create mode 100644 crates/lint/src/rules/anti_slop/no_real_time_wait_in_tests.rs create mode 100644 crates/lint/src/rules/anti_slop/no_test_in_loop.rs create mode 100644 crates/lint/src/rules/anti_slop/no_unreasoned_test_skip.rs create mode 100644 crates/lint/src/rules/anti_slop/shared/testing.rs create mode 100644 crates/lint/src/rules/anti_slop/shared/timers.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_low_value_comments.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_polling_loop_in_tests.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_real_time_wait_in_tests.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_test_in_loop.rs create mode 100644 crates/lint/src/rules/anti_slop/tests/no_unreasoned_test_skip.rs diff --git a/README.md b/README.md index d949d13..552138a 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ fmtkit changes layout and applies safe lint fixes; it does not change behaviour **Vue, HTML, Markdown:** formatted as whole documents. Scripts inside them go through the script pipeline; styles and CSS fences through the oxc CSS formatter. Markdown prose is not rewrapped. -**Lint:** Oxlint (pinned to `oxlint_v1.86.0`) with a bundled policy of 197 rules, including native `perfectionist/*`, `@nkzw/*`, and `no-only-tests/*` rules. The `anti-slop/*` rules are available but off by default. `error` fails the run; `warn` does not. +**Lint:** Oxlint (pinned to `oxlint_v1.86.0`) with a bundled policy of 198 rules, including native `perfectionist/*`, `@nkzw/*`, and `no-only-tests/*` rules. The `anti-slop/*` rules are available but off by default. `error` fails the run; `warn` does not. ### What it leaves alone diff --git a/crates/lint/policy/oxlintrc.json b/crates/lint/policy/oxlintrc.json index 9d8d963..39b22ad 100644 --- a/crates/lint/policy/oxlintrc.json +++ b/crates/lint/policy/oxlintrc.json @@ -237,6 +237,14 @@ "default": "generic" } ], + "typescript/ban-ts-comment": [ + "error", + { + "ts-expect-error": "allow-with-description", + "ts-ignore": "allow-with-description", + "ts-nocheck": "allow-with-description" + } + ], "typescript/consistent-type-imports": "error", "typescript/no-duplicate-enum-values": "error", "typescript/no-empty-object-type": "error", diff --git a/crates/lint/src/rules/anti_slop/mod.rs b/crates/lint/src/rules/anti_slop/mod.rs index d4360cb..8c02244 100644 --- a/crates/lint/src/rules/anti_slop/mod.rs +++ b/crates/lint/src/rules/anti_slop/mod.rs @@ -1,23 +1,31 @@ -//! fmtkit's own `anti-slop` rules, ported from the v1 oxlint JS plugin. +//! fmtkit's own `anti-slop` rules: the v1 oxlint JS plugin, ported, and +//! rules adapted from t3code's oxlint plugin +//! (). //! //! The rules read oxc's AST through [`shared`], which hides the nodes ESTree -//! does not have, so they report what v1 reported at the same spans. +//! does not have, so the v1 rules report what v1 reported at the same spans. pub mod shared; mod no_ambient_nondeterminism; mod no_chained_type_assertions; mod no_conditional_empty_object_spread; +mod no_hookless_use_function; mod no_known_value_widening; +mod no_low_value_comments; mod no_module_mocking; mod no_object_parameters; +mod no_polling_loop_in_tests; +mod no_real_time_wait_in_tests; mod no_reflect_apply; mod no_reflect_get; mod no_runtime_typeof; mod no_shape_in_symbol_names; +mod no_test_in_loop; mod no_unknown_parameters; mod no_unknown_returns; mod no_unknown_type_aliases; +mod no_unreasoned_test_skip; mod no_unsafe_dictionary_type; mod no_widen_then_assert; mod require_safety_comment_for_type_assertion; @@ -29,22 +37,28 @@ mod tests; use super::Factory; -/// Every v1 rule, by full name. Only `no-runtime-typeof` takes options; the +/// Every rule, by full name. Only `no-runtime-typeof` takes options; the /// others ignore any they are given, as v1 did. pub const RULES: &[(&str, Factory)] = &[ (no_ambient_nondeterminism::NAME, |_| Ok(Box::new(no_ambient_nondeterminism::NoAmbientNondeterminism))), (no_chained_type_assertions::NAME, |_| Ok(Box::new(no_chained_type_assertions::NoChainedTypeAssertions))), (no_conditional_empty_object_spread::NAME, |_| Ok(Box::new(no_conditional_empty_object_spread::NoConditionalEmptyObjectSpread))), + (no_hookless_use_function::NAME, |_| Ok(Box::new(no_hookless_use_function::NoHooklessUseFunction))), (no_known_value_widening::NAME, |_| Ok(Box::new(no_known_value_widening::NoKnownValueWidening))), + (no_low_value_comments::NAME, |_| Ok(Box::new(no_low_value_comments::NoLowValueComments))), (no_module_mocking::NAME, |_| Ok(Box::new(no_module_mocking::NoModuleMocking))), (no_object_parameters::NAME, |_| Ok(Box::new(no_object_parameters::NoObjectParameters))), + (no_polling_loop_in_tests::NAME, |_| Ok(Box::new(no_polling_loop_in_tests::NoPollingLoopInTests))), + (no_real_time_wait_in_tests::NAME, |_| Ok(Box::new(no_real_time_wait_in_tests::NoRealTimeWaitInTests))), (no_reflect_apply::NAME, |_| Ok(Box::new(no_reflect_apply::NoReflectApply))), (no_reflect_get::NAME, |_| Ok(Box::new(no_reflect_get::NoReflectGet))), (no_runtime_typeof::NAME, no_runtime_typeof::build), (no_shape_in_symbol_names::NAME, |_| Ok(Box::new(no_shape_in_symbol_names::NoShapeInSymbolNames))), + (no_test_in_loop::NAME, |_| Ok(Box::new(no_test_in_loop::NoTestInLoop))), (no_unknown_parameters::NAME, |_| Ok(Box::new(no_unknown_parameters::NoUnknownParameters))), (no_unknown_returns::NAME, |_| Ok(Box::new(no_unknown_returns::NoUnknownReturns))), (no_unknown_type_aliases::NAME, |_| Ok(Box::new(no_unknown_type_aliases::NoUnknownTypeAliases))), + (no_unreasoned_test_skip::NAME, |_| Ok(Box::new(no_unreasoned_test_skip::NoUnreasonedTestSkip))), (no_unsafe_dictionary_type::NAME, |_| Ok(Box::new(no_unsafe_dictionary_type::NoUnsafeDictionaryType))), (no_widen_then_assert::NAME, |_| Ok(Box::new(no_widen_then_assert::NoWidenThenAssert))), (require_safety_comment_for_type_assertion::NAME, |_| Ok(Box::new(require_safety_comment_for_type_assertion::RequireSafetyCommentForTypeAssertion))), diff --git a/crates/lint/src/rules/anti_slop/no_ambient_nondeterminism.rs b/crates/lint/src/rules/anti_slop/no_ambient_nondeterminism.rs index 568feae..a0cb836 100644 --- a/crates/lint/src/rules/anti_slop/no_ambient_nondeterminism.rs +++ b/crates/lint/src/rules/anti_slop/no_ambient_nondeterminism.rs @@ -1,12 +1,18 @@ //! `anti-slop/no-ambient-nondeterminism`: no reading the clock or the entropy //! source; deterministic code takes time and randomness from its caller. +//! +//! A reading passed to a mock (`vi.setSystemTime(Date.now())`, or inside a +//! `mockReturnValue` or `vi.mock` factory) seeds fake time rather than measures +//! the run, so it is allowed, as in t3code's `t3code/no-wall-clock-in-tests`. use oxc_ast::AstKind; use oxc_ast::ast::Expression; use oxc_semantic::AstNode; +use oxc_span::GetSpan; use super::super::{Context, Rule}; -use super::shared::{is_global, is_global_member}; +use super::shared::testing::is_test_framework_object; +use super::shared::{ancestors, is_global, is_global_member, member}; pub const NAME: &str = "anti-slop/no-ambient-nondeterminism"; @@ -20,6 +26,13 @@ const AMBIENT_MEMBERS: [(&str, &str, &str); 6] = [ ("process", "hrtime", "process.hrtime()"), ]; +/// Methods of a mock function that set what it returns. +const MOCK_METHODS: [&str; 6] = + ["mockImplementation", "mockImplementationOnce", "mockResolvedValue", "mockResolvedValueOnce", "mockReturnValue", "mockReturnValueOnce"]; + +/// Members of `vi` and `jest` that build a mock or set fake time. +const FRAMEWORK_MOCKS: [&str; 5] = ["doMock", "fn", "mock", "setSystemTime", "spyOn"]; + pub struct NoAmbientNondeterminism; impl Rule for NoAmbientNondeterminism { @@ -34,17 +47,46 @@ impl Rule for NoAmbientNondeterminism { return; } - if let Some((_, _, source)) = AMBIENT_MEMBERS.iter().find(|(owner, property, _)| is_global_member(ctx, &call.callee, owner, property)) { + let source = AMBIENT_MEMBERS + .iter() + .find(|(owner, property, _)| is_global_member(ctx, &call.callee, owner, property)) + .map(|(_, _, source)| *source) + .or_else(|| is_hrtime_bigint(ctx, &call.callee).then_some("process.hrtime.bigint()")); + + if let Some(source) = source + && !feeds_mock(ctx, node) + { ctx.report( call.span, format!("`{source}` makes this output depend on when and where it ran. Take the value as a parameter so the caller owns it."), ); } } - AstKind::NewExpression(new) if new.arguments.is_empty() && is_global(ctx, &new.callee, "Date") => { + AstKind::NewExpression(new) if new.arguments.is_empty() && is_global(ctx, &new.callee, "Date") && !feeds_mock(ctx, node) => { ctx.report(new.span, "`new Date()` without an argument reads the ambient clock. Take the instant as a parameter so the caller owns it."); } _ => {} } } } + +/// `process.hrtime.bigint`, off the unshadowed global `process`. +fn is_hrtime_bigint(ctx: &Context<'_, '_>, callee: &Expression<'_>) -> bool { + member(callee).is_some_and(|(object, property)| property == "bigint" && is_global_member(ctx, object, "process", "hrtime")) +} + +/// Whether `node` is inside the arguments of a call that builds a mock or sets fake time. +fn feeds_mock(ctx: &Context<'_, '_>, node: &AstNode<'_>) -> bool { + let span = node.kind().span(); + + ancestors(ctx.semantic.nodes(), node.id()).any(|ancestor| { + let AstKind::CallExpression(call) = ancestor.kind() else { + return false; + }; + + !call.callee.span().contains_inclusive(span) + && member(&call.callee).is_some_and(|(object, property)| { + MOCK_METHODS.contains(&property) || (FRAMEWORK_MOCKS.contains(&property) && is_test_framework_object(ctx, object)) + }) + }) +} diff --git a/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs b/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs new file mode 100644 index 0000000..c32efb6 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs @@ -0,0 +1,131 @@ +//! `anti-slop/no-hookless-use-function`: a `use*` function calls a hook. The +//! React Compiler only compiles functions that do, so a hookless `useX` is +//! skipped and rebuilds what it returns on every render. Adapted from +//! t3code's `t3code/no-hookless-use-function`, after eslint-react's +//! `no-unnecessary-use-prefix`. +//! +//! Vue and other frameworks name plain composables `useX` too, so the rule +//! only reads React files: `.jsx` and `.tsx` files, and files importing +//! `react`. A `.vue` file never is one. + +use std::ffi::OsStr; +use std::path::Path; + +use oxc_ast::AstKind; +use oxc_ast::ast::{ArrowFunctionExpression, BindingPattern, CallExpression, Expression, Function}; +use oxc_ast_visit::{Visit, walk}; +use oxc_semantic::{AstNode, Semantic}; +use oxc_syntax::scope::ScopeFlags; + +use super::super::{Context, Rule}; +use super::shared::strip_parens; + +pub const NAME: &str = "anti-slop/no-hookless-use-function"; + +pub struct NoHooklessUseFunction; + +impl Rule for NoHooklessUseFunction { + fn name(&self) -> &'static str { + NAME + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { + let (name, span, calls_hook) = match node.kind() { + AstKind::Function(function) if function.is_declaration() => { + let Some(id) = &function.id else { + return; + }; + + (id.name.as_str(), id.span, function.body.as_deref().is_some_and(|body| HookCall::find(|finder| finder.visit_function_body(body)))) + } + AstKind::VariableDeclarator(declarator) => { + let (BindingPattern::BindingIdentifier(id), Some(init)) = (&declarator.id, &declarator.init) else { + return; + }; + + let calls_hook = match init { + Expression::ArrowFunctionExpression(arrow) => HookCall::find(|finder| finder.visit_arrow_function_body(&arrow.body)), + Expression::FunctionExpression(function) => { + function.body.as_deref().is_some_and(|body| HookCall::find(|finder| finder.visit_function_body(body))) + } + _ => return, + }; + + (id.name.as_str(), id.span, calls_hook) + } + _ => return, + }; + + if is_hook_name(name) && !calls_hook && is_react_file(ctx) { + ctx.report( + span, + format!( + "`{name}` calls no hooks, so the React Compiler skips it and whatever it returns is rebuilt on every render. Rename it to a plain function, or call the hook it depends on inside it." + ), + ); + } + } +} + +/// Whether the file is React code; see the module docs. +fn is_react_file(ctx: &mut Context<'_, '_>) -> bool { + match Path::new(ctx.rel).extension().and_then(OsStr::to_str) { + Some("vue") => false, + Some("jsx" | "tsx") => true, + _ => ctx.shared(ImportsReact::detect).0, + } +} + +/// Whether the file imports `react` or one of its subpaths. +struct ImportsReact(bool); + +impl ImportsReact { + fn detect(semantic: &Semantic<'_>) -> Self { + Self(semantic.nodes().iter().any( + |node| matches!(node.kind(), AstKind::ImportDeclaration(import) if import.source.value == "react" || import.source.value.starts_with("react/")), + )) + } +} + +/// `useX` as a hook name: `use` then an uppercase letter. +fn is_hook_name(name: &str) -> bool { + name.strip_prefix("use").is_some_and(|rest| rest.starts_with(|c: char| c.is_ascii_uppercase())) +} + +/// Whether `callee` calls a hook: `useX(…)`, React 19's bare `use(…)`, or `x.useX(…)`. +fn is_hook_callee(callee: &Expression<'_>) -> bool { + let name = match strip_parens(callee) { + Expression::Identifier(identifier) => identifier.name.as_str(), + Expression::StaticMemberExpression(member) => member.property.name.as_str(), + _ => return false, + }; + + name == "use" || is_hook_name(name) +} + +/// Finds a hook call outside nested functions: a hook called in a callback +/// runs when the callback does, not while rendering. +struct HookCall(bool); + +impl HookCall { + fn find(visit: impl FnOnce(&mut Self)) -> bool { + let mut finder = Self(false); + + visit(&mut finder); + finder.0 + } +} + +impl<'a> Visit<'a> for HookCall { + fn visit_call_expression(&mut self, call: &CallExpression<'a>) { + if is_hook_callee(&call.callee) { + self.0 = true; + } else { + walk::walk_call_expression(self, call); + } + } + + fn visit_function(&mut self, _: &Function<'a>, _: ScopeFlags) {} + + fn visit_arrow_function_expression(&mut self, _: &ArrowFunctionExpression<'a>) {} +} diff --git a/crates/lint/src/rules/anti_slop/no_low_value_comments.rs b/crates/lint/src/rules/anti_slop/no_low_value_comments.rs new file mode 100644 index 0000000..0f4de74 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_low_value_comments.rs @@ -0,0 +1,251 @@ +//! `anti-slop/no-low-value-comments`: no comment that only repeats the code, +//! divides a file into sections, labels test steps, or keeps dead code. +//! Adapted from t3code's `t3code/no-low-value-comments`. + +use std::sync::LazyLock; + +use oxc_ast::ast::Comment; +use oxc_span::Span; +use regress::Regex; + +use super::super::{Context, Rule}; +use super::shared::{comment_value, links_issue}; + +pub const NAME: &str = "anti-slop/no-low-value-comments"; + +fn regex(pattern: &str, flags: &str) -> Regex { + Regex::with_flags(pattern, flags).expect("a valid pattern") +} + +/// Comments that instruct a tool rather than a reader. +static DIRECTIVE: LazyLock = LazyLock::new(|| { + regex( + r"^\s*(?:!|(?:oxlint|eslint|tslint|deno-lint)-|@ts-|@effect-diagnostics|prettier-ignore|biome-ignore|c8 |v8 |istanbul |#(?:end)?region|[@#]__(?:PURE|NO_SIDE_EFFECTS)__|@vite-ignore|webpack[A-Z]|@jsx|@license|@preserve|@vitest-environment|@jest-environment|\/?\s* = LazyLock::new(|| regex(r"^(?:[-=*#~_/+]{3,}.*|.*[-=*#~_]{3,})$", "u")); +static TEST_STEP: LazyLock = LazyLock::new(|| regex(r"^(?:arrange|act|assert|given|when|then)\s*[:.]?$", "iu")); +static TASK_MARKER: LazyLock = LazyLock::new(|| regex(r"\b(?:TODO|FIXME|XXX|HACK)\b", "u")); +static JSDOC_TYPE: LazyLock = LazyLock::new(|| regex(r"^@(?:param|returns?|type|typedef)\s+\{", "u")); +static EMPTY_JSDOC_TAG: LazyLock = LazyLock::new(|| regex(r"^@(?:param(?:\s+\[?[\w$.]+\]?)?|returns?)$", "u")); +static COMMENTED_CODE: LazyLock = LazyLock::new(|| { + regex( + r#"^(?:(?:(?:const|let|var)\s+[\w${}[\], ]+=.*|import\s.+\sfrom\s+["'].+["']|(?:await\s+|return\s+)?[\w$.]+\(.*\)|[\w$.[\]]+\s*=\s*.+);|(?:if|for|while|switch)\s*\(.*\)\s*\{|\}(?:\s*else\s*\{)?)$"#, + "u", + ) +}); +static DECLARATION: LazyLock = LazyLock::new(|| { + regex( + r"^\s*(?:export\s+)?(?:default\s+)?(?:declare\s+)?(?:async\s+)?(?:static\s+)?(?:readonly\s+)?(?:(?:const|let|var|function\*?|class|interface|type|enum)\s+)?([A-Za-z_$][\w$]*)\s*[?!]?\s*[:=(<{]", + "u", + ) +}); + +/// Words that carry no meaning of their own when a comment is compared with a name. +const FILLER_WORDS: [&str; 30] = [ + "a", "an", "the", "of", "for", "to", "in", "on", "at", "by", "with", "from", "and", "or", "is", "are", "be", "this", "that", "it", "its", "as", "get", + "set", "return", "create", "new", "current", "value", "given", +]; + +/// Statement keywords the declaration pattern would otherwise read as a name. +const KEYWORDS: [&str; 6] = ["catch", "for", "if", "return", "switch", "while"]; + +pub struct NoLowValueComments; + +impl Rule for NoLowValueComments { + fn name(&self) -> &'static str { + NAME + } + + fn run_once(&self, ctx: &mut Context<'_, '_>) { + let semantic = ctx.semantic; + let source = semantic.source_text(); + let lines = Lines::new(source); + let typescript = semantic.source_type().is_typescript(); + let groups = groups(source, &lines, semantic.comments()); + + for group in groups { + let text: Vec<&str> = group.iter().map(|comment| comment_value(source, comment)).collect(); + let Some(message) = problem(source, &lines, typescript, &group, &text) else { + continue; + }; + + ctx.report(Span::new(group[0].span.start, group[group.len() - 1].span.end), message); + } + } +} + +/// Line starts, for line numbers and columns by byte offset. +struct Lines(Vec); + +impl Lines { + fn new(source: &str) -> Self { + Self(std::iter::once(0).chain(source.match_indices('\n').map(|(at, _)| at + 1)).collect()) + } + + /// The zero-based line holding `offset`. + fn line(&self, offset: u32) -> usize { + self.0.partition_point(|&start| start <= offset as usize) - 1 + } + + fn column(&self, offset: u32) -> usize { + offset as usize - self.0[self.line(offset)] + } + + /// The text of zero-based line `line`, without its line break. + fn text<'s>(&self, source: &'s str, line: usize) -> Option<&'s str> { + let start = *self.0.get(line)?; + let end = self.0.get(line + 1).map_or(source.len(), |next| next - 1); + + Some(source[start..end].trim_end_matches('\r')) + } +} + +/// Comments as a reader sees them: consecutive `//` lines at one indent are one comment. +fn groups<'c>(source: &str, lines: &Lines, comments: &'c [Comment]) -> Vec> { + let mut groups: Vec> = Vec::new(); + + for comment in comments { + let continues = groups.last().and_then(|group| group.last()).is_some_and(|previous| { + comment.is_line() + && previous.is_line() + && lines.line(previous.span.end) + 1 == lines.line(comment.span.start) + && lines.column(previous.span.start) == lines.column(comment.span.start) + && source[previous.span.end as usize..comment.span.start as usize].trim().is_empty() + }); + + match groups.last_mut() { + Some(group) if continues => group.push(comment), + _ => groups.push(vec![comment]), + } + } + + groups +} + +/// The comment's lines without their `*` gutter, blank ones dropped. +fn content_lines<'s>(text: &[&'s str]) -> Vec<&'s str> { + text.iter() + .flat_map(|value| value.split('\n')) + .map(|line| { + let line = line.trim_start(); + let line = line.strip_prefix('*').unwrap_or(line); + + line.strip_prefix([' ', '\t']).unwrap_or(line).trim() + }) + .filter(|line| !line.is_empty()) + .collect() +} + +fn matches(regex: &Regex, text: &str) -> bool { + regex.find(text).is_some() +} + +/// What is wrong with the comment group, if anything. +fn problem(source: &str, lines: &Lines, typescript: bool, group: &[&Comment], text: &[&str]) -> Option<&'static str> { + let content = content_lines(text); + + if content.is_empty() || text.iter().any(|value| matches(&DIRECTIVE, value)) { + return None; + } + + let is_line = group[0].is_line(); + + if content.iter().any(|line| matches(&DIVIDER_LINE, line)) { + return Some("Drop the section divider; split the file or let the code's structure show the grouping."); + } + + if is_line && content.len() == 1 && matches(&TEST_STEP, content[0]) { + return Some("Drop the test step label."); + } + + let joined = text.join("\n"); + + if matches(&TASK_MARKER, &joined) && !links_issue(&joined) { + return Some("Link the issue that tracks this, or drop the marker."); + } + + if typescript && content.iter().any(|line| matches(&JSDOC_TYPE, line)) { + return Some("Drop the JSDoc type; TypeScript already has it."); + } + + if content.iter().any(|line| matches(&EMPTY_JSDOC_TAG, line)) { + return Some("Drop the JSDoc tag that has no description."); + } + + if is_line && content.iter().all(|line| matches(&COMMENTED_CODE, line)) { + return Some("Delete commented-out code; git keeps the history."); + } + + if restates_next_name(source, lines, group, &content) { + return Some("Drop the comment; it only repeats the name below it. Say why, or say nothing."); + } + + None +} + +/// Whether an own-line comment says nothing but the name declared on the next line. +fn restates_next_name(source: &str, lines: &Lines, group: &[&Comment], content: &[&str]) -> bool { + let first = group[0].span.start; + let own_line = source[lines.0[lines.line(first)]..first as usize].trim().is_empty(); + + if !own_line || content.iter().any(|line| line.starts_with('@')) { + return false; + } + + let Some(next) = lines.text(source, lines.line(group[group.len() - 1].span.end) + 1) else { + return false; + }; + + let Some(name) = DECLARATION.find(next).and_then(|found| found.group(1)).map(|range| &next[range]) else { + return false; + }; + + if KEYWORDS.contains(&name) { + return false; + } + + let said = words(&content.join(" ")); + let named = words(&split_camel_case(name)); + + (1..=6).contains(&said.len()) && said.iter().all(|word| named.contains(word)) +} + +/// The meaningful words of `text`, lowercased and with a plural `s` dropped. +fn words(text: &str) -> Vec { + text.to_lowercase() + .split(|c: char| !c.is_ascii_lowercase() && !c.is_ascii_digit()) + .filter(|word| !word.is_empty() && !FILLER_WORDS.contains(word)) + .map(stem) + .filter(|word| !FILLER_WORDS.contains(&word.as_str())) + .collect() +} + +fn stem(word: &str) -> String { + match word.strip_suffix('s') { + Some(rest) if word.len() > 3 && !rest.ends_with('s') => rest.to_owned(), + _ => word.to_owned(), + } +} + +/// `parseHTMLDocument` as `parse HTML Document`. +fn split_camel_case(name: &str) -> String { + let chars: Vec = name.chars().collect(); + let mut out = String::with_capacity(name.len() + 4); + + for (at, &c) in chars.iter().enumerate() { + if at > 0 && c.is_ascii_uppercase() { + let previous = chars[at - 1]; + let next_is_lower = chars.get(at + 1).is_some_and(char::is_ascii_lowercase); + + if previous.is_ascii_lowercase() || previous.is_ascii_digit() || (previous.is_ascii_uppercase() && next_is_lower) { + out.push(' '); + } + } + + out.push(c); + } + + out +} diff --git a/crates/lint/src/rules/anti_slop/no_module_mocking.rs b/crates/lint/src/rules/anti_slop/no_module_mocking.rs index d5eca5d..d92ac13 100644 --- a/crates/lint/src/rules/anti_slop/no_module_mocking.rs +++ b/crates/lint/src/rules/anti_slop/no_module_mocking.rs @@ -2,11 +2,11 @@ //! replace dependencies through real interfaces. use oxc_ast::AstKind; -use oxc_ast::ast::Expression; use oxc_semantic::AstNode; use super::super::{Context, Rule}; -use super::shared::{member, strip_parens}; +use super::shared::member; +use super::shared::testing::is_test_framework_object; pub const NAME: &str = "anti-slop/no-module-mocking"; @@ -29,29 +29,3 @@ impl Rule for NoModuleMocking { } } } - -/// Whether `expression` names the global `vi` or `jest`, or `vi` imported from -/// `vitest` or `jest` from `@jest/globals` under any local name. -fn is_test_framework_object(ctx: &Context<'_, '_>, expression: &Expression<'_>) -> bool { - let Expression::Identifier(identifier) = strip_parens(expression) else { - return false; - }; - - let nodes = ctx.semantic.nodes(); - let scoping = ctx.semantic.scoping(); - let Some(symbol) = scoping.find_binding(nodes.get_node(identifier.node_id.get()).scope_id(), identifier.name) else { - return identifier.name == "vi" || identifier.name == "jest"; - }; - - let declaration = scoping.symbol_declaration(symbol); - let AstKind::ImportSpecifier(specifier) = nodes.kind(declaration) else { - return false; - }; - - let imported = specifier.imported.name(); - - nodes - .ancestor_kinds(declaration) - .find_map(|kind| if let AstKind::ImportDeclaration(import) = kind { Some(import.source.value.as_str()) } else { None }) - .is_some_and(|source| (source == "vitest" && imported == "vi") || (source == "@jest/globals" && imported == "jest")) -} diff --git a/crates/lint/src/rules/anti_slop/no_polling_loop_in_tests.rs b/crates/lint/src/rules/anti_slop/no_polling_loop_in_tests.rs new file mode 100644 index 0000000..234be96 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_polling_loop_in_tests.rs @@ -0,0 +1,183 @@ +//! `anti-slop/no-polling-loop-in-tests`: no test spins on a tick or a sleep +//! until something becomes true. How many turns that takes depends on the +//! scheduler and the runner's load, so the test is slow at best and flaky at +//! worst. Adapted from the plain-JavaScript half of t3code's +//! `t3code/no-polling-loop-in-tests`. +//! +//! A loop polls when it waits on every turn and a predicate decides when it +//! stops: an `if` that leaves the loop, or a test that is neither `true` nor a +//! counter the loop advances. A fixed number of flushes is not polling. + +use oxc_ast::AstKind; +use oxc_ast::ast::{ + ArrowFunctionExpression, AssignmentExpression, AssignmentOperator, AssignmentTarget, AwaitExpression, BinaryOperator, BreakStatement, CallExpression, + Expression, Function, IfStatement, ReturnStatement, SimpleAssignmentTarget, Statement, ThrowStatement, UpdateExpression, +}; +use oxc_ast_visit::{Visit, walk}; +use oxc_semantic::AstNode; +use oxc_span::GetSpan; +use oxc_syntax::scope::ScopeFlags; + +use super::super::{Context, Rule}; +use super::shared::strip_parens; +use super::shared::testing::is_test_file; +use super::shared::timers::PromiseTimers; + +pub const NAME: &str = "anti-slop/no-polling-loop-in-tests"; + +const MESSAGE: &str = "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`."; + +pub struct NoPollingLoopInTests; + +impl Rule for NoPollingLoopInTests { + fn name(&self) -> &'static str { + NAME + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { + let (test, body, update) = match node.kind() { + AstKind::ForStatement(it) => (it.test.as_ref(), &it.body, it.update.as_ref()), + AstKind::WhileStatement(it) => (Some(&it.test), &it.body, None), + AstKind::DoWhileStatement(it) => (Some(&it.test), &it.body, None), + _ => return, + }; + + if !is_test_file(ctx.rel) { + return; + } + + let timers = ctx.shared(PromiseTimers::collect); + let mut scan = Scan::new(&timers, None); + + scan.visit_statement(body); + + if !scan.waits { + return; + } + + if scan.exits_on_condition || test.is_some_and(|test| !is_true(test) && !is_counter_test(&timers, test, body, update)) { + ctx.report(node.kind().span(), MESSAGE); + } + } +} + +fn is_true(test: &Expression<'_>) -> bool { + matches!(strip_parens(test), Expression::BooleanLiteral(literal) if literal.value) +} + +/// A bare counter (`i < 3`, `attempt < max`) the loop itself advances bounds +/// the loop without asking anything of the code under test. `events.length < 1`, +/// or a counter nothing advances, is a predicate. +fn is_counter_test(timers: &PromiseTimers, test: &Expression<'_>, body: &Statement<'_>, update: Option<&Expression<'_>>) -> bool { + let Expression::BinaryExpression(binary) = strip_parens(test) else { + return false; + }; + + if !matches!(binary.operator, BinaryOperator::LessThan | BinaryOperator::LessEqualThan | BinaryOperator::GreaterThan | BinaryOperator::GreaterEqualThan) { + return false; + } + + [(&binary.left, &binary.right), (&binary.right, &binary.left)].into_iter().any(|(counter, bound)| { + let Expression::Identifier(counter) = strip_parens(counter) else { + return false; + }; + + if !matches!(strip_parens(bound), Expression::Identifier(_) | Expression::NumericLiteral(_)) { + return false; + } + + let mut scan = Scan::new(timers, Some(counter.name.as_str())); + + scan.visit_statement(body); + + if let Some(update) = update { + scan.visit_expression(update); + } + + scan.advances_counter + }) +} + +/// One pass over a loop body that skips nested functions: a wait or an exit in +/// a callback runs on the callback's schedule, not the loop's. +struct Scan<'t> { + timers: &'t PromiseTimers, + counter: Option<&'t str>, + waits: bool, + exits_on_condition: bool, + advances_counter: bool, + /// How many `if` branches enclose the current node. + in_branch: usize, +} + +impl<'t> Scan<'t> { + fn new(timers: &'t PromiseTimers, counter: Option<&'t str>) -> Self { + Self { timers, counter, waits: false, exits_on_condition: false, advances_counter: false, in_branch: 0 } + } + + fn is_counter(&self, name: &str) -> bool { + self.counter == Some(name) + } +} + +impl<'a> Visit<'a> for Scan<'_> { + fn visit_await_expression(&mut self, it: &AwaitExpression<'a>) { + self.waits |= self.timers.is_tick_promise(&it.argument); + walk::walk_await_expression(self, it); + } + + fn visit_call_expression(&mut self, it: &CallExpression<'a>) { + // `setImmediate(…)` on its own yields a turn, the callback-style tick. + self.waits |= self.timers.is_callback_timer(&it.callee, &["setImmediate"]); + walk::walk_call_expression(self, it); + } + + fn visit_if_statement(&mut self, it: &IfStatement<'a>) { + self.visit_expression(&it.test); + self.in_branch += 1; + self.visit_statement(&it.consequent); + + if let Some(alternate) = &it.alternate { + self.visit_statement(alternate); + } + + self.in_branch -= 1; + } + + fn visit_return_statement(&mut self, it: &ReturnStatement<'a>) { + self.exits_on_condition |= self.in_branch > 0; + walk::walk_return_statement(self, it); + } + + fn visit_break_statement(&mut self, it: &BreakStatement<'a>) { + self.exits_on_condition |= self.in_branch > 0; + walk::walk_break_statement(self, it); + } + + fn visit_throw_statement(&mut self, it: &ThrowStatement<'a>) { + self.exits_on_condition |= self.in_branch > 0; + walk::walk_throw_statement(self, it); + } + + fn visit_update_expression(&mut self, it: &UpdateExpression<'a>) { + if let SimpleAssignmentTarget::AssignmentTargetIdentifier(identifier) = &it.argument { + self.advances_counter |= self.is_counter(&identifier.name); + } + + walk::walk_update_expression(self, it); + } + + fn visit_assignment_expression(&mut self, it: &AssignmentExpression<'a>) { + if let AssignmentTarget::AssignmentTargetIdentifier(identifier) = &it.left + && matches!(it.operator, AssignmentOperator::Addition | AssignmentOperator::Subtraction) + { + self.advances_counter |= self.is_counter(&identifier.name); + } + + walk::walk_assignment_expression(self, it); + } + + fn visit_function(&mut self, _: &Function<'a>, _: ScopeFlags) {} + + fn visit_arrow_function_expression(&mut self, _: &ArrowFunctionExpression<'a>) {} +} diff --git a/crates/lint/src/rules/anti_slop/no_real_time_wait_in_tests.rs b/crates/lint/src/rules/anti_slop/no_real_time_wait_in_tests.rs new file mode 100644 index 0000000..6752ae2 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_real_time_wait_in_tests.rs @@ -0,0 +1,64 @@ +//! `anti-slop/no-real-time-wait-in-tests`: no test waits on the real clock. +//! A sleep long enough on a fast machine is too short on a loaded runner, and +//! every sleep is time the suite spends doing nothing. Adapted from the +//! plain-JavaScript half of t3code's `t3code/no-real-time-wait-in-tests`. +//! +//! A file that turns on fake timers is left alone: its timers fire when the +//! test advances them, not when the clock does. + +use oxc_ast::AstKind; +use oxc_ast::ast::Expression; +use oxc_semantic::{AstNode, Semantic}; +use oxc_span::GetSpan; + +use super::super::{Context, Rule}; +use super::shared::member; +use super::shared::testing::is_test_file; +use super::shared::timers::{PromiseTimers, TIMER_FUNCTIONS, is_inside_promise_executor}; + +pub const NAME: &str = "anti-slop/no-real-time-wait-in-tests"; + +const MESSAGE: &str = "Real-clock wait in a test: it passes or fails with runner load. Await the promise, event, or callback that marks the milestone, or turn on fake timers and advance them."; + +pub struct NoRealTimeWaitInTests; + +impl Rule for NoRealTimeWaitInTests { + fn name(&self) -> &'static str { + NAME + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { + let AstKind::CallExpression(call) = node.kind() else { + return; + }; + + if !is_test_file(ctx.rel) { + return; + } + + let timers = ctx.shared(PromiseTimers::collect); + let waits = timers.is_call(&call.callee) + || (timers.is_callback_timer(&call.callee, &TIMER_FUNCTIONS) && is_inside_promise_executor(ctx.semantic.nodes(), node.id())); + + if waits && !ctx.shared(FakeTimers::detect).0 { + ctx.report(call.callee.span(), MESSAGE); + } + } +} + +/// Whether the file calls `vi.useFakeTimers()` or `jest.useFakeTimers()`. +struct FakeTimers(bool); + +impl FakeTimers { + fn detect(semantic: &Semantic<'_>) -> Self { + Self(semantic.nodes().iter().any(|node| { + let AstKind::CallExpression(call) = node.kind() else { + return false; + }; + + member(&call.callee).is_some_and(|(object, property)| { + property == "useFakeTimers" && matches!(object, Expression::Identifier(object) if object.name == "vi" || object.name == "jest") + }) + })) + } +} diff --git a/crates/lint/src/rules/anti_slop/no_test_in_loop.rs b/crates/lint/src/rules/anti_slop/no_test_in_loop.rs new file mode 100644 index 0000000..c2e1cea --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_test_in_loop.rs @@ -0,0 +1,73 @@ +//! `anti-slop/no-test-in-loop`: no test registered inside a `for` loop; the +//! cases go through `.each`, which names each one and runs it on its own. +//! Adapted from t3code's `t3code/no-test-in-loop`. + +use oxc_ast::AstKind; +use oxc_ast::ast::Expression; +use oxc_semantic::AstNode; +use oxc_span::GetSpan; + +use super::super::{Context, Rule}; +use super::shared::testing::{TEST_FUNCTIONS, is_test_file}; +use super::shared::{ancestors, strip_parens}; + +pub const NAME: &str = "anti-slop/no-test-in-loop"; + +/// The modifiers between a test function and its call that also have `.each`. +const EACH_MODIFIERS: [&str; 6] = ["concurrent", "effect", "live", "only", "sequential", "skip"]; + +pub struct NoTestInLoop; + +impl Rule for NoTestInLoop { + fn name(&self) -> &'static str { + NAME + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { + let AstKind::CallExpression(call) = node.kind() else { + return; + }; + + if !is_test_file(ctx.rel) { + return; + } + + let Some(name) = test_call_name(&call.callee) else { + return; + }; + + if is_inside_loop(ctx, node) && !uses_node_test(ctx.source()) { + ctx.report(call.callee.span(), format!("Do not call `{name}(…)` inside a for loop. Use `{name}.each(cases)(name, …)` instead.")); + } + } +} + +/// `node:test` has no `.each`, so files written against it keep their loops. +fn uses_node_test(source: &str) -> bool { + source.contains("\"node:test\"") || source.contains("'node:test'") +} + +/// `it` or `it.skip` as written, when `callee` registers a test. +fn test_call_name(callee: &Expression<'_>) -> Option { + match strip_parens(callee) { + Expression::Identifier(identifier) if TEST_FUNCTIONS.contains(&identifier.name.as_str()) => Some(identifier.name.to_string()), + Expression::StaticMemberExpression(member) if EACH_MODIFIERS.contains(&member.property.name.as_str()) => match strip_parens(&member.object) { + Expression::Identifier(object) if TEST_FUNCTIONS.contains(&object.name.as_str()) => Some(format!("{}.{}", object.name, member.property.name)), + _ => None, + }, + _ => None, + } +} + +/// Whether a `for` loop encloses `node` within its own function. +fn is_inside_loop(ctx: &Context<'_, '_>, node: &AstNode<'_>) -> bool { + for ancestor in ancestors(ctx.semantic.nodes(), node.id()) { + match ancestor.kind() { + AstKind::ForStatement(_) | AstKind::ForInStatement(_) | AstKind::ForOfStatement(_) => return true, + AstKind::Function(_) | AstKind::ArrowFunctionExpression(_) => return false, + _ => {} + } + } + + false +} diff --git a/crates/lint/src/rules/anti_slop/no_unreasoned_test_skip.rs b/crates/lint/src/rules/anti_slop/no_unreasoned_test_skip.rs new file mode 100644 index 0000000..ca672a1 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/no_unreasoned_test_skip.rs @@ -0,0 +1,93 @@ +//! `anti-slop/no-unreasoned-test-skip`: a skipped or to-do test links the +//! issue that tracks it in a comment above, so it does not stay skipped +//! forever. Adapted from the skip half of t3code's +//! `t3code/no-test-timeout-or-unreasoned-skip`. + +use oxc_ast::AstKind; +use oxc_ast::ast::{Argument, Expression, ObjectPropertyKind}; +use oxc_semantic::AstNode; +use oxc_span::{GetSpan, Span}; + +use super::super::{Context, Rule}; +use super::shared::testing::{is_test_chain, is_test_file}; +use super::shared::{ancestors, comment_value, comments_before, links_issue, strip_parens}; + +pub const NAME: &str = "anti-slop/no-unreasoned-test-skip"; + +const MESSAGE: &str = "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it."; + +/// The modifiers that keep a test from running: `it.skip`, `it.todo`. +const SKIP_MODIFIERS: [&str; 2] = ["skip", "todo"]; + +/// The Jasmine-style names for a skipped test or suite. +const SKIP_FUNCTIONS: [&str; 3] = ["xdescribe", "xit", "xtest"]; + +pub struct NoUnreasonedTestSkip; + +impl Rule for NoUnreasonedTestSkip { + fn name(&self) -> &'static str { + NAME + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { + if !is_test_file(ctx.rel) { + return; + } + + let span = match node.kind() { + AstKind::CallExpression(call) => match strip_parens(&call.callee) { + Expression::Identifier(identifier) if SKIP_FUNCTIONS.contains(&identifier.name.as_str()) => identifier.span, + // `test(name, { skip: true }, fn)` is the options spelling of `test.skip`. A computed + // value such as `{ skip: isWindows }` is a platform gate, like `skipIf`, and stays allowed. + callee if is_test_chain(callee) && call.arguments.iter().skip(1).take(2).any(skips_by_option) => call.span, + _ => return, + }, + AstKind::StaticMemberExpression(member) if SKIP_MODIFIERS.contains(&member.property.name.as_str()) && is_test_chain(&member.object) => member.span, + // `it["skip"](…)` is the same registration; only a dynamic key is unknowable here. + AstKind::ComputedMemberExpression(member) + if matches!(strip_parens(&member.expression), Expression::StringLiteral(key) if SKIP_MODIFIERS.contains(&key.value.as_str())) + && is_test_chain(&member.object) => + { + member.span + } + _ => return, + }; + + if !is_tracked(ctx, node) { + ctx.report(span, MESSAGE); + } + } +} + +/// Whether `argument` is an options object with `skip: true` or `todo: true`. +fn skips_by_option(argument: &Argument<'_>) -> bool { + let Some(Expression::ObjectExpression(object)) = argument.as_expression().map(strip_parens) else { + return false; + }; + + object.properties.iter().any(|property| { + let ObjectPropertyKind::ObjectProperty(property) = property else { + return false; + }; + + property.key.static_name().is_some_and(|key| SKIP_MODIFIERS.contains(&key.as_ref())) + && matches!(strip_parens(&property.value), Expression::BooleanLiteral(value) if value.value) + }) +} + +/// Whether a comment directly above the statement holding `node` links an issue. +fn is_tracked(ctx: &Context<'_, '_>, node: &AstNode<'_>) -> bool { + let statement = std::iter::once(node).chain(ancestors(ctx.semantic.nodes(), node.id())).find_map(|node| statement_span(node.kind())); + let Some(statement) = statement else { + return false; + }; + + let source = ctx.source(); + + comments_before(ctx.semantic, statement.start).iter().any(|comment| links_issue(comment_value(source, comment))) +} + +/// The span of `kind` when it is a statement or a declaration. +fn statement_span(kind: AstKind<'_>) -> Option { + (kind.is_statement() || kind.is_declaration()).then(|| kind.span()) +} diff --git a/crates/lint/src/rules/anti_slop/shared/mod.rs b/crates/lint/src/rules/anti_slop/shared/mod.rs index 0e894ed..ef65603 100644 --- a/crates/lint/src/rules/anti_slop/shared/mod.rs +++ b/crates/lint/src/rules/anti_slop/shared/mod.rs @@ -6,6 +6,8 @@ pub mod lexical; pub mod params; +pub mod testing; +pub mod timers; pub mod types; use oxc_ast::AstKind; @@ -140,3 +142,8 @@ pub fn comment_value<'a>(source: &'a str, comment: &Comment) -> &'a str { &source[span.start as usize..span.end as usize] } + +/// Whether `text` holds an issue reference: `#123` or an `http(s)://` link. +pub fn links_issue(text: &str) -> bool { + text.contains("http://") || text.contains("https://") || text.match_indices('#').any(|(at, _)| text[at + 1..].starts_with(|c: char| c.is_ascii_digit())) +} diff --git a/crates/lint/src/rules/anti_slop/shared/testing.rs b/crates/lint/src/rules/anti_slop/shared/testing.rs new file mode 100644 index 0000000..d5e0f91 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/shared/testing.rs @@ -0,0 +1,70 @@ +//! What the test rules share: which files are tests, which names register a +//! test, and which objects are the Vitest or Jest test utilities. + +use std::ffi::OsStr; +use std::path::Path; + +use oxc_ast::AstKind; +use oxc_ast::ast::Expression; + +use super::super::super::Context; +use super::strip_parens; + +/// The functions that register a test or a suite. +pub const TEST_FUNCTIONS: [&str; 4] = ["describe", "it", "suite", "test"]; + +/// The script extensions a test file can have, as `[cm]?[jt]sx?`. +const SCRIPT_EXTENSIONS: [&str; 12] = ["js", "jsx", "ts", "tsx", "cjs", "cjsx", "cts", "ctsx", "mjs", "mjsx", "mts", "mtsx"]; + +/// Whether `rel` names a test file: `*.test.` or `*.spec.`. +pub fn is_test_file(rel: &str) -> bool { + let path = Path::new(rel); + let extension = path.extension().and_then(OsStr::to_str); + let kind = path.file_stem().map(Path::new).and_then(Path::extension).and_then(OsStr::to_str); + + extension.is_some_and(|extension| SCRIPT_EXTENSIONS.contains(&extension)) && matches!(kind, Some("test" | "spec")) +} + +/// The identifier a callee chain starts from: `it` in `it.skip.each(cases)`. +pub fn chain_root<'a>(mut expression: &Expression<'a>) -> Option<&'a str> { + loop { + match strip_parens(expression) { + Expression::Identifier(identifier) => return Some(identifier.name.as_str()), + Expression::StaticMemberExpression(member) => expression = &member.object, + Expression::ComputedMemberExpression(member) => expression = &member.object, + Expression::CallExpression(call) => expression = &call.callee, + _ => return None, + } + } +} + +/// Whether `expression` is a chain that registers a test or a suite. +pub fn is_test_chain(expression: &Expression<'_>) -> bool { + chain_root(expression).is_some_and(|root| TEST_FUNCTIONS.contains(&root)) +} + +/// Whether `expression` names the global `vi` or `jest`, or `vi` imported from +/// `vitest` or `jest` from `@jest/globals` under any local name. +pub fn is_test_framework_object(ctx: &Context<'_, '_>, expression: &Expression<'_>) -> bool { + let Expression::Identifier(identifier) = strip_parens(expression) else { + return false; + }; + + let nodes = ctx.semantic.nodes(); + let scoping = ctx.semantic.scoping(); + let Some(symbol) = scoping.find_binding(nodes.get_node(identifier.node_id.get()).scope_id(), identifier.name) else { + return identifier.name == "vi" || identifier.name == "jest"; + }; + + let declaration = scoping.symbol_declaration(symbol); + let AstKind::ImportSpecifier(specifier) = nodes.kind(declaration) else { + return false; + }; + + let imported = specifier.imported.name(); + + nodes + .ancestor_kinds(declaration) + .find_map(|kind| if let AstKind::ImportDeclaration(import) = kind { Some(import.source.value.as_str()) } else { None }) + .is_some_and(|source| (source == "vitest" && imported == "vi") || (source == "@jest/globals" && imported == "jest")) +} diff --git a/crates/lint/src/rules/anti_slop/shared/timers.rs b/crates/lint/src/rules/anti_slop/shared/timers.rs new file mode 100644 index 0000000..ff21532 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/shared/timers.rs @@ -0,0 +1,141 @@ +//! Timers as the test-timing rules see them: the callback timers, the promise +//! timers of `node:timers/promises`, and the promises that only wait. + +use oxc_ast::AstKind; +use oxc_ast::ast::{Argument, ArrowFunctionExpression, CallExpression, Expression, Function, ImportDeclarationSpecifier}; +use oxc_ast_visit::{Visit, walk}; +use oxc_semantic::{AstNodes, NodeId, Semantic}; +use oxc_span::GetSpan; +use oxc_syntax::scope::ScopeFlags; +use rustc_hash::FxHashSet; + +use super::{ancestors, member, parent, strip_parens}; + +/// The timers that schedule a callback, or resolve a promise from `timers/promises`. +pub const TIMER_FUNCTIONS: [&str; 3] = ["setImmediate", "setInterval", "setTimeout"]; + +/// What the file imports from `timers/promises`. +#[derive(Default)] +pub struct PromiseTimers { + /// Local names of `setTimeout` and the others: `import { setTimeout as sleep }`. + locals: FxHashSet, + /// Local names of the module itself: `import * as timers`. + namespaces: FxHashSet, +} + +impl PromiseTimers { + pub fn collect(semantic: &Semantic<'_>) -> Self { + let mut timers = Self::default(); + + for node in semantic.nodes().iter() { + let AstKind::ImportDeclaration(import) = node.kind() else { + continue; + }; + + if !matches!(import.source.value.as_str(), "node:timers/promises" | "timers/promises") { + continue; + } + + for specifier in import.specifiers.iter().flatten() { + match specifier { + ImportDeclarationSpecifier::ImportSpecifier(specifier) if TIMER_FUNCTIONS.contains(&specifier.imported.name().as_str()) => { + timers.locals.insert(specifier.local.name.to_string()); + } + ImportDeclarationSpecifier::ImportNamespaceSpecifier(specifier) => { + timers.namespaces.insert(specifier.local.name.to_string()); + } + ImportDeclarationSpecifier::ImportDefaultSpecifier(specifier) => { + timers.namespaces.insert(specifier.local.name.to_string()); + } + ImportDeclarationSpecifier::ImportSpecifier(_) => {} + } + } + } + + timers + } + + /// Whether `callee` is a promise timer: `sleep(…)` or `timers.setTimeout(…)`. + pub fn is_call(&self, callee: &Expression<'_>) -> bool { + match strip_parens(callee) { + Expression::Identifier(identifier) => self.locals.contains(identifier.name.as_str()), + callee => member(callee).is_some_and(|(object, property)| { + TIMER_FUNCTIONS.contains(&property) + && matches!(strip_parens(object), Expression::Identifier(object) if self.namespaces.contains(object.name.as_str())) + }), + } + } + + /// Whether `callee` is a callback timer: `setTimeout`, not a promise timer of the same name. + pub fn is_callback_timer(&self, callee: &Expression<'_>, names: &[&str]) -> bool { + matches!(strip_parens(callee), Expression::Identifier(identifier) if names.contains(&identifier.name.as_str()) && !self.locals.contains(identifier.name.as_str())) + } + + /// Whether `expression` is a promise that only waits: `Promise.resolve()`, a + /// promise timer, or `new Promise` with an executor that starts a timer. + pub fn is_tick_promise(&self, expression: &Expression<'_>) -> bool { + const TICK_TIMERS: [&str; 2] = ["setImmediate", "setTimeout"]; + + match strip_parens(expression) { + Expression::CallExpression(call) => { + self.is_call(&call.callee) || member(&call.callee).is_some_and(|(object, property)| property == "resolve" && is_identifier(object, "Promise")) + } + Expression::NewExpression(new) if is_identifier(&new.callee, "Promise") => { + match new.arguments.first().and_then(Argument::as_expression).map(strip_parens) { + Some(Expression::ArrowFunctionExpression(executor)) => { + self.starts_timer(&TICK_TIMERS, |finder| finder.visit_arrow_function_body(&executor.body)) + } + Some(Expression::FunctionExpression(executor)) => { + executor.body.as_deref().is_some_and(|body| self.starts_timer(&TICK_TIMERS, |finder| finder.visit_function_body(body))) + } + Some(executor) => self.is_callback_timer(executor, &TICK_TIMERS), + None => false, + } + } + _ => false, + } + } + + /// Whether the body `visit` walks calls one of the callback timers `names` + /// outside its nested functions. + fn starts_timer(&self, names: &[&str], visit: impl FnOnce(&mut TimerCall<'_>)) -> bool { + let mut finder = TimerCall { timers: self, names, found: false }; + + visit(&mut finder); + finder.found + } +} + +struct TimerCall<'t> { + timers: &'t PromiseTimers, + names: &'t [&'t str], + found: bool, +} + +impl<'a> Visit<'a> for TimerCall<'_> { + fn visit_call_expression(&mut self, call: &CallExpression<'a>) { + self.found |= self.timers.is_callback_timer(&call.callee, self.names); + walk::walk_call_expression(self, call); + } + + fn visit_function(&mut self, _: &Function<'a>, _: ScopeFlags) {} + + fn visit_arrow_function_expression(&mut self, _: &ArrowFunctionExpression<'a>) {} +} + +fn is_identifier(expression: &Expression<'_>, name: &str) -> bool { + matches!(strip_parens(expression), Expression::Identifier(identifier) if identifier.name == name) +} + +/// Whether the nearest function around `id` is the executor of a `new Promise(…)`. +pub fn is_inside_promise_executor(nodes: &AstNodes<'_>, id: NodeId) -> bool { + let Some(function) = ancestors(nodes, id).find(|node| matches!(node.kind(), AstKind::Function(_) | AstKind::ArrowFunctionExpression(_))) else { + return false; + }; + + let AstKind::NewExpression(new) = parent(nodes, function.id()).kind() else { + return false; + }; + + is_identifier(&new.callee, "Promise") && new.arguments.first().is_some_and(|first| first.span() == function.kind().span()) +} diff --git a/crates/lint/src/rules/anti_slop/tests/mod.rs b/crates/lint/src/rules/anti_slop/tests/mod.rs index f410386..fe6e7f7 100644 --- a/crates/lint/src/rules/anti_slop/tests/mod.rs +++ b/crates/lint/src/rules/anti_slop/tests/mod.rs @@ -1,23 +1,30 @@ //! Table tests for the anti-slop rules. //! -//! Every table holds v1's own test cases plus edge cases, with the findings -//! the v1 plugin produced for them under oxlint 1.86: byte spans and exact -//! messages. Like v1's rule tester, each case runs one rule alone, and the -//! snippet's own `oxlint-disable` directives apply. +//! Every table for a v1 rule holds v1's own test cases plus edge cases, with +//! the findings the v1 plugin produced for them under oxlint 1.86: byte spans +//! and exact messages. The rules adapted from t3code have no recorded +//! findings; their tables are fmtkit's own. Like v1's rule tester, each case +//! runs one rule alone, and the snippet's own `oxlint-disable` directives apply. mod no_ambient_nondeterminism; mod no_chained_type_assertions; mod no_conditional_empty_object_spread; +mod no_hookless_use_function; mod no_known_value_widening; +mod no_low_value_comments; mod no_module_mocking; mod no_object_parameters; +mod no_polling_loop_in_tests; +mod no_real_time_wait_in_tests; mod no_reflect_apply; mod no_reflect_get; mod no_runtime_typeof; mod no_shape_in_symbol_names; +mod no_test_in_loop; mod no_unknown_parameters; mod no_unknown_returns; mod no_unknown_type_aliases; +mod no_unreasoned_test_skip; mod no_unsafe_dictionary_type; mod no_widen_then_assert; mod require_safety_comment_for_type_assertion; @@ -88,7 +95,7 @@ pub fn check(rule: &str, options: &str, cases: &[Case]) { } #[test] -fn every_v1_rule_is_registered() { +fn every_rule_is_registered() { let names: Vec<&str> = RULES.iter().map(|(name, _)| *name).collect(); assert_eq!( @@ -97,16 +104,22 @@ fn every_v1_rule_is_registered() { "anti-slop/no-ambient-nondeterminism", "anti-slop/no-chained-type-assertions", "anti-slop/no-conditional-empty-object-spread", + "anti-slop/no-hookless-use-function", "anti-slop/no-known-value-widening", + "anti-slop/no-low-value-comments", "anti-slop/no-module-mocking", "anti-slop/no-object-parameters", + "anti-slop/no-polling-loop-in-tests", + "anti-slop/no-real-time-wait-in-tests", "anti-slop/no-reflect-apply", "anti-slop/no-reflect-get", "anti-slop/no-runtime-typeof", "anti-slop/no-shape-in-symbol-names", + "anti-slop/no-test-in-loop", "anti-slop/no-unknown-parameters", "anti-slop/no-unknown-returns", "anti-slop/no-unknown-type-aliases", + "anti-slop/no-unreasoned-test-skip", "anti-slop/no-unsafe-dictionary-type", "anti-slop/no-widen-then-assert", "anti-slop/require-safety-comment-for-type-assertion", diff --git a/crates/lint/src/rules/anti_slop/tests/no_ambient_nondeterminism.rs b/crates/lint/src/rules/anti_slop/tests/no_ambient_nondeterminism.rs index d81b60c..e510e5c 100644 --- a/crates/lint/src/rules/anti_slop/tests/no_ambient_nondeterminism.rs +++ b/crates/lint/src/rules/anti_slop/tests/no_ambient_nondeterminism.rs @@ -191,3 +191,55 @@ fn edges() { ], ); } + +/// fmtkit's additions: readings that seed a mock are allowed, as in +/// t3code's `t3code/no-wall-clock-in-tests`, and `process.hrtime.bigint()` is a reading. +#[test] +fn additions() { + check( + "anti-slop/no-ambient-nondeterminism", + "[]", + &[ + Case { name: "seeds fake time", path: "case.test.ts", code: "import { vi } from \"vitest\";\n\nvi.setSystemTime(Date.now());\n", expected: &[] }, + Case { name: "inside a vi.fn implementation", path: "case.test.ts", code: "export const now = vi.fn(() => Date.now());\n", expected: &[] }, + Case { name: "returned by a mock", path: "case.test.ts", code: "clock.mockReturnValue(new Date());\n", expected: &[] }, + Case { + name: "inside a vi.mock factory", + path: "case.test.ts", + code: "vi.mock(\"./clock\", () => ({ now: () => performance.now() }));\n", + expected: &[], + }, + Case { + name: "shadowed vi is not the framework", + path: "case.test.ts", + code: "const vi = { fn: (f: T): T => f };\n\nexport const now = vi.fn(() => Date.now());\n", + expected: &[(70, 80, "`Date.now()` makes this output depend on when and where it ran. Take the value as a parameter so the caller owns it.")], + }, + Case { + name: "measured by the test", + path: "case.test.ts", + code: "it(\"is fast\", () => {\n\tconst start = Date.now();\n\n\trun();\n\texpect(Date.now() - start).toBeLessThan(5);\n});\n", + expected: &[ + (37, 47, "`Date.now()` makes this output depend on when and where it ran. Take the value as a parameter so the caller owns it."), + (66, 76, "`Date.now()` makes this output depend on when and where it ran. Take the value as a parameter so the caller owns it."), + ], + }, + Case { + name: "process.hrtime.bigint", + path: "case.ts", + code: "export const at = process.hrtime.bigint();\n", + expected: &[( + 18, + 41, + "`process.hrtime.bigint()` makes this output depend on when and where it ran. Take the value as a parameter so the caller owns it.", + )], + }, + Case { + name: "shadowed process", + path: "case.ts", + code: "const process = { hrtime: { bigint: (): bigint => 0n } };\n\nexport const at = process.hrtime.bigint();\n", + expected: &[], + }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs b/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs new file mode 100644 index 0000000..dd030bb --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs @@ -0,0 +1,65 @@ +//! `anti-slop/no-hookless-use-function`: adapted from t3code's `t3code/no-hookless-use-function`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-hookless-use-function", + "[]", + &[ + Case { + name: "hookless declaration in tsx", + path: "case.tsx", + code: "export function useTitle(name: string): string {\n\treturn name.toUpperCase();\n}\n", + expected: &[( + 16, + 24, + "`useTitle` calls no hooks, so the React Compiler skips it and whatever it returns is rebuilt on every render. Rename it to a plain function, or call the hook it depends on inside it.", + )], + }, + Case { + name: "calls useState", + path: "case.tsx", + code: "import { useState } from \"react\";\n\nexport function useCount(): number {\n\tconst [n] = useState(0);\n\n\treturn n;\n}\n", + expected: &[], + }, + Case { + name: "calls React.useState", + path: "case.tsx", + code: "export function useCount(): number {\n\treturn React.useState(0)[0];\n}\n", + expected: &[], + }, + Case { name: "calls use", path: "case.tsx", code: "export function useValue(p: Promise): number {\n\treturn use(p);\n}\n", expected: &[] }, + Case { + name: "hook only in a callback", + path: "case.tsx", + code: "export const useLater = () => {\n\treturn () => useContext(Theme);\n};\n", + expected: &[( + 13, + 21, + "`useLater` calls no hooks, so the React Compiler skips it and whatever it returns is rebuilt on every render. Rename it to a plain function, or call the hook it depends on inside it.", + )], + }, + Case { + name: "function expression in a file importing react", + path: "case.ts", + code: "import { memo } from \"react\";\n\nexport const useTitle = function (name: string): string {\n\treturn name;\n};\n\nexport const Title = memo(() => null);\n", + expected: &[( + 44, + 52, + "`useTitle` calls no hooks, so the React Compiler skips it and whatever it returns is rebuilt on every render. Rename it to a plain function, or call the hook it depends on inside it.", + )], + }, + Case { + name: "composable outside React", + path: "case.ts", + code: "export function useTitle(name: string): string {\n\treturn name;\n}\n", + expected: &[], + }, + Case { name: "not a hook name", path: "case.tsx", code: "export function user(): string {\n\treturn \"\";\n}\n", expected: &[] }, + Case { name: "bare use is not a hook name", path: "case.tsx", code: "export function use(): string {\n\treturn \"\";\n}\n", expected: &[] }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_low_value_comments.rs b/crates/lint/src/rules/anti_slop/tests/no_low_value_comments.rs new file mode 100644 index 0000000..d880aec --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_low_value_comments.rs @@ -0,0 +1,101 @@ +//! `anti-slop/no-low-value-comments`: adapted from t3code's `t3code/no-low-value-comments`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-low-value-comments", + "[]", + &[ + Case { + name: "section divider", + path: "case.ts", + code: "// ---------- helpers ----------\nexport const a = 1;\n", + expected: &[(0, 32, "Drop the section divider; split the file or let the code's structure show the grouping.")], + }, + Case { + name: "block divider", + path: "case.ts", + code: "/* ===================== */\nexport const a = 1;\n", + expected: &[(0, 27, "Drop the section divider; split the file or let the code's structure show the grouping.")], + }, + Case { + name: "test step label", + path: "case.test.ts", + code: "it(\"x\", () => {\n\t// Arrange\n\tconst a = 1;\n\n\texpect(a).toBe(1);\n});\n", + expected: &[(17, 27, "Drop the test step label.")], + }, + Case { + name: "unlinked TODO", + path: "case.ts", + code: "// TODO: handle errors\nexport const a = 1;\n", + expected: &[(0, 22, "Link the issue that tracks this, or drop the marker.")], + }, + Case { name: "linked TODO", path: "case.ts", code: "// TODO(#12): handle errors\nexport const a = 1;\n", expected: &[] }, + Case { + name: "JSDoc type in TypeScript", + path: "case.ts", + code: "/**\n * Greets someone by name.\n * @param {string} name Who to greet.\n */\nexport function greet(name: string): string {\n\treturn name;\n}\n", + expected: &[(0, 72, "Drop the JSDoc type; TypeScript already has it.")], + }, + Case { + name: "JSDoc type in JavaScript", + path: "case.js", + code: "/**\n * Greets someone by name.\n * @param {string} name Who to greet.\n */\nexport function greet(name) {\n\treturn name;\n}\n", + expected: &[], + }, + Case { + name: "empty JSDoc tag", + path: "case.ts", + code: "/**\n * Greets someone.\n * @returns\n */\nexport function greet(): string {\n\treturn \"hi\";\n}\n", + expected: &[(0, 38, "Drop the JSDoc tag that has no description.")], + }, + Case { + name: "commented-out code", + path: "case.ts", + code: "// const a = compute();\n// a.run();\nexport const b = 1;\n", + expected: &[(0, 35, "Delete commented-out code; git keeps the history.")], + }, + Case { + name: "prose that mentions code", + path: "case.ts", + code: "// compute() runs once per file, so cache it.\nexport const b = 1;\n", + expected: &[], + }, + Case { + name: "restates the name", + path: "case.ts", + code: "// Get the user name\nexport function getUserName(): string {\n\treturn \"\";\n}\n", + expected: &[(0, 20, "Drop the comment; it only repeats the name below it. Say why, or say nothing.")], + }, + Case { + name: "restates a JSDoc name", + path: "case.ts", + code: "/** The user names. */\nexport const userNames: Array = [];\n", + expected: &[(0, 22, "Drop the comment; it only repeats the name below it. Say why, or say nothing.")], + }, + Case { + name: "says why", + path: "case.ts", + code: "// Cached: the lookup hits the network.\nexport function getUserName(): string {\n\treturn \"\";\n}\n", + expected: &[], + }, + Case { name: "trailing comment", path: "case.ts", code: "export const a = 1; // the user name\nexport const userName = \"\";\n", expected: &[] }, + Case { name: "statement keyword", path: "case.ts", code: "// If\nif (a) {\n\tb();\n}\n", expected: &[] }, + Case { + name: "tool directives", + path: "case.ts", + code: "// prettier-ignore\nexport const a = [1,2];\n/*! (c) Acme --------- */\nexport const b = /* @__PURE__ */ make();\n", + expected: &[], + }, + Case { + name: "separate indents are separate comments", + path: "case.ts", + code: "// Rebuilds on change because the cache key is the path.\n\t// a.run();\nexport const b = 1;\n", + expected: &[(58, 69, "Delete commented-out code; git keeps the history.")], + }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_polling_loop_in_tests.rs b/crates/lint/src/rules/anti_slop/tests/no_polling_loop_in_tests.rs new file mode 100644 index 0000000..3de3e8a --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_polling_loop_in_tests.rs @@ -0,0 +1,80 @@ +//! `anti-slop/no-polling-loop-in-tests`: adapted from t3code's `t3code/no-polling-loop-in-tests`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-polling-loop-in-tests", + "[]", + &[ + Case { + name: "sleep until a predicate", + path: "case.test.ts", + code: "it(\"x\", async () => {\n\twhile (!done()) {\n\t\tawait new Promise((resolve) => setTimeout(resolve, 10));\n\t}\n});\n", + expected: &[( + 23, + 102, + "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`.", + )], + }, + Case { + name: "tick until a break", + path: "case.test.ts", + code: "for (;;) {\n\tif (ready()) {\n\t\tbreak;\n\t}\n\n\tawait Promise.resolve();\n}\n", + expected: &[( + 0, + 67, + "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`.", + )], + }, + Case { + name: "counter nothing advances", + path: "case.test.ts", + code: "let tries = 0;\n\nwhile (tries < 5) {\n\tawait Promise.resolve();\n}\n", + expected: &[( + 16, + 63, + "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`.", + )], + }, + Case { + name: "promise timer", + path: "case.test.ts", + code: "import { setTimeout as sleep } from \"node:timers/promises\";\n\nwhile (!done()) {\n\tawait sleep(5);\n}\n", + expected: &[( + 61, + 97, + "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`.", + )], + }, + Case { + name: "setImmediate on a predicate", + path: "case.test.ts", + code: "do {\n\tsetImmediate(tick);\n} while (pending.length > 0);\n", + expected: &[( + 0, + 55, + "Polling loop in a test: spinning on a tick or a sleep depends on the scheduler and runner load. Await the promise, event, or callback that marks the milestone, or use `expect.poll`.", + )], + }, + Case { name: "fixed flushes", path: "case.test.ts", code: "for (let i = 0; i < 3; i++) {\n\tawait Promise.resolve();\n}\n", expected: &[] }, + Case { + name: "advanced counter", + path: "case.test.ts", + code: "let attempt = 0;\n\nwhile (attempt < max) {\n\tawait Promise.resolve();\n\tattempt += 1;\n}\n", + expected: &[], + }, + Case { name: "no wait", path: "case.test.ts", code: "while (!done()) {\n\tstep();\n}\n", expected: &[] }, + Case { + name: "wait in a callback", + path: "case.test.ts", + code: "while (!done()) {\n\tqueue.push(async () => {\n\t\tawait Promise.resolve();\n\t});\n}\n", + expected: &[], + }, + Case { name: "loop over items", path: "case.test.ts", code: "for (const item of items) {\n\tawait Promise.resolve(item);\n}\n", expected: &[] }, + Case { name: "not a test file", path: "case.ts", code: "while (!done()) {\n\tawait Promise.resolve();\n}\n", expected: &[] }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_real_time_wait_in_tests.rs b/crates/lint/src/rules/anti_slop/tests/no_real_time_wait_in_tests.rs new file mode 100644 index 0000000..10a435f --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_real_time_wait_in_tests.rs @@ -0,0 +1,72 @@ +//! `anti-slop/no-real-time-wait-in-tests`: adapted from t3code's `t3code/no-real-time-wait-in-tests`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-real-time-wait-in-tests", + "[]", + &[ + Case { + name: "sleep through a promise", + path: "case.test.ts", + code: "it(\"waits\", async () => {\n\tawait new Promise((resolve) => setTimeout(resolve, 100));\n});\n", + expected: &[( + 58, + 68, + "Real-clock wait in a test: it passes or fails with runner load. Await the promise, event, or callback that marks the milestone, or turn on fake timers and advance them.", + )], + }, + Case { + name: "function executor", + path: "case.test.ts", + code: "it(\"waits\", async () => {\n\tawait new Promise(function (resolve) {\n\t\tsetTimeout(resolve, 100);\n\t});\n});\n", + expected: &[( + 68, + 78, + "Real-clock wait in a test: it passes or fails with runner load. Await the promise, event, or callback that marks the milestone, or turn on fake timers and advance them.", + )], + }, + Case { + name: "promise timer", + path: "case.test.ts", + code: "import { setTimeout as sleep } from \"node:timers/promises\";\n\nit(\"waits\", async () => {\n\tawait sleep(100);\n});\n", + expected: &[( + 94, + 99, + "Real-clock wait in a test: it passes or fails with runner load. Await the promise, event, or callback that marks the milestone, or turn on fake timers and advance them.", + )], + }, + Case { + name: "promise timer namespace", + path: "case.test.ts", + code: "import * as timers from \"timers/promises\";\n\nit(\"waits\", async () => {\n\tawait timers.setTimeout(10);\n});\n", + expected: &[( + 77, + 94, + "Real-clock wait in a test: it passes or fails with runner load. Await the promise, event, or callback that marks the milestone, or turn on fake timers and advance them.", + )], + }, + Case { + name: "timer outside a promise", + path: "case.test.ts", + code: "it(\"schedules\", () => {\n\tsetTimeout(() => {}, 10);\n});\n", + expected: &[], + }, + Case { + name: "fake timers", + path: "case.test.ts", + code: "vi.useFakeTimers();\n\nit(\"waits\", async () => {\n\tconst done = new Promise((resolve) => setTimeout(resolve, 100));\n\n\tvi.advanceTimersByTime(100);\n\tawait done;\n});\n", + expected: &[], + }, + Case { + name: "not a test file", + path: "case.ts", + code: "export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));\n", + expected: &[], + }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_test_in_loop.rs b/crates/lint/src/rules/anti_slop/tests/no_test_in_loop.rs new file mode 100644 index 0000000..005ee47 --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_test_in_loop.rs @@ -0,0 +1,59 @@ +//! `anti-slop/no-test-in-loop`: adapted from t3code's `t3code/no-test-in-loop`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-test-in-loop", + "[]", + &[ + Case { + name: "it in a for-of loop", + path: "case.test.ts", + code: "for (const c of cases) {\n\tit(c.name, () => {});\n}\n", + expected: &[(26, 28, "Do not call `it(…)` inside a for loop. Use `it.each(cases)(name, …)` instead.")], + }, + Case { + name: "describe in a for loop", + path: "case.test.ts", + code: "for (let i = 0; i < 3; i++) {\n\tdescribe(String(i), () => {});\n}\n", + expected: &[(31, 39, "Do not call `describe(…)` inside a for loop. Use `describe.each(cases)(name, …)` instead.")], + }, + Case { + name: "test.concurrent in a for-in loop", + path: "case.test.ts", + code: "for (const key in table) {\n\ttest.concurrent(key, () => {});\n}\n", + expected: &[(28, 43, "Do not call `test.concurrent(…)` inside a for loop. Use `test.concurrent.each(cases)(name, …)` instead.")], + }, + Case { name: "it.each", path: "case.test.ts", code: "it.each(cases)(\"%s\", () => {});\n", expected: &[] }, + Case { + name: "a loop inside a test", + path: "case.test.ts", + code: "it(\"sums\", () => {\n\tfor (const c of cases) {\n\t\texpect(c).toBe(1);\n\t}\n});\n", + expected: &[], + }, + Case { + name: "a test in a callback inside a loop", + path: "case.test.ts", + code: "for (const c of cases) {\n\tgroups.forEach(() => it(c, () => {}));\n}\n", + expected: &[], + }, + Case { name: "a while loop", path: "case.test.ts", code: "while (next()) {\n\tit(\"x\", () => {});\n}\n", expected: &[] }, + Case { name: "not a test file", path: "case.ts", code: "for (const c of cases) {\n\tit(c.name, () => {});\n}\n", expected: &[] }, + Case { + name: "node:test has no each", + path: "case.test.ts", + code: "import { it } from \"node:test\";\n\nfor (const c of cases) {\n\tit(c.name, () => {});\n}\n", + expected: &[], + }, + Case { + name: "spec file", + path: "case.spec.js", + code: "for (const c of cases) {\n\ttest(c.name, () => {});\n}\n", + expected: &[(26, 30, "Do not call `test(…)` inside a for loop. Use `test.each(cases)(name, …)` instead.")], + }, + ], + ); +} diff --git a/crates/lint/src/rules/anti_slop/tests/no_unreasoned_test_skip.rs b/crates/lint/src/rules/anti_slop/tests/no_unreasoned_test_skip.rs new file mode 100644 index 0000000..06e411a --- /dev/null +++ b/crates/lint/src/rules/anti_slop/tests/no_unreasoned_test_skip.rs @@ -0,0 +1,74 @@ +//! `anti-slop/no-unreasoned-test-skip`: adapted from the skip half of t3code's `t3code/no-test-timeout-or-unreasoned-skip`. + +use super::{Case, check}; + +/// The rule's behaviour, written for fmtkit rather than recorded from t3code's plugin. +#[test] +fn adapted() { + check( + "anti-slop/no-unreasoned-test-skip", + "[]", + &[ + Case { + name: "it.skip", + path: "case.test.ts", + code: "it.skip(\"x\", () => {});\n", + expected: &[(0, 7, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "describe.todo", + path: "case.test.ts", + code: "describe.todo(\"later\");\n", + expected: &[(0, 13, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "xit", + path: "case.test.ts", + code: "xit(\"x\", () => {});\n", + expected: &[(0, 3, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "skip option", + path: "case.test.ts", + code: "test(\"x\", { skip: true }, () => {});\n", + expected: &[(0, 35, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "computed key", + path: "case.test.ts", + code: "it[\"todo\"](\"x\");\n", + expected: &[(0, 10, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "skip then each", + path: "case.test.ts", + code: "it.skip.each(cases)(\"%s\", () => {});\n", + expected: &[(0, 7, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "modifier chain", + path: "case.test.ts", + code: "it.concurrent.skip(\"x\", () => {});\n", + expected: &[(0, 18, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { + name: "comment without a link", + path: "case.test.ts", + code: "// Flaky on CI.\nit.skip(\"x\", () => {});\n", + expected: &[(16, 23, "Say why this test is skipped: link the issue that tracks it (`#123` or a URL) in a comment above it.")], + }, + Case { name: "issue number above", path: "case.test.ts", code: "// Flaky on CI until #123 lands.\nit.skip(\"x\", () => {});\n", expected: &[] }, + Case { name: "issue link above", path: "case.test.ts", code: "// https://github.com/acme/app/issues/9\ntest.todo(\"x\");\n", expected: &[] }, + Case { + name: "linked skip inside a suite", + path: "case.test.ts", + code: "describe(\"suite\", () => {\n\t// Blocked on #41.\n\tit.skip(\"x\", () => {});\n});\n", + expected: &[], + }, + Case { name: "computed skip option", path: "case.test.ts", code: "test(\"x\", { skip: isWindows }, () => {});\n", expected: &[] }, + Case { name: "skipIf is a gate", path: "case.test.ts", code: "describe.skipIf(isWindows)(\"x\", () => {});\n", expected: &[] }, + Case { name: "skip off a non-test object", path: "case.test.ts", code: "list.skip(1);\n", expected: &[] }, + Case { name: "not a test file", path: "case.ts", code: "it.skip(\"x\", () => {});\n", expected: &[] }, + ], + ); +} diff --git a/crates/lint/tests/linter.rs b/crates/lint/tests/linter.rs index 1f5962d..f5621ab 100644 --- a/crates/lint/tests/linter.rs +++ b/crates/lint/tests/linter.rs @@ -71,6 +71,40 @@ fn overrides_replace_aliased_policy_entries() { assert!(rules_of(&on, "a.ts", Lang::Ts, "if (1 == 2) {}\n").iter().any(|(rule, ..)| rule == "eslint/eqeqeq")); } +#[test] +fn bundled_policy_requires_a_reason_for_type_checker_directives() { + let linter = Linter::new(&Lint::default()).unwrap(); + let flagged = |source: &str| rules_of(&linter, "a.ts", Lang::Ts, source).into_iter().filter(|(rule, ..)| rule == "typescript/ban-ts-comment").count(); + + for bare in [ + "// @ts-ignore +let a = 1; +", + "// @ts-expect-error +let a = 1; +", + "// @ts-nocheck +let a = 1; +", + ] { + assert_eq!(flagged(bare), 1, "{bare}"); + } + + for reasoned in [ + "// @ts-ignore: the fixture is untyped +let a = 1; +", + "// @ts-expect-error -- checks the runtime guard +let a = 1; +", + "// @ts-nocheck generated file +let a = 1; +", + ] { + assert_eq!(flagged(reasoned), 0, "{reasoned}"); + } +} + #[test] fn plugins_are_enabled_by_rule_names() { let linter = linter(&[("unicorn/prefer-node-protocol", json!("error"))]); From c7da75d42173d4edf1f5e676262f3fc16a9ce7f8 Mon Sep 17 00:00:00 2001 From: Gustavo Ocanto Date: Wed, 21 Oct 2026 17:31:55 +0800 Subject: [PATCH 2/4] Skip hook overloads and declare-function hooks in no-hookless-use-function Function::is_declaration() also matches TSDeclareFunction, so every overload signature of a real hook, and every `declare function useX()` in a .d.ts or .tsx file, was reported. A signature with no body has nothing to check. --- .../src/rules/anti_slop/no_hookless_use_function.rs | 5 +++-- .../anti_slop/tests/no_hookless_use_function.rs | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs b/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs index c32efb6..6735e7f 100644 --- a/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs +++ b/crates/lint/src/rules/anti_slop/no_hookless_use_function.rs @@ -32,11 +32,12 @@ impl Rule for NoHooklessUseFunction { fn run<'a>(&self, node: &AstNode<'a>, ctx: &mut Context<'_, 'a>) { let (name, span, calls_hook) = match node.kind() { AstKind::Function(function) if function.is_declaration() => { - let Some(id) = &function.id else { + // An overload or `declare function` has no body to check. + let (Some(id), Some(body)) = (&function.id, function.body.as_deref()) else { return; }; - (id.name.as_str(), id.span, function.body.as_deref().is_some_and(|body| HookCall::find(|finder| finder.visit_function_body(body)))) + (id.name.as_str(), id.span, HookCall::find(|finder| finder.visit_function_body(body))) } AstKind::VariableDeclarator(declarator) => { let (BindingPattern::BindingIdentifier(id), Some(init)) = (&declarator.id, &declarator.init) else { diff --git a/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs b/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs index dd030bb..ffe645d 100644 --- a/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs +++ b/crates/lint/src/rules/anti_slop/tests/no_hookless_use_function.rs @@ -25,6 +25,18 @@ fn adapted() { code: "import { useState } from \"react\";\n\nexport function useCount(): number {\n\tconst [n] = useState(0);\n\n\treturn n;\n}\n", expected: &[], }, + Case { + name: "overload signatures of a real hook", + path: "case.tsx", + code: "import { useState } from \"react\";\n\nexport function useN(a: string): number;\nexport function useN(a: number): number;\nexport function useN(a: unknown): number {\n\treturn useState(0)[0];\n}\n", + expected: &[], + }, + Case { + name: "declared hook", + path: "case.d.ts", + code: "import type { Foo } from \"react\";\n\nexport declare function useFoo(): Foo;\n", + expected: &[], + }, Case { name: "calls React.useState", path: "case.tsx", From de837e2563d726fabe8aa7d3ea668950a7a63dab Mon Sep 17 00:00:00 2001 From: Gustavo Ocanto Date: Wed, 21 Oct 2026 17:52:17 +0800 Subject: [PATCH 3/4] Pin hyperfine to 1.x in the bench workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hyperfine 2.0.0 (2026-10-07) runs benchmarked commands without a shell by default, so bench.sh's `cd … && fmtkit check` command is rejected, and it changes the --export-json schema that bench.sh reads the mean from. The workflow installed the latest release, so every pull request's bench job now fails. --- .github/workflows/bench.yml | 4 +++- scripts/bench.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 7a6266b..473445e 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -38,10 +38,12 @@ jobs: go-version-file: go/helper/go.mod cache-dependency-path: go/helper/go.sum + # hyperfine 2 runs commands without a shell and changes the JSON + # export, and scripts/bench.sh relies on both. - uses: taiki-e/install-action@v2 name: Install hyperfine with: - tool: hyperfine + tool: hyperfine@1 # Each side is built in its own checkout into storage/bench/. # A base that predates the Rust workspace has nothing to compare. diff --git a/scripts/bench.sh b/scripts/bench.sh index 72294b5..7e852ad 100755 --- a/scripts/bench.sh +++ b/scripts/bench.sh @@ -13,7 +13,7 @@ set -euo pipefail # fmtkit-go-helper. # # usage: bench.sh -# Requires bash, git, and hyperfine. +# Requires bash, git, and hyperfine 1.x. source "$(dirname "$0")/lib/env.sh" From b2eea1a7707fa731b1a3ccd8203ed4a7254e7eaf Mon Sep 17 00:00:00 2001 From: Gustavo Ocanto Date: Wed, 21 Oct 2026 17:58:59 +0800 Subject: [PATCH 4/4] Pin install-action to a commit and give the bench token read-only access The bench workflow runs pull-request code, so its GITHUB_TOKEN only needs to read the repository, and the action that installs hyperfine is pinned to the commit v2 points at today rather than the mutable tag. --- .github/workflows/bench.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 473445e..4902b6a 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -7,6 +7,9 @@ on: branches: - main +permissions: + contents: read + env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true' CARGO_TERM_COLOR: always @@ -40,7 +43,7 @@ jobs: # hyperfine 2 runs commands without a shell and changes the JSON # export, and scripts/bench.sh relies on both. - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@8cca0918f8471c33515202b73f56ab27f2bed283 # v2.87.29 name: Install hyperfine with: tool: hyperfine@1