Skip to content

Commit 0d41d3c

Browse files
NullVoxPopuliNullVoxPopuli
authored
Merge pull request #2644 from johanrd/post-merge-review/no-unused-block-params
Post-merge-review: Fix `template-no-unused-block-params`: detect angle-bracket block params and walk modifiers
2 parents c97528a + 0585ff8 commit 0d41d3c

2 files changed

Lines changed: 140 additions & 88 deletions

File tree

‎lib/rules/template-no-unused-block-params.js‎

Lines changed: 117 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ function collectChildNodes(n) {
2424
if (n.children) {
2525
children.push(...n.children);
2626
}
27+
if (n.modifiers) {
28+
children.push(...n.modifiers);
29+
}
2730
// GlimmerPathExpression also has 'parts', so make sure we're not treating
2831
// concat'd path string parts as AST nodes.
2932
if (n.type === 'GlimmerConcatStatement' && n.parts) {
@@ -56,6 +59,113 @@ function buildShadowedSet(shadowedParams, innerBlockParams, outerBlockParams) {
5659
return newShadowed;
5760
}
5861

62+
function checkBlockParts(n, blockParams, usedParams, shadowedParams, newShadowed, checkNodeFn) {
63+
// Check the path/params of the block statement itself with current scope
64+
if (n.path) {
65+
checkNodeFn(n.path, shadowedParams);
66+
}
67+
if (n.params) {
68+
for (const param of n.params) {
69+
checkNodeFn(param, shadowedParams);
70+
}
71+
}
72+
if (n.hash?.pairs) {
73+
for (const pair of n.hash.pairs) {
74+
checkNodeFn(pair.value, shadowedParams);
75+
}
76+
}
77+
78+
// Check the program body with the updated shadowed set
79+
if (n.program) {
80+
checkNodeFn(n.program, newShadowed);
81+
}
82+
if (n.inverse) {
83+
checkNodeFn(n.inverse, newShadowed);
84+
}
85+
}
86+
87+
/**
88+
* Scan child nodes for usage of blockParams, then report the first unused
89+
* trailing param. Shared by both GlimmerBlockStatement and GlimmerElementNode.
90+
*/
91+
function checkUnusedBlockParams(context, node, blockParams, startNodes) {
92+
const usedParams = new Set();
93+
94+
function checkNode(n, shadowedParams) {
95+
if (!n) {
96+
return;
97+
}
98+
99+
if (n.type === 'GlimmerPathExpression') {
100+
markParamIfUsed(n.original, blockParams, usedParams, shadowedParams);
101+
}
102+
103+
if (n.type === 'GlimmerElementNode') {
104+
markParamIfUsed(n.tag, blockParams, usedParams, shadowedParams);
105+
}
106+
107+
if (isPartialStatement(n)) {
108+
for (const p of blockParams) {
109+
if (!shadowedParams.has(p)) {
110+
usedParams.add(p);
111+
}
112+
}
113+
}
114+
115+
// Nested block with its own blockParams — shadow them
116+
if (n.type === 'GlimmerBlockStatement' && n.program?.blockParams?.length > 0) {
117+
const newShadowed = buildShadowedSet(shadowedParams, n.program.blockParams, blockParams);
118+
checkBlockParts(n, blockParams, usedParams, shadowedParams, newShadowed, checkNode);
119+
return;
120+
}
121+
122+
// Nested element with block params (e.g. <Component as |x|>) — shadow them
123+
if (n.type === 'GlimmerElementNode' && n.blockParams?.length > 0) {
124+
const newShadowed = buildShadowedSet(shadowedParams, n.blockParams, blockParams);
125+
if (n.attributes) {
126+
for (const attr of n.attributes) {
127+
checkNode(attr.value, shadowedParams);
128+
}
129+
}
130+
if (n.children) {
131+
for (const child of n.children) {
132+
checkNode(child, newShadowed);
133+
}
134+
}
135+
return;
136+
}
137+
138+
for (const child of collectChildNodes(n)) {
139+
checkNode(child, shadowedParams);
140+
}
141+
}
142+
143+
for (const startNode of startNodes) {
144+
checkNode(startNode, new Set());
145+
}
146+
147+
// Find the last index of a used param
148+
let lastUsedIndex = -1;
149+
for (let i = blockParams.length - 1; i >= 0; i--) {
150+
if (usedParams.has(blockParams[i])) {
151+
lastUsedIndex = i;
152+
break;
153+
}
154+
}
155+
156+
// Only report trailing unused params (after the last used one)
157+
const unusedTrailing = blockParams.slice(lastUsedIndex + 1);
158+
const firstUnusedTrailing = unusedTrailing[0];
159+
160+
if (firstUnusedTrailing) {
161+
context.report({
162+
node,
163+
messageId: 'unusedBlockParam',
164+
data: { param: firstUnusedTrailing },
165+
});
166+
}
167+
}
168+
59169
/** @type {import('eslint').Rule.RuleModule} */
60170
module.exports = {
61171
meta: {
@@ -82,98 +192,17 @@ module.exports = {
82192
return {
83193
GlimmerBlockStatement(node) {
84194
const blockParams = node.program?.blockParams || [];
85-
if (blockParams.length === 0) {
86-
return;
195+
if (blockParams.length > 0) {
196+
checkUnusedBlockParams(context, node, blockParams, [node.program]);
87197
}
198+
},
88199

89-
const usedParams = new Set();
90-
91-
function checkNode(n, shadowedParams) {
92-
if (!n) {
93-
return;
94-
}
95-
96-
if (n.type === 'GlimmerPathExpression') {
97-
markParamIfUsed(n.original, blockParams, usedParams, shadowedParams);
98-
}
99-
100-
if (n.type === 'GlimmerElementNode') {
101-
markParamIfUsed(n.tag, blockParams, usedParams, shadowedParams);
102-
}
103-
104-
if (isPartialStatement(n)) {
105-
for (const p of blockParams) {
106-
if (!shadowedParams.has(p)) {
107-
usedParams.add(p);
108-
}
109-
}
110-
}
111-
112-
// When entering a nested block, add its blockParams to the shadowed set
113-
if (n.type === 'GlimmerBlockStatement' && n.program?.blockParams?.length > 0) {
114-
const newShadowed = buildShadowedSet(
115-
shadowedParams,
116-
n.program.blockParams,
117-
blockParams
118-
);
119-
checkBlockParts(n, blockParams, usedParams, shadowedParams, newShadowed, checkNode);
120-
return;
121-
}
122-
123-
// Recursively check children
124-
for (const child of collectChildNodes(n)) {
125-
checkNode(child, shadowedParams);
126-
}
127-
}
128-
129-
checkNode(node.program, new Set());
130-
131-
// Find the last index of a used param
132-
let lastUsedIndex = -1;
133-
for (let i = blockParams.length - 1; i >= 0; i--) {
134-
if (usedParams.has(blockParams[i])) {
135-
lastUsedIndex = i;
136-
break;
137-
}
138-
}
139-
140-
// Only report trailing unused params (after the last used one)
141-
const unusedTrailing = blockParams.slice(lastUsedIndex + 1);
142-
const firstUnusedTrailing = unusedTrailing[0];
143-
144-
if (firstUnusedTrailing) {
145-
context.report({
146-
node,
147-
messageId: 'unusedBlockParam',
148-
data: { param: firstUnusedTrailing },
149-
});
200+
GlimmerElementNode(node) {
201+
const blockParams = node.blockParams || [];
202+
if (blockParams.length > 0) {
203+
checkUnusedBlockParams(context, node, blockParams, node.children || []);
150204
}
151205
},
152206
};
153207
},
154208
};
155-
156-
function checkBlockParts(n, blockParams, usedParams, shadowedParams, newShadowed, checkNodeFn) {
157-
// Check the path/params of the block statement itself with current scope
158-
if (n.path) {
159-
checkNodeFn(n.path, shadowedParams);
160-
}
161-
if (n.params) {
162-
for (const param of n.params) {
163-
checkNodeFn(param, shadowedParams);
164-
}
165-
}
166-
if (n.hash?.pairs) {
167-
for (const pair of n.hash.pairs) {
168-
checkNodeFn(pair.value, shadowedParams);
169-
}
170-
}
171-
172-
// Check the program body with the updated shadowed set
173-
if (n.program) {
174-
checkNodeFn(n.program, newShadowed);
175-
}
176-
if (n.inverse) {
177-
checkNodeFn(n.inverse, newShadowed);
178-
}
179-
}

‎tests/lib/rules/template-no-unused-block-params.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,12 @@ const validHbs = [
9090
'{{#with (component "foo-bar") as |FooBar|}}<FooBar />{{/with}}',
9191
'<BurgerMenu as |menu|><header>Something</header><menu.item>Text</menu.item></BurgerMenu>',
9292
'{{#burger-menu as |menu|}}<header>Something</header>{{#menu.item}}Text{{/menu.item}}{{/burger-menu}}',
93+
'<MyComponent as |item|>{{item}}</MyComponent>',
94+
// Outer `item` is used via @value (outer scope); inner `as |item|` shadows
95+
// outer in <Inner>'s children.
96+
'<Outer as |item|><Inner @value={{item}} as |item|>{{item}}</Inner></Outer>',
97+
'<MyComponent as |handler|><button {{on "click" handler}}>X</button></MyComponent>',
98+
'{{#let this.fn as |handler|}}<button {{on "click" handler}}>X</button>{{/let}}',
9399
];
94100

95101
const invalidHbs = [
@@ -123,6 +129,23 @@ const invalidHbs = [
123129
output: null,
124130
errors: [{ messageId: 'unusedBlockParam', data: { param: 'cat' } }],
125131
},
132+
{
133+
code: '<MyComponent as |item unused|>{{item}}</MyComponent>',
134+
output: null,
135+
errors: [{ messageId: 'unusedBlockParam', data: { param: 'unused' } }],
136+
},
137+
{
138+
code: '<MyComponent as |unused|>content</MyComponent>',
139+
output: null,
140+
errors: [{ messageId: 'unusedBlockParam', data: { param: 'unused' } }],
141+
},
142+
// The outer `item` is only "used" inside <Inner> where it is shadowed by
143+
// the inner `as |item|`, so the outer should be reported as unused.
144+
{
145+
code: '<Outer as |item|><Inner as |item|>{{item}}</Inner></Outer>',
146+
output: null,
147+
errors: [{ messageId: 'unusedBlockParam', data: { param: 'item' } }],
148+
},
126149
];
127150

128151
function wrapTemplate(entry) {

0 commit comments

Comments
 (0)