Skip to content

Commit 79d72ef

Browse files
jfeingold35jfeingold35
authored
FIX @W-19772057@ Refactored selection algorithm (#367)
1 parent 94b7aeb commit 79d72ef

File tree

4 files changed

+94
-62
lines changed

4 files changed

+94
-62
lines changed

‎packages/code-analyzer-core/package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "@salesforce/code-analyzer-core",
33
"description": "Core Package for the Salesforce Code Analyzer",
4-
"version": "0.38.0",
4+
"version": "0.38.1-SNAPSHOT",
55
"author": "The Salesforce Code Analyzer Team",
66
"license": "BSD-3-Clause",
77
"homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",
@@ -72,4 +72,4 @@
7272
"!src/index.ts"
7373
]
7474
}
75-
}
75+
}

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,10 @@ const MESSAGE_CATALOG : MessageCatalog = {
167167
`Rule selectors can't be empty strings.`,
168168

169169
SelectorLooksIncorrect:
170-
`Rule selector '%s' looks incorrect. Make sure that parentheses are balanced and that all subselectors are joined by a colon (:) or comma (,).`,
170+
`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 (,).`,
171+
172+
SelectorStartsOrEndsWithOperator:
173+
`Rule selectors should not start or end with an operator. Selector: '%s'`,
171174

172175
EngineRunResultsMissing:
173176
`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‎

Lines changed: 48 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -5,81 +5,72 @@ export interface Selector {
55
}
66

77
export function toSelector(selectorString: string): Selector {
8-
// We parse the selector back-to-front, so that the front-most selectors end up at the bottom of the tree we create
9-
// and therefore get resolved first.
108
if (selectorString === '') {
119
// ERROR CASE: The selector is empty. Possible if you do something like "()" or "a:()".
1210
throw new Error(getMessage("SelectorCannotBeEmpty"));
13-
} else if (selectorString.endsWith(')')) {
14-
// If the selector ends in close-paren, then we need to find the open-paren that matches it.
15-
const correspondingOpenParen: number = identifyCorrespondingOpenParen(selectorString);
16-
if (correspondingOpenParen === 0) {
17-
// RECURSIVE CASE: The entire selector is wrapped in parens. Pop them off and call recursively.
18-
return toSelector(selectorString.slice(1, -1))
19-
} else {
20-
// RECURSIVE CASE: The open-paren is somewhere in the middle of the selector and accompanied by an operator.
21-
const left: string = selectorString.slice(0, correspondingOpenParen - 1);
22-
const right: string = selectorString.slice(correspondingOpenParen);
23-
const op: string = selectorString[correspondingOpenParen - 1];
24-
return toComplexSelector(left, right, op);
25-
}
26-
} else {
27-
// If there's a close-paren in the string, only look for operators after it.
28-
const lastCloseParen: number = Math.max(selectorString.lastIndexOf(')'), 0);
29-
const lastComma: number = selectorString.slice(lastCloseParen).lastIndexOf(',');
30-
const lastColon: number = selectorString.slice(lastCloseParen).lastIndexOf(':');
31-
32-
// BASE CASE: The selector contains no commas or colons.
33-
if (lastComma === -1 && lastColon === -1) {
34-
// Parens only make sense in conjunction with operators, so if we find any, the selector is malformed.
35-
if (selectorString.includes(')') || selectorString.includes('(')) {
36-
throw new Error(getMessage('SelectorLooksIncorrect', selectorString));
37-
}
38-
return new SimpleSelector(selectorString);
39-
} else if (lastComma !== -1) {
40-
// Commas resolve before colons, so that "x,a:b" and "a:b,x" both resolve equivalently the combination of
41-
// "x" and "a:b".
42-
const left: string = selectorString.slice(0, lastComma + lastCloseParen);
43-
const right: string = selectorString.slice(lastComma + lastCloseParen + 1);
44-
return toComplexSelector(left, right, ',');
45-
} else {
46-
const left: string = selectorString.slice(0, lastColon + lastCloseParen);
47-
const right: string = selectorString.slice(lastColon + lastCloseParen + 1);
48-
return toComplexSelector(left, right, ':');
49-
}
5011
}
51-
}
5212

53-
function identifyCorrespondingOpenParen(selectorString: string): number {
54-
const reversedLetters: string[] = selectorString.split('').reverse();
13+
let commaIdx: number|null = null;
14+
let colonIdx: number|null = null;
5515
let parenBalance: number = 0;
56-
let idx = 0;
57-
for (const letter of reversedLetters) {
58-
if (letter === ')') {
16+
let hasParens: boolean = false;
17+
for (let i = 0; i < selectorString.length; i++) {
18+
const char: string = selectorString[i];
19+
if ((i === 0 || i === selectorString.length - 1) && (char === ',' || char === ':')) {
20+
// ERROR CASE: The selector cannot start or end with a binary operator, because that's nonsense.
21+
throw new Error(getMessage("SelectorStartsOrEndsWithOperator", selectorString));
22+
} else if (char === ' ') {
23+
// ERROR CASE: The selector contains whitespace.
24+
throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
25+
} else if (char === '(') {
5926
parenBalance += 1;
60-
} else if (letter === '(') {
27+
hasParens = true;
28+
} else if (char === ')') {
6129
parenBalance -= 1;
30+
hasParens = true;
31+
// If our parenthesis balance is negative, it means there are more close-parens than open-parens, which is a problem.
32+
if (parenBalance < 0) {
33+
throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
34+
}
35+
} else if (char === ',') {
36+
// If we're not inside of parentheses, and we haven't already found a comma, note the location of this one.
37+
if (parenBalance === 0) {
38+
commaIdx = i;
39+
// Commas trump everything else, so we can just break.
40+
break;
41+
}
42+
} else if (char === ':') {
43+
// If we're not inside of parentheses, and we haven't already found a colon, note the location of this one.
44+
if (parenBalance === 0 && colonIdx === null) {
45+
colonIdx = i;
46+
}
6247
}
63-
if (parenBalance === 0) {
64-
break;
65-
}
66-
idx += 1;
6748
}
6849

50+
// If our final parenthesis balance is negative, it means there are more open-parens than close-parens, which is a problem.
6951
if (parenBalance > 0) {
7052
throw new Error(getMessage("SelectorLooksIncorrect", selectorString));
7153
}
7254

73-
return selectorString.length - idx - 1;
74-
}
75-
76-
function toComplexSelector(left: string, right: string, op: string): Selector {
77-
if (op === ',') {
55+
// Commas trump colons, so if we have a comma, split along that.
56+
if (commaIdx != null) {
57+
const left: string = selectorString.slice(0, commaIdx);
58+
const right: string = selectorString.slice(commaIdx + 1);
7859
return new OrSelector(toSelector(left), toSelector(right));
79-
} else if (op === ':') {
60+
} else if (colonIdx != null) {
61+
// If there are colons but no commas, split along the first colon.
62+
const left: string = selectorString.slice(0, colonIdx);
63+
const right: string = selectorString.slice(colonIdx + 1);
8064
return new AndSelector(toSelector(left), toSelector(right));
65+
} else if (selectorString[0] === '(' && selectorString[selectorString.length - 1] === ')') {
66+
// If the first and last character are parentheses, then pop those off and run again.
67+
return toSelector(selectorString.slice(1, selectorString.length - 1));
68+
} else if (hasParens) {
69+
// There shouldn't be parentheses in the middle of a selector that has no operators.
70+
throw new Error(getMessage('SelectorLooksIncorrect', selectorString));
8171
} else {
82-
throw new Error(getMessage("SelectorLooksIncorrect", `${left}${op}${right}`));
72+
// A string with no operators or problems is just a simple string-selector.
73+
return new SimpleSelector(selectorString);
8374
}
8475
}
8576

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

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,17 @@ describe('Tests for selecting rules', () => {
249249
expect(ruleNamesFor(selection, 'stubEngine3')).toEqual(stubEngine3Rules);
250250
})
251251

252+
it('Operations with nested parentheses are properly resolved', async () => {
253+
const selection: RuleSelection = await codeAnalyzer.selectRules(['stubEngine1:(2,Performance:(3,4))'])
254+
255+
expect(selection.getEngineNames()).toEqual(['stubEngine1']);
256+
expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleB', 'stub1RuleC', 'stub1RuleE'])
257+
});
258+
259+
it('Whitespace within selectors is not allowed', async () => {
260+
await expect(codeAnalyzer.selectRules([' ( stubEngine1 ) , ( stubEngine2 ) '])).rejects.toThrow('looks incorrect');
261+
})
262+
252263
it.each([
253264
{
254265
case: 'colons are used and multiple selectors are provided',
@@ -266,8 +277,35 @@ describe('Tests for selecting rules', () => {
266277
expect(ruleNamesFor(selection, 'stubEngine2')).toEqual(['stub2RuleC']);
267278
});
268279

269-
it('Parentheses cannot be empty', async () => {
270-
await expect(codeAnalyzer.selectRules(['()'])).rejects.toThrow('empty');
280+
it.each([
281+
{
282+
case: 'empty string',
283+
selector: ''
284+
},
285+
{
286+
case: 'empty parentheses',
287+
selector: '()'
288+
}
289+
])('Empty selectors are rejected. Case: $case', async ({selector}) => {
290+
await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('empty');
291+
});
292+
293+
it.each([
294+
{
295+
case: 'leading commas',
296+
selector: ',asdfasdf'
297+
},{
298+
case: 'leading colons',
299+
selector: ':asdfasdf'
300+
},{
301+
case: 'trailing commas',
302+
selector: 'asdfasdf,'
303+
},{
304+
case: 'trailing colons',
305+
selector: 'asdfasdf:'
306+
}
307+
])('$case are rejected', async ({selector}) => {
308+
await expect(codeAnalyzer.selectRules([selector])).rejects.toThrow('start or end with an operator');
271309
});
272310

273311
it('Redundant parentheses are accepted', async () => {

0 commit comments

Comments
 (0)