From 4de54f780afc5a0c07eee83881ade5abe5008189 Mon Sep 17 00:00:00 2001 From: akovtko Date: Mon, 29 Jul 2024 16:25:34 +0300 Subject: [PATCH] add rule for creating node child within a loop --- README.md | 2 + src/index.ts | 2 + src/plugins/codeStyle/diagnosticMessages.ts | 10 ++- src/plugins/codeStyle/index.ts | 91 ++++++++++++++++++++- src/util.ts | 6 +- 5 files changed, 106 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 7e27449..6febc52 100644 --- a/README.md +++ b/README.md @@ -255,6 +255,8 @@ Default rules: - `no-array-component-field-type`: Using 'array' type in component markup can result in inefficient copying of data during transfer to the render thread. Use 'node' type if possible for more efficient transfer of data from the task thread to the render thread (`error | warn | info | off`) +- `no-createchild-in-loop`: CreateChild should be used when you want to append a single instance of a type of node to a parent, but avoid modifiing that child after it's created. Do not use in a loop to append multiple children. Use CreateChildren() instead (`error | warn | info | off`) + ### Strictness rules - `type-annotations`: validation of presence of `as` type annotations, for function diff --git a/src/index.ts b/src/index.ts index 86af465..39bcd45 100644 --- a/src/index.ts +++ b/src/index.ts @@ -51,6 +51,7 @@ export type BsLintConfig = Pick ({ + severity, + code: CodeStyleError.NoCreateChildInLoop, + source: 'bslint', + message: 'Avoid setting a value to node field after its creation within the same loop', + range }) }; diff --git a/src/plugins/codeStyle/index.ts b/src/plugins/codeStyle/index.ts index 5f1d416..285d669 100644 --- a/src/plugins/codeStyle/index.ts +++ b/src/plugins/codeStyle/index.ts @@ -4,6 +4,12 @@ import { FunctionExpression, isBrsFile, isXmlFile, + isFunctionExpression, + isForEachStatement, + isForStatement, + isWhileStatement, + isDottedGetExpression, + isComponentType, isGroupingExpression, TokenKind, WalkMode, @@ -18,7 +24,12 @@ import { isVoidType, Statement, SymbolTypeFlag, - XmlFile + XmlFile, + isAssignmentStatement, + isDottedSetStatement, + isVariableExpression, + CallExpression, + AstNode } from 'brighterscript'; import { RuleAAComma } from '../..'; import { addFixesToEvent } from '../../textEdit'; @@ -76,7 +87,7 @@ export default class CodeStyle implements CompilerPlugin { validateBrsFile(file: BrsFile) { const diagnostics: (Omit)[] = []; const { severity } = this.lintContext; - const { inlineIfStyle, blockIfStyle, conditionStyle, noPrint, noTodo, noStop, aaCommaStyle, eolLast, colorFormat } = severity; + const { inlineIfStyle, blockIfStyle, conditionStyle, noPrint, noTodo, noStop, aaCommaStyle, eolLast, colorFormat, noCreateObjectInLoop } = severity; const validatePrint = noPrint !== DiagnosticSeverity.Hint; const validateTodo = noTodo !== DiagnosticSeverity.Hint; const validateNoStop = noStop !== DiagnosticSeverity.Hint; @@ -92,6 +103,7 @@ export default class CodeStyle implements CompilerPlugin { const validateEolLast = eolLast !== 'off'; const disallowEolLast = eolLast === 'never'; const validateColorStyle = validateColorFormat ? createColorValidator(severity) : undefined; + const validateNoCreateChildInLoop = noCreateObjectInLoop !== DiagnosticSeverity.Hint; // Check if the file is empty by going backwards from the last token, // meaning there are tokens other than `Eof` and `Newline`. @@ -133,6 +145,8 @@ export default class CodeStyle implements CompilerPlugin { } } + const createChildinLoop = new Map(); + file.ast.walk(createVisitor({ // validate function style (`function` or `sub`) FunctionExpression: (func) => { @@ -212,12 +226,85 @@ export default class CodeStyle implements CompilerPlugin { } } } + }, + CallExpression: (node) => { + if (validateNoCreateChildInLoop) { + this.findCreateChildinLoop(node, createChildinLoop); + } } }), { walkMode: WalkMode.visitAllRecursive }); + for (const [loop, candidates] of createChildinLoop) { + loop.walk(createVisitor({ + DottedSetStatement: (s) => { + const leftExpression = s.obj; + let varStr = ''; + if (isVariableExpression(leftExpression)) { + varStr = leftExpression.tokens.name.text; + } else if (isDottedGetExpression(leftExpression)) { + varStr = this.getDottedStr(leftExpression.obj); + } + + if (candidates.includes(varStr)) { + diagnostics.push(messages.NoCreateChildInLoop(s.tokens.name.location.range, noCreateObjectInLoop)); + } + } + }), { walkMode: WalkMode.visitAllRecursive }); + } + return diagnostics; } + isLoop(statement: AstNode) { + return isForStatement(statement) || isForEachStatement(statement) || isWhileStatement(statement); + } + + findCreateChildinLoop(node: CallExpression, createChildinLoop: Map) { + if (!isDottedGetExpression(node.callee) || node.callee?.tokens?.name.text.toLowerCase() !== 'createchild') { + return; + } + const objType = node.callee.obj.getType({ flags: SymbolTypeFlag.runtime }); + if (isComponentType(objType)) { + const parentLoop = node.findAncestor((node, cancel) => { + if (isFunctionExpression(node)) { + cancel.cancel(); + } else if (this.isLoop(node)) { + return true; + } + }); + if (this.isLoop(parentLoop)) { + let candidatesArray = createChildinLoop.get(parentLoop); + + if (!candidatesArray) { + candidatesArray = []; + createChildinLoop.set(parentLoop, []); + } + + let varStr = ''; + if (isAssignmentStatement(node.parent)) { + varStr = node.parent.tokens.name.text; + } else if (isDottedSetStatement(node.parent)) { + varStr = this.getDottedStr(node.parent.obj); + } + createChildinLoop.get(parentLoop).push(varStr); + } + } + } + + getDottedStr(expression: Expression) { + let currentExpression = expression; + let varStr = ''; + while (isDottedGetExpression(currentExpression)) { + varStr = `.${currentExpression.tokens.name.text}${varStr}`; + currentExpression = currentExpression.obj; + } + if (isVariableExpression(currentExpression)) { + varStr = currentExpression.tokens.name.text + varStr; + } + + return varStr; + } + afterFileValidate(event: AfterFileValidateEvent) { const { file } = event; if (this.lintContext.ignores(file)) { diff --git a/src/util.ts b/src/util.ts index 5895248..5e8082b 100644 --- a/src/util.ts +++ b/src/util.ts @@ -28,7 +28,8 @@ export function getDefaultRules(): BsLintConfig['rules'] { 'type-annotations': 'off', 'no-print': 'off', 'no-assocarray-component-field-type': 'off', - 'no-array-component-field-type': 'off' + 'no-array-component-field-type': 'off', + 'no-createchild-in-loop': 'off' }; } @@ -166,7 +167,8 @@ function rulesToSeverity(rules: BsLintConfig['rules']) { colorAlphaDefaults: rules['color-alpha-defaults'], colorCertCompliant: rules['color-cert'], noAssocarrayComponentFieldType: ruleToSeverity(rules['no-assocarray-component-field-type']), - noArrayComponentFieldType: ruleToSeverity(rules['no-array-component-field-type']) + noArrayComponentFieldType: ruleToSeverity(rules['no-array-component-field-type']), + noCreateObjectInLoop: ruleToSeverity(rules['no-createchild-in-loop']) }; }