From adca2085b6f8df20a1290799477023cf7518de7b Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Thu, 9 Oct 2025 12:35:27 -0500 Subject: [PATCH 1/4] @W-19772057@ Refactored selection algorithm --- packages/code-analyzer-core/package.json | 4 +- packages/code-analyzer-core/src/selectors.ts | 98 ++++++++----------- .../test/rule-selection.test.ts | 43 +++++++- 3 files changed, 82 insertions(+), 63 deletions(-) diff --git a/packages/code-analyzer-core/package.json b/packages/code-analyzer-core/package.json index 2ae363b2..ca3a67c8 100644 --- a/packages/code-analyzer-core/package.json +++ b/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" ] } -} \ No newline at end of file +} diff --git a/packages/code-analyzer-core/src/selectors.ts b/packages/code-analyzer-core/src/selectors.ts index c7b6128e..14e21d08 100644 --- a/packages/code-analyzer-core/src/selectors.ts +++ b/packages/code-analyzer-core/src/selectors.ts @@ -5,81 +5,63 @@ 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 === '') { + const trimmedSelector: string = selectorString.trim(); + + if (trimmedSelector === '') { // 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 === ')') { + for (let i = 0; i < trimmedSelector.length; i++) { + const char: string = trimmedSelector[i]; + if (char === '(') { parenBalance += 1; - } else if (letter === '(') { + } else if (char === ')') { parenBalance -= 1; + // 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 === null) { + commaIdx = i; + } + } 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 = trimmedSelector.slice(0, commaIdx); + const right: string = trimmedSelector.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 = trimmedSelector.slice(0, colonIdx); + const right: string = trimmedSelector.slice(colonIdx + 1); return new AndSelector(toSelector(left), toSelector(right)); + } else if (trimmedSelector[0] === '(' && trimmedSelector[trimmedSelector.length - 1] === ')') { + // If the first and last character are parentheses, then pop those off and run again. + return toSelector(trimmedSelector.slice(1, trimmedSelector.length - 1)); + } else if (trimmedSelector.includes('(') || trimmedSelector.includes(')')) { + // 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(trimmedSelector); } } diff --git a/packages/code-analyzer-core/test/rule-selection.test.ts b/packages/code-analyzer-core/test/rule-selection.test.ts index 2a93240b..834b5546 100644 --- a/packages/code-analyzer-core/test/rule-selection.test.ts +++ b/packages/code-analyzer-core/test/rule-selection.test.ts @@ -249,6 +249,21 @@ 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 does not interfere with selection', async () => { + const selection: RuleSelection = await codeAnalyzer.selectRules([' ( stubEngine1 ) , ( stubEngine2 ) ']); + + expect(selection.getEngineNames()).toEqual(['stubEngine1', 'stubEngine2']); + expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleA', 'stub1RuleB', 'stub1RuleC', 'stub1RuleD', 'stub1RuleE']); + expect(ruleNamesFor(selection, 'stubEngine2')).toEqual(['stub2RuleA', 'stub2RuleB', 'stub2RuleC']); + }) + it.each([ { case: 'colons are used and multiple selectors are provided', @@ -266,9 +281,31 @@ 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: '()' + }, + { + case: 'leading comma', + selector: ',asdfasdf' + },{ + case: 'leading colon', + selector: ':asdfasdf' + },{ + case: 'trailing comma', + selector: 'asdfasdf,' + },{ + case: 'trailing colon', + selector: 'asdfasdf:' + } + ])('Empty selectors are rejected. Case: $case', async ({selector}) => { + await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('empty'); + }) it('Redundant parentheses are accepted', async () => { const selection: RuleSelection = await codeAnalyzer.selectRules(['((((((((stub1RuleC))))))))']); From 76548c7fea8fc7ae6ba7d2be47fec448cd2a1dd8 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Thu, 9 Oct 2025 15:36:29 -0500 Subject: [PATCH 2/4] @W-19772057@ Feedback from code review --- packages/code-analyzer-core/src/messages.ts | 5 ++- packages/code-analyzer-core/src/selectors.ts | 33 +++++++++++-------- .../test/rule-selection.test.ts | 29 ++++++++-------- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/packages/code-analyzer-core/src/messages.ts b/packages/code-analyzer-core/src/messages.ts index 8984345c..8f2ad2e5 100644 --- a/packages/code-analyzer-core/src/messages.ts +++ b/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.`, diff --git a/packages/code-analyzer-core/src/selectors.ts b/packages/code-analyzer-core/src/selectors.ts index 14e21d08..ff90ca0b 100644 --- a/packages/code-analyzer-core/src/selectors.ts +++ b/packages/code-analyzer-core/src/selectors.ts @@ -5,22 +5,29 @@ export interface Selector { } export function toSelector(selectorString: string): Selector { - const trimmedSelector: string = selectorString.trim(); - - if (trimmedSelector === '') { + if (selectorString.includes(' ')) { + // ERROR CASE: The selector contains whitespace. + throw new Error(getMessage("SelectorLooksIncorrect", selectorString)); + } else if (selectorString === '') { // ERROR CASE: The selector is empty. Possible if you do something like "()" or "a:()". throw new Error(getMessage("SelectorCannotBeEmpty")); + } else if (new RegExp('^[,:]').test(selectorString) || new RegExp('[,:]$').test(selectorString)) { + // ERROR CASE: The selector cannot start with a binary operator, because that's nonsense. + throw new Error(getMessage("SelectorStartsOrEndsWithOperator", selectorString)); } let commaIdx: number|null = null; let colonIdx: number|null = null; let parenBalance: number = 0; - for (let i = 0; i < trimmedSelector.length; i++) { - const char: string = trimmedSelector[i]; + let hasParens: boolean = false; + for (let i = 0; i < selectorString.length; i++) { + const char: string = selectorString[i]; if (char === '(') { parenBalance += 1; + 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)); @@ -45,23 +52,23 @@ export function toSelector(selectorString: string): Selector { // Commas trump colons, so if we have a comma, split along that. if (commaIdx != null) { - const left: string = trimmedSelector.slice(0, commaIdx); - const right: string = trimmedSelector.slice(commaIdx + 1); + const left: string = selectorString.slice(0, commaIdx); + const right: string = selectorString.slice(commaIdx + 1); return new OrSelector(toSelector(left), toSelector(right)); } else if (colonIdx != null) { // If there are colons but no commas, split along the first colon. - const left: string = trimmedSelector.slice(0, colonIdx); - const right: string = trimmedSelector.slice(colonIdx + 1); + const left: string = selectorString.slice(0, colonIdx); + const right: string = selectorString.slice(colonIdx + 1); return new AndSelector(toSelector(left), toSelector(right)); - } else if (trimmedSelector[0] === '(' && trimmedSelector[trimmedSelector.length - 1] === ')') { + } 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(trimmedSelector.slice(1, trimmedSelector.length - 1)); - } else if (trimmedSelector.includes('(') || trimmedSelector.includes(')')) { + 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 { // A string with no operators or problems is just a simple string-selector. - return new SimpleSelector(trimmedSelector); + return new SimpleSelector(selectorString); } } diff --git a/packages/code-analyzer-core/test/rule-selection.test.ts b/packages/code-analyzer-core/test/rule-selection.test.ts index 834b5546..d5074014 100644 --- a/packages/code-analyzer-core/test/rule-selection.test.ts +++ b/packages/code-analyzer-core/test/rule-selection.test.ts @@ -256,12 +256,8 @@ describe('Tests for selecting rules', () => { expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleB', 'stub1RuleC', 'stub1RuleE']) }); - it('Whitespace does not interfere with selection', async () => { - const selection: RuleSelection = await codeAnalyzer.selectRules([' ( stubEngine1 ) , ( stubEngine2 ) ']); - - expect(selection.getEngineNames()).toEqual(['stubEngine1', 'stubEngine2']); - expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleA', 'stub1RuleB', 'stub1RuleC', 'stub1RuleD', 'stub1RuleE']); - expect(ruleNamesFor(selection, 'stubEngine2')).toEqual(['stub2RuleA', 'stub2RuleB', 'stub2RuleC']); + it('Whitespace within selectors is not allowed', async () => { + await expect(codeAnalyzer.selectRules([' ( stubEngine1 ) , ( stubEngine2 ) '])).rejects.toThrow('looks incorrect'); }) it.each([ @@ -289,23 +285,28 @@ describe('Tests for selecting rules', () => { { case: 'empty parentheses', selector: '()' - }, + } + ])('Empty selectors are rejected. Case: $case', async ({selector}) => { + await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('empty'); + }); + + it.each([ { - case: 'leading comma', + case: 'leading commas', selector: ',asdfasdf' },{ - case: 'leading colon', + case: 'leading colons', selector: ':asdfasdf' },{ - case: 'trailing comma', + case: 'trailing commas', selector: 'asdfasdf,' },{ - case: 'trailing colon', + case: 'trailing colons', selector: 'asdfasdf:' } - ])('Empty selectors are rejected. Case: $case', async ({selector}) => { - await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('empty'); - }) + ])('$case are rejected', async ({selector}) => { + await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('start or end with an operator'); + }); it('Redundant parentheses are accepted', async () => { const selection: RuleSelection = await codeAnalyzer.selectRules(['((((((((stub1RuleC))))))))']); From 9db6735725d73e5585c27c60220e578e6fdca213 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Fri, 10 Oct 2025 09:32:53 -0500 Subject: [PATCH 3/4] @W-19772057@ Feedback from code review --- packages/code-analyzer-core/src/selectors.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/code-analyzer-core/src/selectors.ts b/packages/code-analyzer-core/src/selectors.ts index ff90ca0b..e7a445a6 100644 --- a/packages/code-analyzer-core/src/selectors.ts +++ b/packages/code-analyzer-core/src/selectors.ts @@ -5,15 +5,9 @@ export interface Selector { } export function toSelector(selectorString: string): Selector { - if (selectorString.includes(' ')) { - // ERROR CASE: The selector contains whitespace. - throw new Error(getMessage("SelectorLooksIncorrect", selectorString)); - } else if (selectorString === '') { + if (selectorString === '') { // ERROR CASE: The selector is empty. Possible if you do something like "()" or "a:()". throw new Error(getMessage("SelectorCannotBeEmpty")); - } else if (new RegExp('^[,:]').test(selectorString) || new RegExp('[,:]$').test(selectorString)) { - // ERROR CASE: The selector cannot start with a binary operator, because that's nonsense. - throw new Error(getMessage("SelectorStartsOrEndsWithOperator", selectorString)); } let commaIdx: number|null = null; @@ -22,7 +16,13 @@ export function toSelector(selectorString: string): Selector { let hasParens: boolean = false; for (let i = 0; i < selectorString.length; i++) { const char: string = selectorString[i]; - if (char === '(') { + 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; hasParens = true; } else if (char === ')') { From a07f44eaad76637dcc0ec9901b038429dc971a3f Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Fri, 10 Oct 2025 09:44:53 -0500 Subject: [PATCH 4/4] @W-19772057@ Additional feedback --- packages/code-analyzer-core/src/selectors.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/code-analyzer-core/src/selectors.ts b/packages/code-analyzer-core/src/selectors.ts index e7a445a6..fc13a6fc 100644 --- a/packages/code-analyzer-core/src/selectors.ts +++ b/packages/code-analyzer-core/src/selectors.ts @@ -34,8 +34,10 @@ export function toSelector(selectorString: string): Selector { } } 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 === null) { + 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.