From 86ab88f70eda1e5968bb29fcf68bf5b15e1d849e Mon Sep 17 00:00:00 2001 From: Karel Bousson Date: Tue, 6 Oct 2026 15:06:56 +0200 Subject: [PATCH 1/2] fix(template-no-positive-tabindex): allow dynamic values whose type is provably safe When type information is available, resolve `this.foo`, `@foo` and in-scope variables used as tabindex values (directly, in concat, or in if/unless branches) and accept them when every member of their type is a numeric literal <= 0 or null/undefined, e.g. `0 | -1`. The TypeScript resolution of Glimmer paths lives in lib/utils/glimmer-path-type.js so other rules can reuse it. Fixes #2805 Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/rules/template-no-positive-tabindex.md | 12 ++ lib/rules/template-no-positive-tabindex.js | 158 ++++++++++-------- lib/utils/glimmer-path-type.js | 84 ++++++++++ .../component-stub.ts | 3 + .../template-no-positive-tabindex/tabindex.ts | 2 + .../template-no-positive-tabindex/usage.gts | 2 + .../rules/template-no-positive-tabindex.js | 144 ++++++++++++++++ 7 files changed, 334 insertions(+), 71 deletions(-) create mode 100644 lib/utils/glimmer-path-type.js create mode 100644 tests/lib/rules-preprocessor/template-no-positive-tabindex/component-stub.ts create mode 100644 tests/lib/rules-preprocessor/template-no-positive-tabindex/tabindex.ts create mode 100644 tests/lib/rules-preprocessor/template-no-positive-tabindex/usage.gts diff --git a/docs/rules/template-no-positive-tabindex.md b/docs/rules/template-no-positive-tabindex.md index dd5cb20cbe..78cb24729f 100644 --- a/docs/rules/template-no-positive-tabindex.md +++ b/docs/rules/template-no-positive-tabindex.md @@ -12,6 +12,8 @@ This rule prevents usage of any `tabindex` values other than `0` and `-1`. It does allow for dynamic values (choosing which value to show based on some condition / helper / etc), but only if that inline `if` condition has static `0`/`-1` as the value. +In `.gts` files linted with type information (`parserOptions.project` or `parserOptions.projectService`), dynamic values such as `this.tabIndex`, `@tabIndex` or an in-scope variable are also allowed when their TypeScript type only contains numeric literals `<= 0` (and optionally `null`/`undefined`), e.g. `0 | -1`. Types like `number` are still reported, since they can't be proven safe. + This rule takes no arguments. ## Examples @@ -26,6 +28,16 @@ This rule **allows** the following:
baz
``` +With type information: + +```gts +import Component from '@glimmer/component'; + +export default class Tab extends Component<{ Args: { tabIndex: 0 | -1 } }> { + +} +``` + This rule **forbids** the following: ```hbs diff --git a/lib/rules/template-no-positive-tabindex.js b/lib/rules/template-no-positive-tabindex.js index 6764634a43..481e06ac9f 100644 --- a/lib/rules/template-no-positive-tabindex.js +++ b/lib/rules/template-no-positive-tabindex.js @@ -1,91 +1,103 @@ -/** - * Check a tabindex attribute value and return the violation type, if any. - * Returns null if safe, 'positive' if the value is a positive integer, - * or 'mustBeNegativeNumeric' if the value is non-numeric/dynamic/boolean. - */ -function getTabindexViolation(attrValue) { - if (!attrValue) { - return null; - } +'use strict'; - // Handle simple text values like tabindex="0" or tabindex="-1" - if (attrValue.type === 'GlimmerTextNode') { - const value = Number.parseInt(attrValue.chars, 10); - if (Number.isNaN(value)) { - return 'mustBeNegativeNumeric'; - } - return value > 0 ? 'positive' : null; - } +const { createGlimmerPathTypeResolver } = require('../utils/glimmer-path-type'); - // Handle mustache statements like tabindex={{-1}} or tabindex={{someProperty}} - if (attrValue.type === 'GlimmerMustacheStatement') { - const path = attrValue.path; +// ts.TypeFlags values, hardcoded to avoid adding a direct `typescript` +// dependency (see also template-no-deprecated). +const TS_UNDEFINED_FLAG = 32_768; +const TS_NULL_FLAG = 65_536; - if (path.type === 'GlimmerNumberLiteral') { - return Number.parseInt(path.original, 10) > 0 ? 'positive' : null; - } - if (path.type === 'GlimmerStringLiteral') { - const value = Number.parseInt(path.original, 10); - if (Number.isNaN(value)) { - return 'mustBeNegativeNumeric'; - } - return value > 0 ? 'positive' : null; - } - - // Handle conditional expressions like {{if this.show -1 0}} - if ( - path.type === 'GlimmerPathExpression' && - (path.original === 'if' || path.original === 'unless') - ) { - return getConditionalTabindexViolation(attrValue.params); - } - - // Any other dynamic value (variable, boolean, etc.) is not verifiably safe +function getNumericViolation(value) { + if (Number.isNaN(value)) { return 'mustBeNegativeNumeric'; } + return value > 0 ? 'positive' : null; +} - // Handle concat statements like tabindex="{{-1}}" or tabindex="{{false}}" - if (attrValue.type === 'GlimmerConcatStatement') { - const parts = attrValue.parts || []; - if (parts.length > 0 && parts[0].type === 'GlimmerMustacheStatement') { - return getTabindexViolation(parts[0]); +/** + * Every member of the type must be a numeric (string) literal <= 0, or + * null/undefined (which omits the attribute), e.g. `0 | -1`. + */ +function getTypeViolation(type) { + let violation = null; + for (const member of type.isUnion() ? type.types : [type]) { + // eslint-disable-next-line no-bitwise + if (member.flags & (TS_UNDEFINED_FLAG | TS_NULL_FLAG)) { + continue; } - return 'mustBeNegativeNumeric'; + const literal = member.isNumberLiteral() || member.isStringLiteral() ? member.value : ''; + const memberViolation = getNumericViolation(Number.parseInt(literal, 10)); + if (memberViolation === 'mustBeNegativeNumeric') { + return memberViolation; + } + violation ||= memberViolation; } - - return 'mustBeNegativeNumeric'; + return violation; } /** - * Check that all branches of a conditional (if/unless) expression are safe. + * Check a single value expression: a literal, or a path whose TypeScript type + * proves it safe. Anything else is not verifiably safe. */ -function getConditionalTabindexViolation(params) { - if (!params) { - return 'mustBeNegativeNumeric'; +function getExpressionViolation(node, getPathType) { + switch (node.type) { + case 'GlimmerNumberLiteral': + case 'GlimmerStringLiteral': { + return getNumericViolation(Number.parseInt(node.original, 10)); + } + case 'GlimmerPathExpression': { + const type = getPathType?.(node); + return type ? getTypeViolation(type) : 'mustBeNegativeNumeric'; + } + default: { + return 'mustBeNegativeNumeric'; + } } +} - // Check the value branches (params[1] and optionally params[2]) - for (let i = 1; i < params.length && i < 3; i++) { - const param = params[i]; - if (param.type === 'GlimmerNumberLiteral') { - if (Number.parseInt(param.original, 10) > 0) { - return 'positive'; - } - } else if (param.type === 'GlimmerStringLiteral') { - const val = Number.parseInt(param.original, 10); - if (Number.isNaN(val)) { - return 'mustBeNegativeNumeric'; - } - if (val > 0) { - return 'positive'; +/** + * Check a tabindex attribute value and return the violation type, if any. + * Returns null if safe, 'positive' if the value is a positive integer, + * or 'mustBeNegativeNumeric' if the value is non-numeric/dynamic/boolean. + */ +function getTabindexViolation(attrValue, getPathType) { + switch (attrValue.type) { + // tabindex="0" + case 'GlimmerTextNode': { + return getNumericViolation(Number.parseInt(attrValue.chars, 10)); + } + // tabindex={{-1}}, tabindex={{this.tabIndex}}, tabindex={{if this.show -1 0}} + case 'GlimmerMustacheStatement': { + const { path, params } = attrValue; + if ( + path.type === 'GlimmerPathExpression' && + (path.original === 'if' || path.original === 'unless') + ) { + // Every value branch must be safe + for (const branch of params.slice(1, 3)) { + const violation = getExpressionViolation(branch, getPathType); + if (violation) { + return violation; + } + } + return null; } - } else { - // Dynamic value in branch — not verifiably safe + // A path with params is a helper call, which is not verifiably safe + return params.length > 0 + ? 'mustBeNegativeNumeric' + : getExpressionViolation(path, getPathType); + } + // tabindex="{{-1}}" + case 'GlimmerConcatStatement': { + const [firstPart] = attrValue.parts ?? []; + return firstPart?.type === 'GlimmerMustacheStatement' + ? getTabindexViolation(firstPart, getPathType) + : 'mustBeNegativeNumeric'; + } + default: { return 'mustBeNegativeNumeric'; } } - - return null; } /** @type {import('eslint').Rule.RuleModule} */ @@ -113,7 +125,11 @@ module.exports = { }, create(context) { + const typeResolver = createGlimmerPathTypeResolver(context); + return { + ...typeResolver?.visitors, + GlimmerElementNode(node) { const tabindexAttr = node.attributes?.find((attr) => attr.name === 'tabindex'); @@ -121,7 +137,7 @@ module.exports = { return; } - const violation = getTabindexViolation(tabindexAttr.value); + const violation = getTabindexViolation(tabindexAttr.value, typeResolver?.getPathType); if (violation) { context.report({ node: tabindexAttr, diff --git a/lib/utils/glimmer-path-type.js b/lib/utils/glimmer-path-type.js new file mode 100644 index 0000000000..60b7f41fd8 --- /dev/null +++ b/lib/utils/glimmer-path-type.js @@ -0,0 +1,84 @@ +'use strict'; + +/** + * Resolve the TypeScript type of Glimmer path expressions (`this.foo.bar`, + * `@foo`, `foo`) in gjs/gts templates. + * + * Returns `null` when no type information is available. Otherwise returns + * `{ visitors, getPathType }`: `visitors` must be merged into the rule's + * visitors (they track the enclosing class), and `getPathType(path)` returns + * the path's `ts.Type`, or `undefined` when it can't be resolved (block + * params, unknown properties, template-only components, ...). + */ +function createGlimmerPathTypeResolver(context) { + const sourceCode = context.sourceCode; + const services = sourceCode.parserServices; + if (!services?.program || !services.esTreeNodeToTSNodeMap) { + return null; + } + + const checker = services.program.getTypeChecker(); + const classStack = []; + + function getClassInstanceType() { + const tsClass = services.esTreeNodeToTSNodeMap.get(classStack.at(-1)); + if (!tsClass) { + return undefined; + } + // Class declarations resolve to the instance type, class expressions to + // the constructor type. + const type = checker.getTypeAtLocation(tsClass); + return type.getConstructSignatures()[0]?.getReturnType() ?? type; + } + + function getPropertyType(type, name) { + const symbol = type?.getProperty(name); + const location = services.esTreeNodeToTSNodeMap.get(classStack.at(-1) ?? sourceCode.ast); + return symbol && checker.getTypeOfSymbolAtLocation(symbol, location); + } + + function getHeadType(path) { + switch (path.head.type) { + case 'ThisHead': { + return getClassInstanceType(); + } + case 'AtHead': { + const argsType = getPropertyType(getClassInstanceType(), 'args'); + return getPropertyType(argsType, path.head.name.slice(1)); + } + case 'VarHead': { + const ref = sourceCode + .getScope(path) + .references.find((reference) => reference.identifier === path.head); + const tsNode = services.esTreeNodeToTSNodeMap.get(ref?.resolved?.defs[0]?.name); + return tsNode && checker.getTypeAtLocation(tsNode); + } + default: { + return undefined; + } + } + } + + function getPathType(path) { + try { + return path.tail.reduce(getPropertyType, getHeadType(path)); + } catch { + return undefined; + } + } + + const pushClass = (node) => classStack.push(node); + const popClass = () => classStack.pop(); + + return { + visitors: { + ClassDeclaration: pushClass, + ClassExpression: pushClass, + 'ClassDeclaration:exit': popClass, + 'ClassExpression:exit': popClass, + }, + getPathType, + }; +} + +module.exports = { createGlimmerPathTypeResolver }; diff --git a/tests/lib/rules-preprocessor/template-no-positive-tabindex/component-stub.ts b/tests/lib/rules-preprocessor/template-no-positive-tabindex/component-stub.ts new file mode 100644 index 0000000000..e926929a71 --- /dev/null +++ b/tests/lib/rules-preprocessor/template-no-positive-tabindex/component-stub.ts @@ -0,0 +1,3 @@ +export default class ComponentBase { + declare args: S['Args']; +} diff --git a/tests/lib/rules-preprocessor/template-no-positive-tabindex/tabindex.ts b/tests/lib/rules-preprocessor/template-no-positive-tabindex/tabindex.ts new file mode 100644 index 0000000000..7234d16d5b --- /dev/null +++ b/tests/lib/rules-preprocessor/template-no-positive-tabindex/tabindex.ts @@ -0,0 +1,2 @@ +export const safeTabindex: 0 | -1 = 0; +export const positiveTabindex: 0 | 1 = 0; diff --git a/tests/lib/rules-preprocessor/template-no-positive-tabindex/usage.gts b/tests/lib/rules-preprocessor/template-no-positive-tabindex/usage.gts new file mode 100644 index 0000000000..2df5fd9f47 --- /dev/null +++ b/tests/lib/rules-preprocessor/template-no-positive-tabindex/usage.gts @@ -0,0 +1,2 @@ +// Placeholder file — actual code is provided inline by tests. +// Its presence lets TypeScript include this path in the program. diff --git a/tests/lib/rules/template-no-positive-tabindex.js b/tests/lib/rules/template-no-positive-tabindex.js index 560e8f8ac8..d09ea97051 100644 --- a/tests/lib/rules/template-no-positive-tabindex.js +++ b/tests/lib/rules/template-no-positive-tabindex.js @@ -177,3 +177,147 @@ hbsRuleTester.run('template-no-positive-tabindex', rule, { }, ], }); + +// Type-aware checking of dynamic values. The filename must physically exist +// so the tsconfig includes it in the TypeScript program. + +const path = require('node:path'); + +const PREPROCESSOR_DIR = path.join(__dirname, '../rules-preprocessor'); +const FIXTURE = path.join(PREPROCESSOR_DIR, 'template-no-positive-tabindex/usage.gts'); + +const ruleTesterTyped = new RuleTester({ + parser: require.resolve('ember-eslint-parser'), + parserOptions: { + project: path.join(PREPROCESSOR_DIR, 'tsconfig.eslint.json'), + tsconfigRootDir: PREPROCESSOR_DIR, + ecmaVersion: 2022, + sourceType: 'module', + extraFileExtensions: ['.gts'], + }, +}); + +ruleTesterTyped.run('template-no-positive-tabindex (with TS project)', rule, { + valid: [ + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex: 0 | -1 = 0; + +}`, + }, + { + filename: FIXTURE, + code: `export default class Foo { + get tabIndex(): -1 | '0' | undefined { return undefined; } + +}`, + }, + { + filename: FIXTURE, + code: `export default class Foo { + state = { tabIndex: -1 as const }; + +}`, + }, + { + filename: FIXTURE, + code: `import ComponentBase from './component-stub'; +export default class Foo extends ComponentBase<{ Args: { tabIndex: 0 | -1 } }> { + +}`, + }, + { + filename: FIXTURE, + code: `import { safeTabindex } from './tabindex'; +`, + }, + { + filename: FIXTURE, + code: `export const Foo = class { + tabIndex: 0 | -1 = 0; + +};`, + }, + { + filename: FIXTURE, + code: `const tabIndex = -1; +`, + }, + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex = -1 as const; + +}`, + }, + ], + invalid: [ + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex = 0; + +}`, + output: null, + errors: [{ messageId: 'mustBeNegativeNumeric' }], + }, + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex: 0 | 1 = 0; + +}`, + output: null, + errors: [{ messageId: 'positive' }], + }, + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex: 0 | -1 | boolean = 0; + +}`, + output: null, + errors: [{ messageId: 'mustBeNegativeNumeric' }], + }, + { + filename: FIXTURE, + code: `import ComponentBase from './component-stub'; +export default class Foo extends ComponentBase<{ Args: { tabIndex: number } }> { + +}`, + output: null, + errors: [{ messageId: 'mustBeNegativeNumeric' }], + }, + { + filename: FIXTURE, + code: `import { positiveTabindex } from './tabindex'; +`, + output: null, + errors: [{ messageId: 'positive' }], + }, + { + filename: FIXTURE, + code: '', + output: null, + errors: [{ messageId: 'mustBeNegativeNumeric' }], + }, + { + filename: FIXTURE, + code: `export default class Foo { + +}`, + output: null, + errors: [{ messageId: 'mustBeNegativeNumeric' }], + }, + { + filename: FIXTURE, + code: `export default class Foo { + tabIndex = 2 as const; + +}`, + output: null, + errors: [{ messageId: 'positive' }], + }, + ], +}); From 14b16f15189d580acb1285c30634e19c126c9206 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:16:58 -0400 Subject: [PATCH 2/2] fix(template-no-positive-tabindex): tighten type-aware checks - Resolve `this`/`@args` only for templates directly in a class body; nested template-only components (`static Inner =