From 640353e3b8a3594381bea1456fb02454cc0d8fc7 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Mon, 17 Oct 2022 14:45:17 +0200 Subject: [PATCH 1/2] Fix incorrect case in context key `implies` --- .../platform/contextkey/common/contextkey.ts | 54 ++++++++++--------- .../contextkey/test/common/contextkey.test.ts | 23 ++++++-- 2 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/vs/platform/contextkey/common/contextkey.ts b/src/vs/platform/contextkey/common/contextkey.ts index b2bcf09282123..2a022d916fa26 100644 --- a/src/vs/platform/contextkey/common/contextkey.ts +++ b/src/vs/platform/contextkey/common/contextkey.ts @@ -106,7 +106,7 @@ export abstract class ContextKeyExpr { return ContextKeyNotExpr.create(key); } public static and(...expr: Array): ContextKeyExpression | undefined { - return ContextKeyAndExpr.create(expr, null); + return ContextKeyAndExpr.create(expr, null, true); } public static or(...expr: Array): ContextKeyExpression | undefined { return ContextKeyOrExpr.create(expr, null, true); @@ -139,7 +139,7 @@ export abstract class ContextKeyExpr { private static _deserializeAndExpression(serialized: string, strict: boolean): ContextKeyExpression | undefined { const pieces = serialized.split('&&'); - return ContextKeyAndExpr.create(pieces.map(p => this._deserializeOne(p, strict)), null); + return ContextKeyAndExpr.create(pieces.map(p => this._deserializeOne(p, strict)), null, true); } private static _deserializeOne(serializedOne: string, strict: boolean): ContextKeyExpression { @@ -1163,8 +1163,8 @@ function eliminateConstantsInArray(arr: ContextKeyExpression[]): (ContextKeyExpr class ContextKeyAndExpr implements IContextKeyExpression { - public static create(_expr: ReadonlyArray, negated: ContextKeyExpression | null): ContextKeyExpression | undefined { - return ContextKeyAndExpr._normalizeArr(_expr, negated); + public static create(_expr: ReadonlyArray, negated: ContextKeyExpression | null, extraRedundantCheck: boolean): ContextKeyExpression | undefined { + return ContextKeyAndExpr._normalizeArr(_expr, negated, extraRedundantCheck); } public readonly type = ContextKeyExprType.And; @@ -1215,7 +1215,7 @@ class ContextKeyAndExpr implements IContextKeyExpression { // no change return this; } - return ContextKeyAndExpr.create(exprArr, this.negated); + return ContextKeyAndExpr.create(exprArr, this.negated, false); } public evaluate(context: IContext): boolean { @@ -1227,7 +1227,7 @@ class ContextKeyAndExpr implements IContextKeyExpression { return true; } - private static _normalizeArr(arr: ReadonlyArray, negated: ContextKeyExpression | null): ContextKeyExpression | undefined { + private static _normalizeArr(arr: ReadonlyArray, negated: ContextKeyExpression | null, extraRedundantCheck: boolean): ContextKeyExpression | undefined { const expr: ContextKeyExpression[] = []; let hasTrue = false; @@ -1298,7 +1298,7 @@ class ContextKeyAndExpr implements IContextKeyExpression { // distribute `lastElement` over `secondToLastElement` const resultElement = ContextKeyOrExpr.create( - lastElement.expr.map(el => ContextKeyAndExpr.create([el, secondToLastElement], null)), + lastElement.expr.map(el => ContextKeyAndExpr.create([el, secondToLastElement], null, extraRedundantCheck)), null, isFinished ); @@ -1313,6 +1313,22 @@ class ContextKeyAndExpr implements IContextKeyExpression { return expr[0]; } + // resolve false AND expressions + if (extraRedundantCheck) { + for (let i = 0; i < expr.length; i++) { + for (let j = i + 1; j < expr.length; j++) { + if (expr[i].negate().equals(expr[j])) { + // A && !A case + return ContextKeyFalseExpr.INSTANCE; + } + } + } + + if (expr.length === 1) { + return expr[0]; + } + } + return new ContextKeyAndExpr(expr, negated); } @@ -1467,13 +1483,13 @@ class ContextKeyOrExpr implements IContextKeyExpression { return expr[0]; } - // eliminate redundant terms + // resolve true OR expressions if (extraRedundantCheck) { for (let i = 0; i < expr.length; i++) { for (let j = i + 1; j < expr.length; j++) { - if (implies(expr[i], expr[j])) { - expr.splice(j, 1); - j--; + if (expr[i].negate().equals(expr[j])) { + // A || !A case + return ContextKeyTrueExpr.INSTANCE; } } } @@ -1518,15 +1534,14 @@ class ContextKeyOrExpr implements IContextKeyExpression { const all: ContextKeyExpression[] = []; for (const left of getTerminals(LEFT)) { for (const right of getTerminals(RIGHT)) { - all.push(ContextKeyAndExpr.create([left, right], null)!); + all.push(ContextKeyAndExpr.create([left, right], null, false)!); } } - const isFinished = (result.length === 0); - result.unshift(ContextKeyOrExpr.create(all, null, isFinished)!); + result.unshift(ContextKeyOrExpr.create(all, null, false)!); } - this.negated = result[0]; + this.negated = ContextKeyOrExpr.create(result, this, true)!; } return this.negated; } @@ -1665,15 +1680,6 @@ function cmp2(key1: string, value1: any, key2: string, value2: any): number { */ export function implies(p: ContextKeyExpression, q: ContextKeyExpression): boolean { - if (q.type === ContextKeyExprType.And && (p.type !== ContextKeyExprType.Or && p.type !== ContextKeyExprType.And)) { - // covers the case: A implies A && B - for (const qTerm of q.expr) { - if (p.equals(qTerm)) { - return true; - } - } - } - const notP = p.negate(); const expr = getTerminals(notP).concat(getTerminals(q)); expr.sort(cmp); diff --git a/src/vs/platform/contextkey/test/common/contextkey.test.ts b/src/vs/platform/contextkey/test/common/contextkey.test.ts index c3cb43292c1f1..fc2d6d6da2bf9 100644 --- a/src/vs/platform/contextkey/test/common/contextkey.test.ts +++ b/src/vs/platform/contextkey/test/common/contextkey.test.ts @@ -224,6 +224,22 @@ suite('ContextKeyExpr', () => { assert.strictEqual(expr.serialize(), 'A || B'); }); + test('Resolves true constant OR expressions', () => { + const expr = ContextKeyExpr.or( + ContextKeyExpr.has('A'), + ContextKeyExpr.not('A') + )!; + assert.strictEqual(expr.serialize(), 'true'); + }); + + test('Resolves false constant AND expressions', () => { + const expr = ContextKeyExpr.and( + ContextKeyExpr.has('A'), + ContextKeyExpr.not('A') + )!; + assert.strictEqual(expr.serialize(), 'false'); + }); + test('issue #129625: Removes duplicated terms in AND expressions', () => { const expr = ContextKeyExpr.and( ContextKeyExpr.has('A'), @@ -242,9 +258,9 @@ suite('ContextKeyExpr', () => { ) )!; assert.strictEqual(expr.serialize(), 'A && B1 || A && B2'); - assert.strictEqual(expr.negate()!.serialize(), '!A || !B1 && !B2'); + assert.strictEqual(expr.negate()!.serialize(), '!A || !A && !B1 || !A && !B2 || !B1 && !B2'); assert.strictEqual(expr.negate()!.negate()!.serialize(), 'A && B1 || A && B2'); - assert.strictEqual(expr.negate()!.negate()!.negate()!.serialize(), '!A || !B1 && !B2'); + assert.strictEqual(expr.negate()!.negate()!.negate()!.serialize(), '!A || !A && !B1 || !A && !B2 || !B1 && !B2'); }); test('issue #129625: remove redundant terms in OR expressions', () => { @@ -253,7 +269,8 @@ suite('ContextKeyExpr', () => { const q = ContextKeyExpr.deserialize(q0)!; return implies(p, q); } - assert.strictEqual(strImplies('a', 'a && b'), true); + assert.strictEqual(strImplies('a && b', 'a'), true); + assert.strictEqual(strImplies('a', 'a && b'), false); }); test('Greater, GreaterEquals, Smaller, SmallerEquals evaluate', () => { From 5cd6abca7f54ce37de336b5fa5516cced75e0c5e Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Mon, 17 Oct 2022 15:19:27 +0200 Subject: [PATCH 2/2] Fix `implies` implementation --- .../platform/contextkey/common/contextkey.ts | 66 +++++++++++++++---- .../contextkey/test/common/contextkey.test.ts | 20 ++++++ 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/contextkey/common/contextkey.ts b/src/vs/platform/contextkey/common/contextkey.ts index 2a022d916fa26..cb5f1b7a88016 100644 --- a/src/vs/platform/contextkey/common/contextkey.ts +++ b/src/vs/platform/contextkey/common/contextkey.ts @@ -1680,22 +1680,66 @@ function cmp2(key1: string, value1: any, key2: string, value2: any): number { */ export function implies(p: ContextKeyExpression, q: ContextKeyExpression): boolean { - const notP = p.negate(); - const expr = getTerminals(notP).concat(getTerminals(q)); - expr.sort(cmp); - - for (let i = 0; i < expr.length; i++) { - const a = expr[i]; - const notA = a.negate(); - for (let j = i + 1; j < expr.length; j++) { - const b = expr[j]; - if (notA.equals(b)) { + if (p.type === ContextKeyExprType.False || q.type === ContextKeyExprType.True) { + // false implies anything + // anything implies true + return true; + } + + if (p.type === ContextKeyExprType.Or) { + if (q.type === ContextKeyExprType.Or) { + // `a || b || c` can only imply something like `a || b || c || d` + return allElementsIncluded(p.expr, q.expr); + } + return false; + } + + if (q.type === ContextKeyExprType.Or) { + for (const element of q.expr) { + if (implies(p, element)) { + return true; + } + } + return false; + } + + if (p.type === ContextKeyExprType.And) { + if (q.type === ContextKeyExprType.And) { + // `a && b && c` implies `a && c` + return allElementsIncluded(q.expr, p.expr); + } + for (const element of p.expr) { + if (implies(element, q)) { return true; } } + return false; } - return false; + return p.equals(q); +} + +/** + * Returns true if all elements in `p` are also present in `q`. + * The two arrays are assumed to be sorted + */ +function allElementsIncluded(p: ContextKeyExpression[], q: ContextKeyExpression[]): boolean { + let pIndex = 0; + let qIndex = 0; + while (pIndex < p.length && qIndex < q.length) { + const cmp = p[pIndex].cmp(q[qIndex]); + + if (cmp < 0) { + // an element from `p` is missing from `q` + return false; + } else if (cmp === 0) { + pIndex++; + qIndex++; + } else { + qIndex++; + } + } + return (pIndex === p.length); } function getTerminals(node: ContextKeyExpression) { diff --git a/src/vs/platform/contextkey/test/common/contextkey.test.ts b/src/vs/platform/contextkey/test/common/contextkey.test.ts index fc2d6d6da2bf9..b5b0d0ae4e054 100644 --- a/src/vs/platform/contextkey/test/common/contextkey.test.ts +++ b/src/vs/platform/contextkey/test/common/contextkey.test.ts @@ -273,6 +273,26 @@ suite('ContextKeyExpr', () => { assert.strictEqual(strImplies('a', 'a && b'), false); }); + test('implies', () => { + function strImplies(p0: string, q0: string): boolean { + const p = ContextKeyExpr.deserialize(p0)!; + const q = ContextKeyExpr.deserialize(q0)!; + return implies(p, q); + } + assert.strictEqual(strImplies('a', 'a'), true); + assert.strictEqual(strImplies('a', 'a || b'), true); + assert.strictEqual(strImplies('a', 'a && b'), false); + assert.strictEqual(strImplies('a', 'a && b || a && c'), false); + assert.strictEqual(strImplies('a && b', 'a'), true); + assert.strictEqual(strImplies('a && b', 'b'), true); + assert.strictEqual(strImplies('a && b', 'a && b || c'), true); + assert.strictEqual(strImplies('a || b', 'a || c'), false); + assert.strictEqual(strImplies('a || b', 'a || b'), true); + assert.strictEqual(strImplies('a && b', 'a && b'), true); + assert.strictEqual(strImplies('a || b', 'a || b || c'), true); + assert.strictEqual(strImplies('c && a && b', 'c && a'), true); + }); + test('Greater, GreaterEquals, Smaller, SmallerEquals evaluate', () => { function checkEvaluate(expr: string, ctx: any, expected: any): void { const _expr = ContextKeyExpr.deserialize(expr)!;