Skip to content

Commit

Permalink
fix(eslint-plugin-template): conditional-complexity not reporting all…
Browse files Browse the repository at this point in the history 
… cases (#279)
  • Loading branch information
@rafaelss95
rafaelss95 committed Jan 11, 2021
1 parent 49ab76a commit a4fd077
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 92 deletions.
105 changes: 69 additions & 36 deletions packages/eslint-plugin-template/src/rules/conditional-complexity.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import {
AST,
ASTWithSource,
Binary,
BindingPipe,
Conditional,
Interpolation,
Lexer,
Parser,
TmplAstBoundAttribute,
Expand All @@ -12,7 +16,6 @@ import {
} from '../utils/create-eslint-rule';

type Options = [{ maxComplexity: number }];

export type MessageIds = 'conditionalСomplexity';
export const RULE_NAME = 'conditional-complexity';

Expand All @@ -21,37 +24,40 @@ export default createESLintRule<Options, MessageIds>({
meta: {
type: 'suggestion',
docs: {
description: `The condition complexity shouldn't exceed a rational limit in a template.`,
description:
'The conditional complexity should not exceed a rational limit',
category: 'Best Practices',
recommended: false,
},
schema: [
{
type: 'object',
properties: {
maxComplexity: {
type: 'number',
minimum: 1,
},
},
properties: { maxComplexity: { minimum: 1, type: 'number' } },
additionalProperties: false,
},
],
messages: {
conditionalСomplexity:
'The condition complexity "{{totalComplexity}}" exceeds the defined limit "{{maxComplexity}}"',
'The conditional complexity "{{totalComplexity}}" exceeds the defined limit "{{maxComplexity}}"',
},
},
defaultOptions: [{ maxComplexity: 5 }],
create(context, [{ maxComplexity }]) {
const sourceCode = context.getSourceCode();
const parserServices = getTemplateParserServices(context);

return {
'BoundAttribute[name="ngIf"]'({
BoundAttribute({
value: { source },
sourceSpan,
value,
}: TmplAstBoundAttribute) {
const totalComplexity = getTotalComplexity(value as ASTWithSource);
}: TmplAstBoundAttribute & { value: ASTWithSource }) {
if (!source) return;

const rawSource = source.replace(/\s/g, '');
const possibleBinary = extractPossibleBinaryOrConditionalFrom(
getParser().parseBinding(rawSource, null, 0).ast,
);
const totalComplexity = getTotalComplexity(possibleBinary);

if (totalComplexity <= maxComplexity) return;

Expand All @@ -63,46 +69,73 @@ export default createESLintRule<Options, MessageIds>({
data: { maxComplexity, totalComplexity },
});
},
Interpolation({ expressions }: Interpolation) {
for (const expression of expressions) {
const totalComplexity = getTotalComplexity(expression);

if (totalComplexity <= maxComplexity) continue;

const {
sourceSpan: { end, start },
} = expression;
const loc = {
end: sourceCode.getLocFromIndex(end),
start: sourceCode.getLocFromIndex(start),
} as const;

context.report({
loc,
messageId: 'conditionalСomplexity',
data: { maxComplexity, totalComplexity },
});
}
},
};
},
});

function extractPossibleBinaryOrConditionalFrom(node: AST): AST {
return node instanceof BindingPipe ? node.exp : node;
}

let parser: Parser | null = null;
// Instantiate the `Parser` class lazily only when this rule is applied.
function getParser(): Parser {
return parser || (parser = new Parser(new Lexer()));
}

function getTotalComplexity({ source }: ASTWithSource): number {
const expression = source?.replace(/\s/g, '') ?? '';
const parser = getParser();
const astWithSource = parser.parseAction(expression, null, 0);
const conditions: Binary[] = [];
let totalComplexity = 0;
let condition = astWithSource.ast as Binary;

if (condition.operation) {
totalComplexity++;
conditions.push(condition);
function getTotalComplexity(ast: AST): number {
const possibleBinaryOrConditional = extractPossibleBinaryOrConditionalFrom(
ast,
);

if (
!(
possibleBinaryOrConditional instanceof Binary ||
possibleBinaryOrConditional instanceof Conditional
)
) {
return 0;
}

while (conditions.length > 0) {
const condition = conditions.pop()!;
let total = 1;

if (!condition.operation) {
continue;
if (possibleBinaryOrConditional instanceof Binary) {
if (possibleBinaryOrConditional.left instanceof Binary) {
total += getTotalComplexity(possibleBinaryOrConditional.left);
}

if (condition.left instanceof Binary) {
totalComplexity++;
conditions.push(condition.left);
if (possibleBinaryOrConditional.right instanceof Binary) {
total += getTotalComplexity(possibleBinaryOrConditional.right);
}
}

if (condition.right instanceof Binary) {
totalComplexity++;
conditions.push(condition.right);
}
if (possibleBinaryOrConditional instanceof Conditional) {
total +=
getTotalComplexity(possibleBinaryOrConditional.condition) +
getTotalComplexity(possibleBinaryOrConditional.trueExp) +
getTotalComplexity(possibleBinaryOrConditional.falseExp);
}

return totalComplexity;
return total;
}
9 changes: 5 additions & 4 deletions packages/eslint-plugin-template/src/utils/create-eslint-rule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ export function getTemplateParserServices(context: any): ParserServices {
* If @angular-eslint/template-parser is not the configured parser when the function is invoked it will throw
*/
export function ensureTemplateParser(
context: TSESLint.RuleContext<string, []>,
context: TSESLint.RuleContext<string, readonly unknown[]>,
) {
if (
!context.parserServices ||
!(context.parserServices as any).convertNodeSourceSpanToLoc ||
(!context.parserServices as any).convertElementSourceSpanToLoc
!((context.parserServices as unknown) as ParserServices)
?.convertNodeSourceSpanToLoc ||
!((context.parserServices as unknown) as ParserServices)
?.convertElementSourceSpanToLoc
) {
/**
* The user needs to have configured "parser" in their eslint config and set it
Expand Down
113 changes: 61 additions & 52 deletions packages/eslint-plugin-template/tests/rules/conditional-complexity.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,91 +14,100 @@ import rule, {
const ruleTester = new RuleTester({
parser: '@angular-eslint/template-parser',
});

const messageId: MessageIds = 'conditionalСomplexity';

ruleTester.run(RULE_NAME, rule, {
valid: [
{
code: `
<div *ngIf="a === '1' || b === '2' && c.d !== e">
Enter your card details
</div>
`,
},
{
code: `
<div *ngIf="a === '1' || (b === '2' && c.d !== e)">
Enter your card details
</div>
`,
},
{
code: `
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
Enter your card details
</div>
`
<div *ngIf="a === '1' || b === '2' && c.d !== e">Content</div>
<div *ngIf="isValid; then thenTemplateRef; else elseTemplateRef">Content</div>
<ng-template #thenTemplateRef>thenTemplateRef</ng-template>
<ng-template #elseTemplateRef>elseTemplateRef</ng-template>
<div [class.mw-100]="test === 7"></div>
<div [attr.aria-label]="testing === 'ab' ? 'bc' : 'de'"></div>
<div [attr.custom-attr]="'test345' | appPipe"></div>
`,
options: [{ maxComplexity: 9 }],
},
{
code: `
<div *ngIf="(b === '3' && c.d !== '1' && e.f !== '6' && q !== g) || a === '3'">
Enter your card details
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
Content
</div>
`,
options: [{ maxComplexity: 9 }],
},
{
code: `
<div *ngIf="isValid; then thenBlock; else elseBlock">
Enter your card details
</div>
<ng-template #thenBlock>
thenBlock
</ng-template>
<ng-template #elseBlock>
elseBlock
</ng-template>
`,
},
],
invalid: [
convertAnnotatedSourceToFailureCase({
messageId,
description: 'should fail with a higher level of complexity',
description:
'should fail if the conditional complexity exceeds the maximum default',
annotatedSource: `
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter your card details
Content
</div>
`,
data: { maxComplexity: 5, totalComplexity: 9 },
}),
convertAnnotatedSourceToFailureCase({
messageId,
description:
'should fail if the conditional complexity exceeds the maximum defined',
annotatedSource: `
<ng-container *ngIf="control && control.invalid && (control.touched || (showOnDirty && control.dirty))">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Content
</ng-container>
`,
data: { maxComplexity: 3, totalComplexity: 4 },
options: [{ maxComplexity: 3 }],
}),
convertAnnotatedSourceToFailureCase({
messageId,
description:
'should fail if the conditional complexity exceeds the maximum defined when using pipes to create a variable',
annotatedSource: `
<ng-container *ngIf="control && control.invalid && (control.touched || (showOnDirty && control.dirty)) && control.errors | keys as errorKeys">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
{{ errorKeys }}
</ng-container>
`,
data: { maxComplexity: 2, totalComplexity: 5 },
options: [{ maxComplexity: 2 }],
}),
convertAnnotatedSourceToFailureCase({
messageId,
description:
'should fail if the conditional complexity exceeds the maximum default when using [class] binding',
annotatedSource: `
<div [class.px-4]="a <= b || (b > c && c >= d && d < e)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Content
</div>
`,
data: { maxComplexity: 5, totalComplexity: 7 },
}),
convertAnnotatedSourceToFailureCase({
messageId,
description:
'should fail with a higher level of complexity and a carrier return',
'should fail if the conditional complexity exceeds the maximum default when using nested conditionals',
annotatedSource: `
<div *ngIf="a === '3' || (b === '3' && c.d !== '1'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
&& e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter your card details
<div [class.my-2]="a === 'x' ? v === c : b === 3 ? 0 : c > 3 && d === 9 ? 9 : 'xa'">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Content
</div>
`,
data: { maxComplexity: 5, totalComplexity: 9 },
}),
convertAnnotatedSourceToFailureCase({
messageId,
description:
'should fail with a higher level of complexity with ng-template',
'should fail if the conditional complexity exceeds the maximum default when using conditionals within interpolation',
annotatedSource: `
<ng-template [ngIf]="a === '3' || (b === '3' && c.d !== '1'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
&& e.f !== '6' && q !== g && x === '1')">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter details
</ng-template>
{{ test.xyz }} {{ ab > 2 && cd === 9 && control?.invalid && (control.touched || (showOnDirty && control.dirty)) ? 'some value' : 'another value' }} {{ control.touched }}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
`,
data: { maxComplexity: 5, totalComplexity: 8 },
}),
],
});

0 comments on commit a4fd077

Please sign in to comment.