Skip to content

Commit 277a309

Browse files
author
Eric Wheeler
committed
feat: Enforce explicit type coercion and empty blocks rules
Add no-implicit-coercion and no-empty rules to improve code quality across the codebase: - no-implicit-coercion: Enforces explicit, context-aware type comparisons to address ambiguity issues with falsy values and improve code clarity - no-empty: Requires all code blocks to have meaningful content, preventing silent error suppression and improving code readability This change includes: - Adding detailed documentation in .roo/rules/eslint-no-implicit-coercion.md with specific guidance on proper type checking patterns - Adding documentation for no-unused-vars rule enforcement in .roo/rules/eslint-no-unused-vars.md - Explicitly adding both rules as "error" in both root and webview-ui configs - Setting --max-warnings=0 in lint scripts to ensure strict enforcement These changes ensure developers use explicit type checks and properly document empty blocks, particularly important for distinguishing between null, undefined, and empty strings in conditional logic. Fixes: #3692 Signed-off-by: Eric Wheeler <[email protected]>
1 parent af2e9ed commit 277a309

File tree

5 files changed

+185
-1
lines changed

5 files changed

+185
-1
lines changed

.roo/rules/eslint-no-empty.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# Handling ESLint `no-empty` Rule
2+
3+
Empty block statements (`{}`) are disallowed. All blocks MUST include a logging statement (e.g., `console.error`, `console.warn`, `console.log`, `console.debug`) to make their purpose explicit.
4+
5+
This applies to `if`, `else`, `while`, `for`, `switch` cases, `try...catch` blocks, and function bodies.
6+
7+
### Examples:
8+
9+
```javascript
10+
// Correct: Logging in blocks
11+
if (condition) {
12+
console.warn("Condition met, no specific action.")
13+
}
14+
15+
try {
16+
criticalOperation()
17+
} catch (error) {
18+
// For unexpected errors:
19+
console.error("Unexpected error in criticalOperation:", error)
20+
}
21+
22+
function foo() {
23+
console.log("foo called, no operation.")
24+
}
25+
```
26+
27+
### Special Considerations:
28+
29+
- **Intentional Error Suppression**: Use `console.debug` in `catch` blocks if an error is intentionally suppressed.
30+
```javascript
31+
try {
32+
operationThatMightBenignlyFail()
33+
} catch (error) {
34+
console.debug("Benign failure in operation, suppressed:", error)
35+
}
36+
```
37+
- **Constructors**: Empty constructors should log, preferably with `console.debug`.
38+
- **Comments**: Comments can supplement logs but do not replace the logging requirement.
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
# Handling ESLint `no-implicit-coercion` Errors
2+
3+
When ESLint's `no-implicit-coercion` rule flags an error, a value is being implicitly converted to a boolean, number, or string. While ESLint might suggest `Boolean(value)`, `Number(value)`, or `String(value)`, this project requires a different approach for boolean coercions.
4+
5+
## Guideline: Explicit, Context-Aware Comparisons
6+
7+
For boolean coercions (e.g., in `if` statements or ternaries), MUST NOT use `Boolean(value)`. Instead, extend conditional expressions to explicitly compare against the specific data type and value expected for that implementation context.
8+
9+
YOU MUST NEVER replace `if (myVar)` with `if (Boolean(myVar))` or `if (!!myVar)` with `if (Boolean(myVar))`.
10+
11+
This rule's purpose is to encourage a thoughtful evaluation of what "truthy" or "falsy" means for the specific variable and logic.
12+
13+
### Examples:
14+
15+
## Incorrect (MUST NOT DO THIS):
16+
17+
```typescript
18+
// Implicit coercion (flagged by ESLint)
19+
if (someStringOrNull) {
20+
// ...
21+
}
22+
23+
// Explicit coercion using Boolean() (not the preferred fix here)
24+
if (Boolean(someStringOrNull)) {
25+
// ...
26+
}
27+
```
28+
29+
## Correct (Preferred Approach):
30+
31+
- Checking for a non-empty string:
32+
33+
```typescript
34+
if (typeof someStringOrNull === "string" && someStringOrNull != "") {
35+
// ...
36+
}
37+
// Or, if an empty string is valid but null/undefined is not:
38+
if (typeof someStringOrNull === "string") {
39+
// ...
40+
}
41+
```
42+
43+
- Checking against `null` or `undefined`:
44+
45+
```typescript
46+
if (someValue !== null && someValue !== undefined) {
47+
// ...
48+
}
49+
// Shorter (catches both null and undefined, mind other falsy values):
50+
if (someValue != null) {
51+
// ...
52+
}
53+
```
54+
55+
- Checking if a variable is assigned (not `undefined`):
56+
57+
```typescript
58+
if (someOptionalValue !== undefined) {
59+
// ...
60+
}
61+
```
62+
63+
- Checking if an array has elements:
64+
```typescript
65+
if (Array.isArray(myArray) && myArray.length > 0) {
66+
// ...
67+
}
68+
```
69+
70+
### Rationale:
71+
72+
Explicitly comparing types and values:
73+
74+
1. Clarifies the code's intent.
75+
2. Reduces ambiguity regarding how falsy values (`null`, `undefined`, `0`, `""`, `NaN`) are handled.
76+
3. Helps avoid bugs from overly general truthiness checks when specific conditions were needed.
77+
4. Be careful of regular expression evaluations. For example, `/foo (.*)bar/` will legitimately match an empty string, or it may not match at all. You MUST differentiate between `match === undefined` vs `typeof match === 'string' && match != ""` because falsy evaluation MUST NOT BE USED because it is usually invalid and certainly imprecise.
78+
79+
Always consider the context: What does "truthy" or "falsy" mean for this variable in this logic? Write conditions reflecting that precise meaning.
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# Handling `@typescript-eslint/no-unused-vars`
2+
3+
If `@typescript-eslint/no-unused-vars` flags an error, a declaration is unused.
4+
5+
## Guideline: Omit or Delete
6+
7+
Unused declarations MUST be omitted or deleted.
8+
9+
CRITICAL: Unused declarations MUST NOT be "fixed" by prefixing with an underscore (`_`). This is disallowed. Remove dead code, do not silence the linter.
10+
11+
### Examples:
12+
13+
#### Incorrect:
14+
15+
```typescript
16+
// error: 'unusedVar' is defined but never used.
17+
function example(usedParam: string, unusedParam: number): void {
18+
console.log(usedParam)
19+
// Incorrect fix:
20+
// const _unusedVar = 10;
21+
}
22+
```
23+
24+
```typescript
25+
// error: 'error' is defined but never used.
26+
try {
27+
// ...
28+
} catch (error) {
29+
// 'error' is unused
30+
// Incorrect fix:
31+
// } catch (_error) {
32+
console.error("An operation failed.")
33+
}
34+
```
35+
36+
#### Correct:
37+
38+
```typescript
39+
// 'unusedParam' removed if not needed by an interface/override.
40+
function example(usedParam: string): void {
41+
// 'unusedParam' removed
42+
console.log(usedParam)
43+
// 'unusedVar' is completely removed.
44+
}
45+
```
46+
47+
```typescript
48+
// 'error' variable is removed from the catch block if not used.
49+
try {
50+
// ...
51+
} catch {
52+
// 'error' variable omitted entirely
53+
console.error("An operation failed.")
54+
}
55+
```
56+
57+
### Rationale:
58+
59+
- Clarity: Removing unused code improves readability.
60+
- Bugs: Unused variables can indicate errors.
61+
- No Workarounds: Prefixing with `_` hides issues.
62+
63+
Code should be actively used.

src/eslint.config.mjs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@ export default [
88
// TODO: These should be fixed and the rules re-enabled.
99
"no-regex-spaces": "off",
1010
"no-useless-escape": "off",
11-
"no-empty": "off",
1211
"prefer-const": "off",
1312

13+
"no-empty": "error",
14+
"no-implicit-coercion": "error",
15+
1416
"@typescript-eslint/no-unused-vars": "off",
1517
"@typescript-eslint/no-explicit-any": "off",
1618
"@typescript-eslint/no-require-imports": "off",

webview-ui/eslint.config.mjs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ export default [
1818
"@typescript-eslint/no-explicit-any": "off",
1919
"react/prop-types": "off",
2020
"react/display-name": "off",
21+
"no-empty": "error",
22+
"no-implicit-coercion": "error",
2123
},
2224
},
2325
{

0 commit comments

Comments
 (0)