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..68fe84d5e8 100644 --- a/lib/rules/template-no-positive-tabindex.js +++ b/lib/rules/template-no-positive-tabindex.js @@ -1,91 +1,106 @@ -/** - * 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, e.g. `0 | -1`. null/undefined omits the attribute, or + * renders `tabindex=""` inside quotes, which browsers ignore (the same as a + * missing `{{if}}` branch). + */ +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, hash } = 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 arguments is a helper call, which is not verifiably safe + return params.length > 0 || hash?.pairs.length > 0 + ? 'mustBeNegativeNumeric' + : getExpressionViolation(path, getPathType); + } + // tabindex="{{-1}}". Multiple parts are concatenated (e.g. "{{-1}}5" + // renders "-15"), so only a single mustache is verifiably safe. + case 'GlimmerConcatStatement': { + const parts = attrValue.parts ?? []; + return parts.length === 1 && parts[0].type === 'GlimmerMustacheStatement' + ? getTabindexViolation(parts[0], getPathType) + : 'mustBeNegativeNumeric'; + } + default: { return 'mustBeNegativeNumeric'; } } - - return null; } /** @type {import('eslint').Rule.RuleModule} */ @@ -113,7 +128,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 +140,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..dd8989f1c4 --- /dev/null +++ b/lib/utils/glimmer-path-type.js @@ -0,0 +1,90 @@ +'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 class that owns the current template), 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(); + // The class whose body directly contains each open