-
-
Notifications
You must be signed in to change notification settings - Fork 402
feat(linter-rules/no-unused-vars): declared var must be used #2955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
b82b8b0
45b51cb
52e40c6
56a7a3a
e7b5ebc
0c70878
899cc71
085291d
b800f27
7fa55fb
6a09e6f
24dcbc0
0cc4bf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
// @ts-check | ||
/** | ||
* Linter rule "no-unused-vars". | ||
* | ||
* Checks that an variable is used if declared (the first use is treated as | ||
* declaration). | ||
*/ | ||
import LinterRule from "../LinterRule.js"; | ||
import { lang as defaultLang } from "../l10n.js"; | ||
import { norm } from "../utils.js"; | ||
|
||
const name = "no-unused-vars"; | ||
|
||
const meta = { | ||
en: { | ||
description: "Variable was defined, but never used.", | ||
howToFix: "Add a `data-ignore-unused` attribute to the `<var>`.", | ||
help: "See developer console.", | ||
}, | ||
}; | ||
// Fall back to english, if language is missing | ||
const lang = defaultLang in meta ? defaultLang : "en"; | ||
|
||
/** | ||
* @param {*} _ | ||
* @param {Document} doc | ||
* @return {import("../LinterRule").LinterResult} | ||
*/ | ||
function linterFunction(_, doc) { | ||
const offendingElements = []; | ||
|
||
/** | ||
* Check if a <section> contains a `".algorithm"` | ||
* | ||
* The selector matches: | ||
* ``` html | ||
* <section><ul class="algorithm"></ul></section> | ||
* ``` | ||
* The selector does not match: | ||
* ``` html | ||
* <section><div><ul class="algorithm"></ul></div></section> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be supported? Seems unlikely, but possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, commented at the same time 😂 yes, I think we should support it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What’s the rationale for this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The selector gets complex (match section only if section contains algorithm, but not any subsection). Looking at specs, most |
||
* <section><section><ul class="algorithm"></ul></section></section> | ||
* ``` | ||
* @param {HTMLElement} section | ||
*/ | ||
const sectionContainsAlgorithm = section => | ||
!!section.querySelector(":scope > :not(section) ~ .algorithm"); | ||
|
||
for (const section of doc.querySelectorAll("section")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we pick only those sections that contain algorithms? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so... we might need to test out a few specs. |
||
if (!sectionContainsAlgorithm(section)) continue; | ||
|
||
/** | ||
* `<var>` in this section, but excluding those in child sections. | ||
* @type {NodeListOf<HTMLElement>} | ||
*/ | ||
const varElems = section.querySelectorAll(":scope > :not(section) var"); | ||
if (!varElems.length) continue; | ||
|
||
/** @type {Map<string, HTMLElement[]>} */ | ||
const varUsage = new Map(); | ||
for (const varElem of varElems) { | ||
const key = norm(varElem.textContent); | ||
const elems = varUsage.get(key) || varUsage.set(key, []).get(key); | ||
elems.push(varElem); | ||
} | ||
|
||
for (const vars of varUsage.values()) { | ||
if (vars.length === 1 && !vars[0].hasAttribute("data-ignore-unused")) { | ||
offendingElements.push(vars[0]); | ||
} | ||
} | ||
} | ||
|
||
if (!offendingElements.length) { | ||
return; | ||
} | ||
return { | ||
name, | ||
offendingElements, | ||
occurrences: offendingElements.length, | ||
...meta[lang], | ||
}; | ||
} | ||
export const rule = new LinterRule(name, linterFunction); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
"use strict"; | ||
|
||
import { rule } from "../../../../src/core/linter-rules/no-unused-vars.js"; | ||
|
||
describe("Core Linter Rule - 'no-unused-vars'", () => { | ||
const config = { | ||
lint: { "no-unused-vars": true }, | ||
}; | ||
const doc = document.implementation.createHTMLDocument("test doc"); | ||
beforeEach(() => { | ||
// Make sure every unordered test get an empty document | ||
// See: https://github.com/w3c/respec/pull/1495 | ||
while (doc.body.firstChild) { | ||
doc.body.removeChild(doc.body.firstChild); | ||
} | ||
}); | ||
|
||
it("skips sections without .algorithm", async () => { | ||
doc.body.innerHTML = ` | ||
<section> | ||
<p><var>varA</var></p> | ||
<div> | ||
<ul class="algorithm"> | ||
<li><var>varB</var></li> | ||
</ul> | ||
</div> | ||
<section> | ||
<p><var>varC</var></p> | ||
</section> | ||
</section> | ||
`; | ||
|
||
const result = await rule.lint(config, doc); | ||
expect(result).toBeUndefined(); | ||
}); | ||
|
||
it("complains on unused vars", async () => { | ||
doc.body.innerHTML = ` | ||
<section> | ||
<p><var>varA</var> is unused</p> | ||
<ul class="algorithm"> | ||
<li><var>varB</var> is unused</li> | ||
<li><var>varC</var></li> | ||
<li><var>varC</var></li> | ||
</ul> | ||
<section> | ||
<p><var>varD</var></p> | ||
<ul class="algorithm"> | ||
<li><var>varE</var></li> | ||
<li><var>varF</var></li> | ||
<li><var>varF</var></li> | ||
</ul> | ||
</section> | ||
</section> | ||
`; | ||
|
||
const result = await rule.lint(config, doc); | ||
expect(result.name).toBe("no-unused-vars"); | ||
expect(result.occurrences).toBe(4); | ||
const unusedVars = result.offendingElements.map(v => v.textContent); | ||
expect(unusedVars).toEqual(["varA", "varB", "varD", "varE"]); | ||
}); | ||
|
||
it("ignores unused vars with data-ignore-unused", async () => { | ||
doc.body.innerHTML = ` | ||
<section> | ||
<p><var data-ignore-unused>varA</var> is unused</p> | ||
<ul class="algorithm"> | ||
<li><var>varB</var> is unused</li> | ||
<li><var data-ignore-unused>varC</var> is unused</li> | ||
<li><var>varD</var></li> | ||
<li><var>varD</var></li> | ||
</ul> | ||
</section> | ||
`; | ||
|
||
const result = await rule.lint(config, doc); | ||
expect(result.name).toBe("no-unused-vars"); | ||
expect(result.occurrences).toBe(1); | ||
const unusedVars = result.offendingElements.map(v => v.textContent); | ||
expect(unusedVars).toEqual(["varB"]); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure the message here looks like one you would get from ESLint... that is, it should be clear that a "variable X was defined but never used".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't have access to "X" in linter message, so maybe:
"Variable was defined, but never used. Add a
data-ignore-unused
attribute to the<var>
, or consider using<em>
/<code>
"Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the "data-ignore-unused" seems like an good suggestion... the other not so much as they are working around the problem, not addressing the problem explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what's going on with my comment's formatting above... 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe author used
<var>
for its style when they meant<em>
or<code>
(See https://github.com/w3c/webdriver/pull/1515/files)?Missed backticks on
<em>
🎈There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we can't add a
data-ignore-unused
to the var micro-syntax.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
<var>
for style is an author error, so let’s not worry about it (or encourage that).