Skip to content

Conversation

@pchorus
Copy link

@pchorus pchorus commented Nov 17, 2025

Add ESLint rule max-statements.

@pchorus pchorus requested a review from camc314 as a code owner November 17, 2025 19:38
@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 17, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-linter Area - Linter C-enhancement Category - New feature or request labels Nov 17, 2025
Comment on lines +323 to +333
fn is_top_level_function(function_span: Span, ctx: &LintContext) -> bool {
let mut top_level_functions_finder =
TopLevelFunctionFinder { function_depth: 0, top_level_functions: vec![] };
top_level_functions_finder.visit_program(ctx.nodes().program());

// If there are several top-level functions, it means that the actual top-level function is the module (i.e., the file) itself.
// If there is only one top-level function, it should be ignored.
if top_level_functions_finder.top_level_functions.len() > 1 {
return false;
}
match top_level_functions_finder.top_level_functions.pop() {
Some(top_level_function) => top_level_function.span() == function_span,
None => false,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is_top_level_function implementation appears to have a logical issue. According to the test cases, all functions defined at the module level should be considered top-level functions, but the current implementation returns false if there are multiple top-level functions.

The condition on lines 330-332 that checks for multiple functions should be removed. Instead, the function should simply check if the given function's span matches any of the spans in the top_level_functions list.

This would better align with the expected behavior demonstrated in the test cases where all module-level functions are treated as top-level functions when ignoreTopLevelFunctions is enabled.

Suggested change
fn is_top_level_function(function_span: Span, ctx: &LintContext) -> bool {
let mut top_level_functions_finder =
TopLevelFunctionFinder { function_depth: 0, top_level_functions: vec![] };
top_level_functions_finder.visit_program(ctx.nodes().program());
// If there are several top-level functions, it means that the actual top-level function is the module (i.e., the file) itself.
// If there is only one top-level function, it should be ignored.
if top_level_functions_finder.top_level_functions.len() > 1 {
return false;
}
match top_level_functions_finder.top_level_functions.pop() {
Some(top_level_function) => top_level_function.span() == function_span,
None => false,
}
}
fn is_top_level_function(function_span: Span, ctx: &LintContext) -> bool {
let mut top_level_functions_finder =
TopLevelFunctionFinder { function_depth: 0, top_level_functions: vec![] };
top_level_functions_finder.visit_program(ctx.nodes().program());
// Check if the given function is one of the top-level functions
top_level_functions_finder.top_level_functions.iter().any(|func| func.span() == function_span)
}

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree to this. The suggestion would even break unit tests.
My understanding is as I stated in the comment in line 324:

"If there are several top-level functions, it means that the actual top-level function is the module (i.e., the file) itself. If there is only one top-level function, it should be ignored."

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 17, 2025

CodSpeed Performance Report

Merging #15804 will not alter performance

Comparing pchorus:feature/max-statements (964b2d8) with main (c167dfa)

Summary

✅ 4 untouched
⏩ 33 skipped1

Footnotes

  1. 33 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@pchorus pchorus force-pushed the feature/max-statements branch from 961e317 to 3efd152 Compare November 17, 2025 19:57
@pchorus
Copy link
Author

pchorus commented Nov 17, 2025

CI / Test Linux fails with error

thread 'size_asserts' (6658) panicked at crates/oxc_linter/src/lib.rs:94:5:
assertion `left == right` failed
  left: 24
 right: 16

@camc314 Unfortunately, I don't know how to fix this one.

@pchorus pchorus requested a review from camc314 November 18, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants