Skip to content

FIX @W-19772057@ Refactored selection algorithm #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jfeingold35 merged 4 commits into from
Oct 10, 2025
Merged

FIX @W-19772057@ Refactored selection algorithm #367

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adca208
@W-19772057@ Refactored selection algorithm
jfeingold35 Oct 9, 2025
76548c7
@W-19772057@ Feedback from code review
jfeingold35 Oct 9, 2025
9db6735
@W-19772057@ Feedback from code review
jfeingold35 Oct 10, 2025
a07f44e
@W-19772057@ Additional feedback
jfeingold35 Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/code-analyzer-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
Expand Down Expand Up @@ -72,4 +72,4 @@
"!src/index.ts"
]
}
}
}
5 changes: 4 additions & 1 deletion packages/code-analyzer-core/src/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.`,
Expand Down
105 changes: 47 additions & 58 deletions packages/code-analyzer-core/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,81 +5,70 @@ 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 === '') {
if (selectorString.includes(' ')) {
// ERROR CASE: The selector contains whitespace.
throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
Copy link
Contributor

@stephen-carter-at-sf stephen-carter-at-sf Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using includes or regex testing, you are increasing the runtime since you are doing additional passes unnecessarily. We want this part of the code to be as performant as possible. So you should move the space check as in if statement inside of the loop since you are already checking character by character.

} else 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, ':');
}
} 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));
Copy link
Contributor

@stephen-carter-at-sf stephen-carter-at-sf Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, you can test the first and last character of each selectorString (since the algorithm is recursive) by just checking the first and last characters without a regular expression.

So

} else if (selectorString.startsWith(',') ||  selectorString.startsWith(':') || selectorString.endsWith(',') ||  selectorString.endsWith(':')) {

would be better here.

Or better yet... you could just add it to the if statement in your loop below. For example

    if (i===0 && (char === ',' || char === ':'))  {

.

If this wasn't a recursive algorithm where the operations get multiplied then it wouldn't be as big of a deal - but it is, so we need to try to keep O(1) operations inside of a single O(n) loop if possible.

}
}

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 (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 === 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;
}
Comment on lines 35 to 46
Copy link
Contributor

@stephen-carter-at-sf stephen-carter-at-sf Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of keeping track of the commaIdx, since commas trump colons, can't we just do a quick return here to avoid processing more. I only think we need to keep track of the colonIdx the whole way through.

That is, can't we just do:

} else if (parenBalance > 0) { // Ignore chars inside of parens
    continue;
} else if (char === ',') {
        const left: string = trimmedSelector.slice(0, i);
        const right: string = trimmedSelector.slice(i + 1);
        return new OrSelector(toSelector(left), toSelector(right));
} else if (char === ':' && colonIdx === null) {
    colonIdx = i;
}

Then you don't need lines 47-50 below.

Also maybe rename colonIdx to firstColonIdx

Copy link
Contributor Author

@jfeingold35 jfeingold35 Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but doing the full iteration allows us to catch situations where the number of open-parens exceeds the number of close-parens, since the balance will be positive at the end.

Copy link
Contributor

@stephen-carter-at-sf stephen-carter-at-sf Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will catch it anyway already because all the parens need to be balanced on both sides of the top level comma anyway and so it'll get caught in the next iteration of the loop.... so I do believe that you can indeed short circuit when you find the first top level comma.

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

Expand Down
42 changes: 40 additions & 2 deletions packages/code-analyzer-core/test/rule-selection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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 () => {
Expand Down