diff --git a/README.md b/README.md index 7e27449..c235c0c 100644 --- a/README.md +++ b/README.md @@ -164,7 +164,8 @@ Default rules: "consistent-return": "error", "no-assocarray-component-field-type": "off", - "no-array-component-field-type": "off" + "no-array-component-field-type": "off", + "no-regex-duplicates": "off" } } ``` @@ -251,9 +252,11 @@ Default rules: - `always`: ensures all white and black color-format values either match or are darker/ lighter than the minimum recommended values. For white the maximum value is `#EBEBEB` and for black the minimum value is `#161616` - `off`: do not validate (**default**) -- `no-assocarray-component-field-type`: Using 'assocarray' 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-assocarray-component-field-type`: Using 'assocarray' type in component markup can result in inefficient copying of data during transfer to the render thread. Use 'node' field types for more efficient transfer of data between component boundaries by avoiding expensive data cloning (`error | warn | info | off`) -- `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-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' field types for more efficient transfer of data between component boundaries by avoiding expensive data cloning(`error | warn | info | off`) + +- `no-regex-duplicates`: Avoid creating multiple roRegex objects with the same pattern as it leads to less performant code. (`error | warn | info | off`) ### Strictness rules diff --git a/src/index.ts b/src/index.ts index 7d820ce..178d172 100644 --- a/src/index.ts +++ b/src/index.ts @@ -51,6 +51,7 @@ export type BsLintConfig = Pick ({ + message: 'Avoid redeclaring identical regular expressions in a loop', + code: CodeStyleError.NoRegexDuplicates, + severity: severity, + source: 'bslint', + range + }), + noRegexRedeclaring: (range: Range, severity: DiagnosticSeverity) => ({ + message: 'Avoid redeclaring identical regular expressions', + code: CodeStyleError.NoRegexDuplicates, + severity: severity, + source: 'bslint', + range }) }; diff --git a/src/plugins/codeStyle/index.spec.ts b/src/plugins/codeStyle/index.spec.ts index 6913355..4959d8b 100644 --- a/src/plugins/codeStyle/index.spec.ts +++ b/src/plugins/codeStyle/index.spec.ts @@ -47,6 +47,7 @@ describe('codeStyle', () => { 'color-alpha': 'off', 'color-alpha-defaults': 'off', 'color-cert': 'off', + 'no-regex-duplicates': 'off', ...(rules ?? {}) } } as BsLintConfig); @@ -665,6 +666,72 @@ describe('codeStyle', () => { }); }); + describe('enforce no regex re-creation', () => { + it('within loop', () => { + init({ + 'no-regex-duplicates': 'warn' + }); + program.setFile('source/main.brs', ` + sub init() + ? type(5) + for i = 0 to 10 + CreateObject("roRegex", "test", "") + for i = 0 to 10 + if false + ? type(5) + CreateObject("roRegex", "test2", "") + end if + CreateObject("roRegex", "test3", "") + end for + end for + end sub + `); + program.validate(); + expectDiagnosticsFmt(program, [ + '05:LINT3026:Avoid redeclaring identical regular expressions in a loop', + '11:LINT3026:Avoid redeclaring identical regular expressions in a loop' + ]); + }); + + it('no error within if', () => { + init({ + 'no-regex-duplicates': 'warn' + }); + program.setFile('source/main.brs', ` + sub init() + CreateObject("roRegex", "test", "") + ? type(5) + if false + CreateObject("roRegex", "test", "") + end if + end sub + `); + program.validate(); + expectDiagnosticsFmt(program, []); + }); + + it('anonymous function', () => { + init({ + 'no-regex-duplicates': 'warn' + }); + program.setFile('source/main.brs', ` + sub init() + CreateObject("roRegex", "test", "") + someAnonFunc = sub() + CreateObject("roRegex", "test", "") + ? type(5) + CreateObject("roRegex", "test2", "") + temp = CreateObject("roRegex", "test2", "") + end sub + end sub + `); + program.validate(); + expectDiagnosticsFmt(program, ['08:LINT3026:Avoid redeclaring identical regular expressions']); + }); + + + }); + describe('color-format', () => { it('finds colors in various templateString expression styles', () => { /* eslint-disable no-template-curly-in-string */ diff --git a/src/plugins/codeStyle/index.ts b/src/plugins/codeStyle/index.ts index 7643a45..39a1786 100644 --- a/src/plugins/codeStyle/index.ts +++ b/src/plugins/codeStyle/index.ts @@ -15,7 +15,17 @@ import { isCommentStatement, AALiteralExpression, AAMemberExpression, - BrsFile + BrsFile, + isVariableExpression, + isLiteralExpression, + CallExpression, + isForEachStatement, + isForStatement, + isWhileStatement, + isIfStatement, + isFunctionExpression, + AstNode, + Expression } from 'brighterscript'; import { RuleAAComma } from '../..'; import { addFixesToEvent } from '../../textEdit'; @@ -25,7 +35,6 @@ import { messages } from './diagnosticMessages'; import { extractFixes } from './styleFixes'; export default class CodeStyle { - name: 'codeStyle'; constructor(private lintContext: PluginContext) { @@ -73,10 +82,11 @@ export default class CodeStyle { 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, noRegexDuplicates } = severity; const validatePrint = noPrint !== DiagnosticSeverity.Hint; const validateTodo = noTodo !== DiagnosticSeverity.Hint; const validateNoStop = noStop !== DiagnosticSeverity.Hint; + const validateNoRegexDuplicates = noRegexDuplicates !== DiagnosticSeverity.Hint; const validateInlineIf = inlineIfStyle !== 'off'; const validateColorFormat = (colorFormat === 'hash-hex' || colorFormat === 'quoted-numeric-hex' || colorFormat === 'never'); const disallowInlineIf = inlineIfStyle === 'never'; @@ -131,6 +141,10 @@ export default class CodeStyle { } } + if (validateNoRegexDuplicates) { + this.validateRegex(file, diagnostics, noRegexDuplicates); + } + file.ast.walk(createVisitor({ IfStatement: s => { const hasThenToken = !!s.tokens.then; @@ -214,6 +228,46 @@ export default class CodeStyle { return diagnostics; } + validateRegex(file: BrsFile, diagnostics: (Omit)[], severity: DiagnosticSeverity) { + for (const fun of file.parser.references.functionExpressions) { + const regexes = new Set(); + for (const callExpression of fun.callExpressions) { + if (!this.isCreateObject(callExpression)) { + continue; + } + + // Check if all args are literals and get them as string + const callArgs = this.getLiteralArgs(callExpression.args); + + // CreateObject for roRegex expects 3 params, + // they should be literals because only in this case we can guarante that call regex is the same + if (callArgs?.length === 3 && callArgs[0] === 'roRegex') { + const parentStatement = callExpression.findAncestor((node, cancel) => { + if (isIfStatement(node)) { + cancel.cancel(); + } else if (this.isLoop(node) || isFunctionExpression(node)) { + return true; + } + }); + + const joinedArgs = callArgs.join(); + const isRegexAlreadyExist = regexes.has(joinedArgs); + if (!isRegexAlreadyExist) { + regexes.add(joinedArgs); + } + + if (isFunctionExpression(parentStatement)) { + if (isRegexAlreadyExist) { + diagnostics.push(messages.noRegexRedeclaring(callExpression.range, severity)); + } + } else if (this.isLoop(parentStatement)) { + diagnostics.push(messages.noIdenticalRegexInLoop(callExpression.range, severity)); + } + } + } + } + } + afterFileValidate(file: BscFile) { if (this.lintContext.ignores(file)) { return; @@ -332,6 +386,27 @@ export default class CodeStyle { } return hasReturnedValue; } + + private isLoop(node: AstNode) { + return isForStatement(node) || isForEachStatement(node) || isWhileStatement(node); + } + + private isCreateObject(s: CallExpression) { + return isVariableExpression(s.callee) && s.callee.name.text.toLowerCase() === 'createobject'; + } + + private getLiteralArgs(args: Expression[]) { + const argsStringValue: string[] = []; + for (const arg of args) { + if (isLiteralExpression(arg)) { + argsStringValue.push(arg?.token?.text?.replace(/"/g, '')); + } else { + return; + } + } + + return argsStringValue; + } } /** diff --git a/src/util.ts b/src/util.ts index c71671c..be7620c 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-regex-duplicates': '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']), + noRegexDuplicates: ruleToSeverity(rules['no-regex-duplicates']) }; }