diff --git a/its/ruling/src/test/expected/ant-design/typescript-S6767.json b/its/ruling/src/test/expected/ant-design/typescript-S6767.json index c06bb435b37..7464cddce96 100644 --- a/its/ruling/src/test/expected/ant-design/typescript-S6767.json +++ b/its/ruling/src/test/expected/ant-design/typescript-S6767.json @@ -1,7 +1,4 @@ { -"ant-design:components/anchor/Anchor.tsx": [ -70 -], "ant-design:components/button/LoadingIcon.tsx": [ 41, 41 diff --git a/packages/analysis/src/jsts/rules/S6767/decorator-indirect-prop-usage.ts b/packages/analysis/src/jsts/rules/S6767/decorator-indirect-prop-usage.ts index 7227678ffef..c9c24e4d15f 100644 --- a/packages/analysis/src/jsts/rules/S6767/decorator-indirect-prop-usage.ts +++ b/packages/analysis/src/jsts/rules/S6767/decorator-indirect-prop-usage.ts @@ -15,184 +15,163 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ +import type { TSESTree } from '@typescript-eslint/utils'; import type { Rule, Scope, SourceCode } from 'eslint'; import type estree from 'estree'; import { getNodeParent } from '../helpers/ancestor.js'; import { isFunctionNode, isIdentifier } from '../helpers/ast.js'; import { isRequiredParserServices } from '../helpers/parser-services.js'; -import { - getComponentIdentifier, - getComponentPropsType, -} from '../helpers/react/component-analysis.js'; +import { getComponentPropsType, getComponentVariable } from '../helpers/react.js'; import { areSameTypeDeclarations, getTypeFromTreeNode } from '../helpers/type.js'; import { isSupportedWholePropsUsage } from './whole-props-usage.js'; /** - * Returns true when the specific reported prop is consumed through a decorator - * factory callback typed with the same props contract as the component. + * False-positive remediation escape: + * returns true when the specific reported prop is consumed through one of these + * decorator-specific indirect patterns: * * track((props: ComponentProps) => props.propName)(Component); + * + * @track((props: ComponentProps) => props.propName) + * class Component extends React.Component {} */ export function hasDecoratorPropUsage( componentNode: estree.Node, context: Rule.RuleContext, propName: string | undefined, ): boolean { + if (!propName) { + return false; + } + return ( - !!propName && hasDecoratorFactoryCallPropUsage(context.sourceCode, componentNode, propName) + hasDecoratorFactoryCallPropUsage(context.sourceCode, componentNode, propName) || + hasDecoratorAnnotationPropUsage(context.sourceCode, componentNode, propName) ); } +/** + * Pseudo code for the matched AST: + * track( + * (props: ComponentProps) => { + * props.propName; + * return buildPayload(props); + * }, + * )(Component); + * + * The decorated target must be the component reference passed as the sole outer + * call argument. + */ function hasDecoratorFactoryCallPropUsage( sourceCode: SourceCode, componentNode: estree.Node, propName: string, ): boolean { - const componentIdentifier = getComponentIdentifier(componentNode); - const componentVariable = - componentIdentifier && getVariableForIdentifier(sourceCode, componentIdentifier); + const componentVariable = getComponentVariable(sourceCode, componentNode); if (!componentVariable) { return false; } - return componentVariable.references.some(reference => - isDecoratorFactoryTargetReference(sourceCode, componentNode, reference, propName), - ); + return componentVariable.references.some(reference => { + const decoratorApplication = getNodeParent(reference.identifier); + return ( + decoratorApplication?.type === 'CallExpression' && + decoratorApplication.arguments.length === 1 && + decoratorApplication.arguments[0] === reference.identifier && + decoratorApplication.callee.type === 'CallExpression' && + decoratorApplication.callee.arguments.some(argument => + isComponentPropsCallbackUsingProp(sourceCode, componentNode, argument, propName), + ) + ); + }); } /** - * Pseudo code: - * track(callback)(Component) - * - * The matched component reference must be the only outer application argument. + * Pseudo code for the matched AST: + * @track( + * (props: ComponentProps) => { + * props.propName; + * return buildPayload(props); + * }, + * ) + * class Component extends React.Component {} */ -function isDecoratorFactoryTargetReference( +function hasDecoratorAnnotationPropUsage( sourceCode: SourceCode, componentNode: estree.Node, - reference: Scope.Reference, propName: string, ): boolean { - const decoratorApplication = getDecoratorApplicationForReference(reference); - return ( - !!decoratorApplication && - hasMatchingDecoratorCallbackArgument( - sourceCode, - componentNode, - decoratorApplication.callee, - propName, - ) - ); -} - -/** - * Pseudo code: - * track(callback)(Component) - * - * Returns the outer application call when the reference is the only target argument. - */ -function getDecoratorApplicationForReference( - reference: Scope.Reference, -): (estree.CallExpression & { callee: estree.CallExpression }) | undefined { - const parent = getNodeParent(reference.identifier); - return isDecoratorApplicationCall(parent, reference.identifier) ? parent : undefined; -} + const componentTsNode = componentNode as TSESTree.Node; + if (componentTsNode.type !== 'ClassDeclaration' && componentTsNode.type !== 'ClassExpression') { + return false; + } -function isDecoratorApplicationCall( - node: estree.Node | undefined, - targetNode: estree.Identifier, -): node is estree.CallExpression & { callee: estree.CallExpression } { return ( - node?.type === 'CallExpression' && - node.arguments.length === 1 && - node.arguments[0] === targetNode && - node.callee.type === 'CallExpression' - ); -} - -/** - * Pseudo code: - * track( - * (props: ComponentProps) => { - * props.propName; - * }, - * )(Component) - * - * At least one inner call argument must be a callback whose first parameter is typed - * with the same declared props contract as the component and uses the reported prop. - */ -function hasMatchingDecoratorCallbackArgument( - sourceCode: SourceCode, - componentNode: estree.Node, - decoratorFactoryCall: estree.CallExpression, - propName: string, -): boolean { - return decoratorFactoryCall.arguments.some(argument => - isComponentPropsCallbackUsingProp(sourceCode, componentNode, argument, propName), + componentTsNode.decorators?.some(({ expression }) => + isDecoratorCallUsingProp(expression, sourceCode, componentNode, propName), + ) ?? false ); } -function isComponentPropsCallbackUsingProp( +function isDecoratorCallUsingProp( + expression: TSESTree.Node, sourceCode: SourceCode, componentNode: estree.Node, - callbackNode: estree.Node, propName: string, ): boolean { - const propsVariable = getMatchingCallbackPropsVariable(sourceCode, componentNode, callbackNode); return ( - !!propsVariable && - propsVariable.references.some(reference => isComponentPropsUsageReference(reference, propName)) + expression.type === 'CallExpression' && + expression.arguments.some(argument => + isComponentPropsCallbackUsingProp( + sourceCode, + componentNode, + argument as estree.Node, + propName, + ), + ) ); } /** - * Pseudo code: - * (props: ComponentProps) => ... + * Pseudo code for the matched AST: + * (props: ComponentProps) => { + * props.propName; + * return buildPayload(props); + * } * - * The first callback parameter must be an identifier typed with the same declared props - * contract as the component. When that holds, return its scope variable. + * The callback first parameter must be provably the same component props type, + * including the same generic instantiation, not just any object with a matching + * alias symbol. */ -function getMatchingCallbackPropsVariable( +function isComponentPropsCallbackUsingProp( sourceCode: SourceCode, componentNode: estree.Node, callbackNode: estree.Node, -): Scope.Variable | undefined { + propName: string, +): boolean { if (!isFunctionNode(callbackNode)) { - return undefined; + return false; } const firstParam = callbackNode.params[0]; - if ( - !isIdentifier(firstParam) || - !isSameDeclaredPropsType(sourceCode, componentNode, firstParam) - ) { - return undefined; + if (!isIdentifier(firstParam)) { + return false; + } + if (!isSameDeclaredPropsType(sourceCode, componentNode, firstParam)) { + return false; } - return sourceCode.getScope(callbackNode).set.get(firstParam.name); -} - -function getVariableForIdentifier( - sourceCode: SourceCode, - identifier: estree.Identifier, -): Scope.Variable | undefined { - let scope: Scope.Scope | null = sourceCode.getScope(identifier); - while (scope) { - const reference = scope.references.find(candidate => candidate.identifier === identifier); - if (reference) { - return reference.resolved ?? undefined; - } - - const variable = scope.variables.find(candidate => - candidate.defs.some(definition => definition.name === identifier), - ); - if (variable) { - return variable; - } - - scope = scope.upper; + const variable = sourceCode.getScope(callbackNode).set.get(firstParam.name); + if (!variable) { + return false; } - return undefined; + // Unlike forwardRef closures, decorator callbacks introduce their own props + // parameter instead of closing over the component's original one. That means + // the rule-level whole-props remediation does not naturally observe callback + // usages such as `buildPayload(props)`, so we re-apply the same whole-props + // shapes against this callback parameter here. + return variable.references.some(reference => isComponentPropsUsageReference(reference, propName)); } function isSameDeclaredPropsType( @@ -204,48 +183,37 @@ function isSameDeclaredPropsType( if (!isRequiredParserServices(services)) { return false; } - const checker = services.program.getTypeChecker(); + const componentPropsType = getComponentPropsType(componentNode, services); const callbackPropsType = getTypeFromTreeNode(callbackPropsParam, services); + // Keep the escape conservative when either side collapses to top-level `any` + // or `unknown`: without a declared props shape, we cannot prove equivalence. return areSameTypeDeclarations(checker, callbackPropsType, componentPropsType); } +/** + * Pseudo code for the matched AST patterns: + * props.propName; + * + * buildPayload(props); + * + * The whole-props handoff must also pass the ignore filter from `whole-props-usage.ts`. + */ function isComponentPropsUsageReference(reference: Scope.Reference, propName: string): boolean { const parent = getNodeParent(reference.identifier); - return ( - isDirectPropMemberAccess(parent, reference.identifier, propName) || - isWholePropsForwardingUsage(parent, reference.identifier) - ); -} + if (!parent) { + return false; + } -/** - * Pseudo code: - * props.propName - */ -function isDirectPropMemberAccess( - node: estree.Node | undefined, - propsIdentifier: estree.Identifier, - propName: string, -): boolean { - return ( - node?.type === 'MemberExpression' && - node.object === propsIdentifier && - !node.computed && - isIdentifier(node.property, propName) - ); -} + if ( + parent.type === 'MemberExpression' && + parent.object === reference.identifier && + !parent.computed && + isIdentifier(parent.property, propName) + ) { + return true; + } -/** - * Pseudo code: - * consume(props) - * [...props] - * - * props[key] - */ -function isWholePropsForwardingUsage( - node: estree.Node | undefined, - propsIdentifier: estree.Identifier, -): boolean { - return !!node && isSupportedWholePropsUsage(node, argument => argument === propsIdentifier); + return isSupportedWholePropsUsage(parent, argument => argument === reference.identifier); } diff --git a/packages/analysis/src/jsts/rules/S6767/decorator.ts b/packages/analysis/src/jsts/rules/S6767/decorator.ts index ef2f2fa5f4b..5b8ea75544e 100644 --- a/packages/analysis/src/jsts/rules/S6767/decorator.ts +++ b/packages/analysis/src/jsts/rules/S6767/decorator.ts @@ -25,15 +25,34 @@ import { hasOwnCustomSuperclassPropsForwarding } from './custom-superclass-forwa import { hasDecoratorPropUsage } from './decorator-indirect-prop-usage.js'; import { hasForwardRefCallbackPropUsage } from './forward-ref-indirect-prop-usage.js'; import * as meta from './generated-meta.js'; +import { hasOnlyNonPropsReportedTypeUsage } from './reported-type-non-props-usage.js'; import { hasSupportedWholePropsUsage } from './whole-props-usage.js'; +const reportedDescriptorsBySourceCode = new WeakMap>(); +type ComponentNodeSuppressor = (componentNode: estree.Node) => boolean; + function allMatch( componentNodes: estree.Node[], predicate: (componentNode: estree.Node) => boolean, -) { +): boolean { return componentNodes.length > 0 && componentNodes.every(predicate); } +function shouldSuppressForAllComponentOwners( + componentNodes: estree.Node[], + context: Rule.RuleContext, + propName: string | undefined, +): boolean { + const suppressors: ComponentNodeSuppressor[] = [ + componentNode => hasSupportedWholePropsUsage(componentNode, context), + componentNode => hasOwnCustomSuperclassPropsForwarding(componentNode), + componentNode => hasForwardRefCallbackPropUsage(componentNode, context, propName), + componentNode => hasDecoratorPropUsage(componentNode, context, propName), + ]; + + return suppressors.some(suppressor => allMatch(componentNodes, suppressor)); +} + export function decorate(rule: Rule.RuleModule): Rule.RuleModule { return interceptReportForReact( { ...rule, meta: generateMeta(meta, rule.meta) }, @@ -41,36 +60,37 @@ export function decorate(rule: Rule.RuleModule): Rule.RuleModule { const { node } = descriptor as { node: estree.Node }; const { data } = descriptor as { data?: Record }; const propName = data?.name; - const componentNodes = findComponentNodes(node, context); - if ( - allMatch(componentNodes, componentNode => - hasSupportedWholePropsUsage(componentNode, context), - ) - ) { - return; - } - if ( - allMatch(componentNodes, componentNode => - hasOwnCustomSuperclassPropsForwarding(componentNode), - ) - ) { + + if (hasOnlyNonPropsReportedTypeUsage(node, context)) { return; } - if ( - allMatch(componentNodes, componentNode => - hasForwardRefCallbackPropUsage(componentNode, context, propName), - ) - ) { + + const componentNodes = findComponentNodes(node, context); + if (shouldSuppressForAllComponentOwners(componentNodes, context, propName)) { return; } - if ( - allMatch(componentNodes, componentNode => - hasDecoratorPropUsage(componentNode, context, propName), - ) - ) { + + const reportedDescriptors = getReportedDescriptors(context.sourceCode); + const reportKey = getReportKey(node, propName); + if (reportedDescriptors.has(reportKey)) { return; } + reportedDescriptors.add(reportKey); context.report(descriptor); }, ); } + +function getReportedDescriptors(sourceCode: Rule.RuleContext['sourceCode']): Set { + let reportedDescriptors = reportedDescriptorsBySourceCode.get(sourceCode); + if (!reportedDescriptors) { + reportedDescriptors = new Set(); + reportedDescriptorsBySourceCode.set(sourceCode, reportedDescriptors); + } + return reportedDescriptors; +} + +function getReportKey(node: estree.Node, propName: string | undefined): string { + const [start = -1, end = -1] = node.range ?? []; + return `${start}:${end}:${propName ?? ''}`; +} diff --git a/packages/analysis/src/jsts/rules/S6767/reported-type-non-props-usage.ts b/packages/analysis/src/jsts/rules/S6767/reported-type-non-props-usage.ts new file mode 100644 index 00000000000..520ee686009 --- /dev/null +++ b/packages/analysis/src/jsts/rules/S6767/reported-type-non-props-usage.ts @@ -0,0 +1,32 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ + +import type { Rule } from 'eslint'; +import type estree from 'estree'; +import { hasOnlyReactClassNonPropsReportedTypeUsage } from '../helpers/react.js'; + +/** + * False-positive remediation escape: + * returns true when the reported type or reported member is owned only through + * React class non-props slots such as `state` / `snapshot`. + */ +export function hasOnlyNonPropsReportedTypeUsage( + node: estree.Node, + context: Rule.RuleContext, +): boolean { + return hasOnlyReactClassNonPropsReportedTypeUsage(node, context); +} diff --git a/packages/analysis/src/jsts/rules/S6767/unit.test.ts b/packages/analysis/src/jsts/rules/S6767/unit.test.ts index 9f34716b09c..109d8ddfdb6 100644 --- a/packages/analysis/src/jsts/rules/S6767/unit.test.ts +++ b/packages/analysis/src/jsts/rules/S6767/unit.test.ts @@ -15,10 +15,84 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ import { rule } from './index.js'; +import { buildBabelParserOptions } from '../../parsers/options.js'; import { NoTypeCheckingRuleTester } from '../../../../tests/jsts/tools/testers/rule-tester.js'; +import parser from '@babel/eslint-parser'; +import { Linter } from 'eslint'; import { describe, it } from 'node:test'; +import { expect } from 'expect'; describe('S6767', () => { + it('should report decorator-factory callbacks without provable props', () => { + const ruleTester = new NoTypeCheckingRuleTester(); + + ruleTester.run('no-unused-prop-types', rule, { + valid: [], + invalid: [ + { + // TP: untyped decorator callback parameter might not be component props + code: ` +function decorate(metadataMapper) { + return function applyDecorator(target) { + return target; + }; +} +function DecoratedComponent(props) { + return
; +} +DecoratedComponent.propTypes = { + contextModule: PropTypes.string, +}; +decorate(function (metadata) { + return { + context_module: metadata.contextModule, + }; +})(DecoratedComponent); +`, + errors: 1, + }, + { + // TP: decorator factory receives no callback that can consume props + code: ` +function track(config) { + return function applyDecorator(target) { + return target; + }; +} +function DecoratorConfigComponent(props) { + return
; +} +DecoratorConfigComponent.propTypes = { + contextModule: PropTypes.string, +}; +track({ event: 'view' })(DecoratorConfigComponent); +`, + errors: 1, + }, + { + // TP: destructured callback parameter is not tracked as whole props usage + code: ` +function track(mapper) { + return function applyDecorator(target) { + return target; + }; +} +function DecoratorDestructureComponent(props) { + return
; +} +DecoratorDestructureComponent.propTypes = { + contextModule: PropTypes.string, +}; +track(({ contextModule }) => ({ + context_module: contextModule, +}))(DecoratorDestructureComponent); +`, + errors: 1, + }, + ], + }); + }); + it('should not report props passed wholesale to a helper function', () => { const ruleTester = new NoTypeCheckingRuleTester(); @@ -120,6 +194,34 @@ class Button extends React.Component { return ; } } +`, + errors: 1, + }, + { + // TP: anonymous class component cannot be resolved for decorator usage + code: ` +export default class extends React.Component { + static propTypes = { + color: PropTypes.string, + }; + render() { + return