Skip to content
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

Jump to bottom

Improve implies implementation #163841

Merged
merged 2 commits into from Oct 17, 2022
Merged

Improve implies implementation #163841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
640353e
Fix incorrect case in context key `implies`
alexdima Oct 17, 2022
5cd6abc
Fix `implies` implementation
alexdima Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
110 changes: 80 additions & 30 deletions src/vs/platform/contextkey/common/contextkey.ts
View file
Open in desktop
Expand Up @@ -106,7 +106,7 @@ export abstract class ContextKeyExpr {
return ContextKeyNotExpr.create(key);
}
public static and(...expr: Array<ContextKeyExpression | undefined | null>): ContextKeyExpression | undefined {
return ContextKeyAndExpr.create(expr, null);
return ContextKeyAndExpr.create(expr, null, true);
}
public static or(...expr: Array<ContextKeyExpression | undefined | null>): ContextKeyExpression | undefined {
return ContextKeyOrExpr.create(expr, null, true);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1163,8 +1163,8 @@ function eliminateConstantsInArray(arr: ContextKeyExpression[]): (ContextKeyExpr

class ContextKeyAndExpr implements IContextKeyExpression {

public static create(_expr: ReadonlyArray<ContextKeyExpression | null | undefined>, negated: ContextKeyExpression | null): ContextKeyExpression | undefined {
return ContextKeyAndExpr._normalizeArr(_expr, negated);
public static create(_expr: ReadonlyArray<ContextKeyExpression | null | undefined>, negated: ContextKeyExpression | null, extraRedundantCheck: boolean): ContextKeyExpression | undefined {
return ContextKeyAndExpr._normalizeArr(_expr, negated, extraRedundantCheck);
}

public readonly type = ContextKeyExprType.And;
Expand Down Expand Up @@ -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 {
Expand All @@ -1227,7 +1227,7 @@ class ContextKeyAndExpr implements IContextKeyExpression {
return true;
}

private static _normalizeArr(arr: ReadonlyArray<ContextKeyExpression | null | undefined>, negated: ContextKeyExpression | null): ContextKeyExpression | undefined {
private static _normalizeArr(arr: ReadonlyArray<ContextKeyExpression | null | undefined>, negated: ContextKeyExpression | null, extraRedundantCheck: boolean): ContextKeyExpression | undefined {
const expr: ContextKeyExpression[] = [];
let hasTrue = false;

Expand Down Expand Up @@ -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
);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
}
}
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1665,31 +1680,66 @@ 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)) {
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;
}

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.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) {
Expand Down
43 changes: 40 additions & 3 deletions src/vs/platform/contextkey/test/common/contextkey.test.ts
View file
Open in desktop
Expand Up @@ -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'),
Expand All @@ -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', () => {
Expand All @@ -253,7 +269,28 @@ 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('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', () => {
Expand Down