Extract rule: template-no-unused-block-params#2590
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-no-unused-block-params
Compared against the original ember-template-lint rule no-unused-block-params.
What's done well
- Handles the key complexity of this rule well. Shadowing of block params in nested blocks,
partialmarking all outer params as used, and the "only trailing unused params" behavior are all correctly implemented. - Test coverage is thorough. All original test cases are ported, including shadowing (
{{#each cats as |cat|}}{{#meow-meow cat as |cat|}}{{cat}}{{/meow-meow}}{{/each}}), component angle-bracket syntax (<BurgerMenu as |menu|>), curly syntax ({{#burger-menu as |menu|}}), andpartialinteraction. - The manual AST walking approach is a reasonable adaptation from the original's scope-based approach, since the ESLint Glimmer AST doesn't have the same scope frame infrastructure.
Observations and potential issues
-
Error message semantics differ. The original reports:
"'${unusedLocal}' is defined but never used", reporting a single unused param name. The new rule reports:"Block param \"${param}\" is unused"whereparamcan be a comma-joined list of all trailing unused params (e.g.,"item, index"). This is a behavioral difference -- the original reports one error per unused param, while the new rule reports one error per block statement with all trailing params joined. This could affect how users interact with the errors (e.g., suppressing individual params). -
Only trailing unused params are reported. The new rule intentionally only reports params after the last used one (matching the original's behavior). This is correct -- in Handlebars, you can't omit middle params, only trailing ones. Good.
-
The
ElementNodeexit visitor from the original is not directly replicated. The original has bothBlock.exitandElementNode.children.exitvisitors. The new rule only listens forGlimmerBlockStatement. This appears correct because in the ESLint Glimmer AST, angle-bracket components yielding block params would still be represented as block statements. However, it's worth verifying that<BurgerMenu as |menu|>is indeed parsed as aGlimmerBlockStatement(the test cases suggest it is, since they pass). -
collectChildNodesis thorough in walking the AST -- it handlesprogram,inverse,params,hash.pairs,body,path,attributes, andchildren. This covers the cases well. -
checkBlockPartsfunction is defined aftermodule.exports, which works in CommonJS but is a bit unusual. Minor style nit. -
No
template-lint-disabletest in gjs mode. The hbs tests includetemplate-lint-disablecomment cases from the original. These are valid in the hbs parser tests but the gjs tests don't test eslint-disable equivalents. Not a blocker, as eslint-disable is handled by ESLint core, not the rule itself.
Summary
This is one of the more complex rules to port due to scope tracking, and the implementation handles the complexity well. The main behavioral difference is the grouped error reporting for trailing params vs per-param errors. All original test scenarios are covered.
Automated review comparing with ember-template-lint source
ee0730a to
66a70c1
Compare
Split from #2371.