add rule for roRegex duplicates (#119)

* add rule for roRegex duplicates

* improve roRegex rule

* adjust readme message

* use ',' as args delimiter

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
 

src/index.ts

@@ -51,6 +51,7 @@ export type BsLintConfig = Pick<BsConfig, 'project' | 'rootDir' | 'files' | 'cwd
         'color-cert'?: RuleColorCertCompliant;
         'no-assocarray-component-field-type'?: RuleSeverity;
         'no-array-component-field-type'?: RuleSeverity;
+        'no-regex-duplicates'?: RuleSeverity;
     };
     globals?: string[];
     ignores?: string[];
@@ -86,6 +87,7 @@ export interface BsLintRules {
     colorCertCompliant: RuleColorCertCompliant;
     noAssocarrayComponentFieldType: BsLintSeverity;
     noArrayComponentFieldType: BsLintSeverity;
+    noRegexDuplicates: BsLintSeverity;
 }
 
 export { Linter };

src/plugins/codeStyle/diagnosticMessages.ts

@@ -25,7 +25,8 @@ export enum CodeStyleError {
     ColorAlphaDefaults = 'LINT3022',
     ColorCertCompliant = 'LINT3023',
     NoAssocarrayFieldType = 'LINT3024',
-    NoArrayFieldType = 'LINT3025'
+    NoArrayFieldType = 'LINT3025',
+    NoRegexDuplicates = 'LINT3026'
 }
 
 const CS = 'Code style:';
@@ -215,5 +216,19 @@ export const messages = {
         severity: severity,
         source: 'bslint',
         range
+    }),
+    noIdenticalRegexInLoop: (range: Range, severity: DiagnosticSeverity) => ({
+        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
     })
 };

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 */

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<BsDiagnostic, 'file'>)[] = [];
         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<BsDiagnostic, 'file'>)[], 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;
+    }
 }
 
 /**

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'])
     };
 }