FIX @W-19772057@ Refactored selection algorithm

packages/code-analyzer-core/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@salesforce/code-analyzer-core",
   "description": "Core Package for the Salesforce Code Analyzer",
-  "version": "0.38.0",
+  "version": "0.38.1-SNAPSHOT",
   "author": "The Salesforce Code Analyzer Team",
   "license": "BSD-3-Clause",
   "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",
@@ -72,4 +72,4 @@
       "!src/index.ts"
     ]
   }
-}
+}

packages/code-analyzer-core/src/messages.ts

@@ -167,7 +167,10 @@ const MESSAGE_CATALOG : MessageCatalog = {
         `Rule selectors can't be empty strings.`,
 
     SelectorLooksIncorrect:
-        `Rule selector '%s' looks incorrect. Make sure that parentheses are balanced and that all subselectors are joined by a colon (:) or comma (,).`,
+        `Rule selector '%s' looks incorrect. Make sure that the expression contains no whitespace, the parentheses are balanced, and that all subselectors are joined by a colon (:) or comma (,).`,
+
+    SelectorStartsOrEndsWithOperator:
+        `Rule selectors should not start or end with an operator. Selector: '%s'`,
 
     EngineRunResultsMissing:
         `Couldn't get results for engine '%s' since they're missing from the overall run results. Most likely the engine didn't run.`,

packages/code-analyzer-core/src/selectors.ts

@@ -5,81 +5,72 @@ export interface Selector {
 }
 
 export function toSelector(selectorString: string): Selector {
-    // We parse the selector back-to-front, so that the front-most selectors end up at the bottom of the tree we create
-    // and therefore get resolved first.
     if (selectorString === '') {
         // ERROR CASE: The selector is empty. Possible if you do something like "()" or "a:()".
         throw new Error(getMessage("SelectorCannotBeEmpty"));
-    } else if (selectorString.endsWith(')')) {
-        // If the selector ends in close-paren, then we need to find the open-paren that matches it.
-        const correspondingOpenParen: number = identifyCorrespondingOpenParen(selectorString);
-        if (correspondingOpenParen === 0) {
-            // RECURSIVE CASE: The entire selector is wrapped in parens. Pop them off and call recursively.
-            return toSelector(selectorString.slice(1, -1))
-        } else {
-            // RECURSIVE CASE: The open-paren is somewhere in the middle of the selector and accompanied by an operator.
-            const left: string = selectorString.slice(0, correspondingOpenParen - 1);
-            const right: string = selectorString.slice(correspondingOpenParen);
-            const op: string = selectorString[correspondingOpenParen - 1];
-            return toComplexSelector(left, right, op);
-        }
-    } else {
-        // If there's a close-paren in the string, only look for operators after it.
-        const lastCloseParen: number = Math.max(selectorString.lastIndexOf(')'), 0);
-        const lastComma: number = selectorString.slice(lastCloseParen).lastIndexOf(',');
-        const lastColon: number = selectorString.slice(lastCloseParen).lastIndexOf(':');
-
-        // BASE CASE: The selector contains no commas or colons.
-        if (lastComma === -1 && lastColon === -1) {
-            // Parens only make sense in conjunction with operators, so if we find any, the selector is malformed.
-            if (selectorString.includes(')') || selectorString.includes('(')) {
-                throw new Error(getMessage('SelectorLooksIncorrect', selectorString));
-            }
-            return new SimpleSelector(selectorString);
-        } else if (lastComma !== -1) {
-            // Commas resolve before colons, so that "x,a:b" and "a:b,x" both resolve equivalently the combination of
-            // "x" and "a:b".
-            const left: string = selectorString.slice(0, lastComma + lastCloseParen);
-            const right: string = selectorString.slice(lastComma + lastCloseParen + 1);
-            return toComplexSelector(left, right, ',');
-        } else {
-            const left: string = selectorString.slice(0, lastColon + lastCloseParen);
-            const right: string = selectorString.slice(lastColon + lastCloseParen + 1);
-            return toComplexSelector(left, right, ':');
-        }
     }
-}
 
-function identifyCorrespondingOpenParen(selectorString: string): number {
-    const reversedLetters: string[] = selectorString.split('').reverse();
+    let commaIdx: number|null = null;
+    let colonIdx: number|null = null;
     let parenBalance: number = 0;
-    let idx = 0;
-    for (const letter of reversedLetters) {
-        if (letter === ')') {
+    let hasParens: boolean = false;
+    for (let i = 0; i < selectorString.length; i++) {
+        const char: string = selectorString[i];
+        if ((i === 0 || i === selectorString.length - 1) && (char === ',' || char === ':')) {
+            // ERROR CASE: The selector cannot start or end with a binary operator, because that's nonsense.
+            throw new Error(getMessage("SelectorStartsOrEndsWithOperator", selectorString));
+        } else if (char === ' ') {
+            // ERROR CASE: The selector contains whitespace.
+            throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
+        } else if (char === '(') {
             parenBalance += 1;
-        } else if (letter === '(') {
+            hasParens = true;
+        } else if (char === ')') {
             parenBalance -= 1;
+            hasParens = true;
+            // If our parenthesis balance is negative, it means there are more close-parens than open-parens, which is a problem.
+            if (parenBalance < 0) {
+                throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
+            }
+        } else if (char === ',') {
+            // If we're not inside of parentheses, and we haven't already found a comma, note the location of this one.
+            if (parenBalance === 0) {
+                commaIdx = i;
+                // Commas trump everything else, so we can just break.
+                break;
+            }
+        } else if (char === ':') {
+            // If we're not inside of parentheses, and we haven't already found a colon, note the location of this one.
+            if (parenBalance === 0 && colonIdx === null) {
+                colonIdx = i;
+            }
         }
-        if (parenBalance === 0) {
-            break;
-        }
-        idx += 1;
     }
 
+    // If our final parenthesis balance is negative, it means there are more open-parens than close-parens, which is a problem.
     if (parenBalance > 0) {
         throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
     }
 
-    return selectorString.length - idx - 1;
-}
-
-function toComplexSelector(left: string, right: string, op: string): Selector {
-    if (op === ',') {
+    // Commas trump colons, so if we have a comma, split along that.
+    if (commaIdx != null) {
+        const left: string = selectorString.slice(0, commaIdx);
+        const right: string = selectorString.slice(commaIdx + 1);
         return new OrSelector(toSelector(left), toSelector(right));
-    } else if (op === ':') {
+    } else if (colonIdx != null) {
+        // If there are colons but no commas, split along the first colon.
+        const left: string = selectorString.slice(0, colonIdx);
+        const right: string = selectorString.slice(colonIdx + 1);
         return new AndSelector(toSelector(left), toSelector(right));
+    } else if (selectorString[0] === '(' && selectorString[selectorString.length - 1] === ')') {
+        // If the first and last character are parentheses, then pop those off and run again.
+        return toSelector(selectorString.slice(1, selectorString.length - 1));
+    } else if (hasParens) {
+        // There shouldn't be parentheses in the middle of a selector that has no operators.
+        throw new Error(getMessage('SelectorLooksIncorrect', selectorString));
     } else {
-        throw new Error(getMessage("SelectorLooksIncorrect", `${left}${op}${right}`));
+        // A string with no operators or problems is just a simple string-selector.
+        return new SimpleSelector(selectorString);
     }
 }
 

packages/code-analyzer-core/test/rule-selection.test.ts

@@ -249,6 +249,17 @@ describe('Tests for selecting rules', () => {
         expect(ruleNamesFor(selection, 'stubEngine3')).toEqual(stubEngine3Rules);
     })
 
+    it('Operations with nested parentheses are properly resolved', async () => {
+        const selection: RuleSelection = await codeAnalyzer.selectRules(['stubEngine1:(2,Performance:(3,4))'])
+
+        expect(selection.getEngineNames()).toEqual(['stubEngine1']);
+        expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleB', 'stub1RuleC', 'stub1RuleE'])
+    });
+
+    it('Whitespace within selectors is not allowed', async () => {
+        await expect(codeAnalyzer.selectRules(['  (  stubEngine1 ) , ( stubEngine2   )   '])).rejects.toThrow('looks incorrect');
+    })
+
     it.each([
         {
             case: 'colons are used and multiple selectors are provided',
@@ -266,8 +277,35 @@ describe('Tests for selecting rules', () => {
         expect(ruleNamesFor(selection, 'stubEngine2')).toEqual(['stub2RuleC']);
     });
 
-    it('Parentheses cannot be empty', async () => {
-        await expect(codeAnalyzer.selectRules(['()'])).rejects.toThrow('empty');
+    it.each([
+        {
+            case: 'empty string',
+            selector: ''
+        },
+        {
+            case: 'empty parentheses',
+            selector: '()'
+        }
+    ])('Empty selectors are rejected. Case: $case', async ({selector}) => {
+        await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('empty');
+    });
+
+    it.each([
+        {
+            case: 'leading commas',
+            selector: ',asdfasdf'
+        },{
+            case: 'leading colons',
+            selector: ':asdfasdf'
+        },{
+            case: 'trailing commas',
+            selector: 'asdfasdf,'
+        },{
+            case: 'trailing colons',
+            selector: 'asdfasdf:'
+        }
+    ])('$case are rejected', async ({selector}) => {
+        await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('start or end with an operator');
     });
 
     it('Redundant parentheses are accepted', async () => {