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.

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

feat(stdlib): Add lookupCI for assocarray and Node #639

Merged
merged 14 commits into from
Apr 30, 2021
5 changes: 3 additions & 2 deletions src/brsTypes/components/BrsComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ export interface BrsIterable {
/**
* Retrieves an element from this component at the provided `index`.
* @param index the index in this component from which to retrieve the desired element.
* @param isCaseSensitive determinate whether operation of getting should be case sensitive or not.
* @returns the element at `index` if one exists, otherwise throws an Error.
*/
get(index: BrsType): BrsType;
get(index: BrsType, isCaseSensitive?: boolean): BrsType;

set(index: BrsType, value: BrsType): BrsInvalid;
set(index: BrsType, value: BrsType, isCaseSensitive?: boolean): BrsInvalid;
}
92 changes: 76 additions & 16 deletions src/brsTypes/components/RoAssociativeArray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ export interface AAMember {
export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIterable {
readonly kind = ValueKind.Object;
elements = new Map<string, BrsType>();
/** Maps lowercased element name to original name used in this.elements.
* Main benefit of it is fast, case-insensitive access.
*/
keyMap = new Map<string, string>();
private modeCaseSensitive: boolean = false;

constructor(elements: AAMember[]) {
super("roAssociativeArray");
elements.forEach((member) =>
this.elements.set(member.name.value.toLowerCase(), member.value)
);
elements.forEach((member) => this.set(member.name, member.value, true));

this.registerMethods({
ifAssociativeArray: [
Expand All @@ -36,6 +38,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
this.keys,
this.items,
this.lookup,
this.lookupCI,
this.setmodecasesensitive,
],
ifEnum: [this.isEmpty],
Expand Down Expand Up @@ -77,7 +80,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
.map((value: BrsType) => value);
}

get(index: BrsType) {
get(index: BrsType, isCaseSensitive = false) {
if (index.kind !== ValueKind.String) {
throw new Error("Associative array indexes must be strings");
}
Expand All @@ -93,19 +96,44 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
// method with the desired name separately? That last bit would work but it's pretty gross.
// That'd allow roArrays to have methods with the methods not accessible via `arr["count"]`.
// Same with RoAssociativeArrays I guess.
let indexValue = this.modeCaseSensitive ? index.value : index.value.toLowerCase();
return this.elements.get(indexValue) || this.getMethod(index.value) || BrsInvalid.Instance;
return (
this.findElement(index.value, isCaseSensitive) ||
this.getMethod(index.value) ||
BrsInvalid.Instance
);
}

set(index: BrsType, value: BrsType) {
set(index: BrsType, value: BrsType, isCaseSensitive = false) {
if (index.kind !== ValueKind.String) {
throw new Error("Associative array indexes must be strings");
}
let indexValue = this.modeCaseSensitive ? index.value : index.value.toLowerCase();
// override old key with new one
let oldKey = this.findElementKey(index.value);
if (!this.modeCaseSensitive && oldKey) {
this.elements.delete(oldKey);
}

let indexValue = isCaseSensitive ? index.value : index.value.toLowerCase();
this.elements.set(indexValue, value);
this.keyMap.set(index.value.toLowerCase(), indexValue);

return BrsInvalid.Instance;
}

/** if AA is in insensitive mode, it means that we should do insensitive search of real key */
alimnios72 marked this conversation as resolved.
Show resolved Hide resolved
private findElementKey(elementKey: string, isCaseSensitiveFind = false) {
if (this.modeCaseSensitive && isCaseSensitiveFind) {
return elementKey;
} else {
return this.keyMap.get(elementKey.toLowerCase());
}
}

private findElement(elementKey: string, isCaseSensitiveFind = false) {
let realKey = this.findElementKey(elementKey, isCaseSensitiveFind);
return realKey !== undefined ? this.elements.get(realKey) : undefined;
}

/** Removes all elements from the associative array */
private clear = new Callable("clear", {
signature: {
Expand All @@ -114,6 +142,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
},
impl: (interpreter: Interpreter) => {
this.elements.clear();
this.keyMap.clear();
return BrsInvalid.Instance;
},
});
Expand All @@ -125,7 +154,23 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
returns: ValueKind.Boolean,
},
impl: (interpreter: Interpreter, str: BrsString) => {
let deleted = this.elements.delete(str.value.toLowerCase());
let key = this.findElementKey(str.value, this.modeCaseSensitive);
let deleted = key ? this.elements.delete(key) : false;

let lKey = str.value.toLowerCase();
if (this.keyMap.get(lKey) === key) {
this.keyMap.delete(lKey);
// if we have {"DD": 0, "dD": 0}, keyMap["dd"] is pointed to "DD",
// and we delete it, then we should find another value for case insensitive accessing ("dD")
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for calling this out! This is an interesting case, and feels like keyMap should be Map<string, Set<string>>: a map of lowercase keys to all encountered cases of that key. In your example with { "DD": 0, "dD": 1}, keyMap.get("dd") would be the set ("DD", "dD"). When someAA.delete("DD") is called from BrightScript, this function removes the first element from that set (something like let newSet = new Set([ ...keyMap.get(lKey) ].slice(1));) in case-sensitive mode, and removes the entire entry (this.keyMap.delete(lKey)) in case-insensitive mode.

Lookups become pretty similar, using the element at index zero of this Set<string>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (this.modeCaseSensitive) {
for (let key of this.elements.keys()) {
if (key.toLowerCase() === lKey) {
this.keyMap.set(lKey, key);
break;
}
}
}
}
return BrsBoolean.from(deleted);
},
});
Expand All @@ -142,7 +187,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
returns: ValueKind.Void,
},
impl: (interpreter: Interpreter, key: BrsString, value: BrsType) => {
this.set(key, value);
this.set(key, value, /* isCaseSensitive */ true);
return BrsInvalid.Instance;
},
});
Expand All @@ -166,7 +211,8 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
},
impl: (interpreter: Interpreter, str: BrsString) => {
let strValue = this.modeCaseSensitive ? str.value : str.value.toLowerCase();
return this.elements.has(strValue) ? BrsBoolean.True : BrsBoolean.False;
let key = this.findElementKey(str.value, this.modeCaseSensitive);
return key && this.elements.has(key) ? BrsBoolean.True : BrsBoolean.False;
},
});

Expand All @@ -182,7 +228,9 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
return BrsInvalid.Instance;
}

this.elements = new Map<string, BrsType>([...this.elements, ...obj.elements]);
obj.elements.forEach((value, key) => {
this.set(new BrsString(key), value, true);
});
lkipke marked this conversation as resolved.
Show resolved Hide resolved

return BrsInvalid.Instance;
},
Expand Down Expand Up @@ -223,15 +271,27 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
},
});

/** Given a key, returns the value associated with that key. This method is case insensitive. */
/** Given a key, returns the value associated with that key.
* This method is case insensitive either-or case sensitive, depends on whether `setModeCasesensitive` was called or not.
*/
private lookup = new Callable("lookup", {
signature: {
args: [new StdlibArgument("key", ValueKind.String)],
returns: ValueKind.Dynamic,
},
impl: (interpreter: Interpreter, key: BrsString) => {
let lKey = key.value;
return this.get(new BrsString(lKey));
return this.get(key, true);
},
});

/** Given a key, returns the value associated with that key. This method always is case insensitive. */
private lookupCI = new Callable("lookupCI", {
alimnios72 marked this conversation as resolved.
Show resolved Hide resolved
signature: {
args: [new StdlibArgument("key", ValueKind.String)],
returns: ValueKind.Dynamic,
},
impl: (interpreter: Interpreter, key: BrsString) => {
return this.get(key);
},
});

Expand All @@ -242,7 +302,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
returns: ValueKind.Void,
},
impl: (interpreter: Interpreter) => {
this.modeCaseSensitive = !this.modeCaseSensitive;
this.modeCaseSensitive = true;
return BrsInvalid.Instance;
},
});
Expand Down
6 changes: 5 additions & 1 deletion src/brsTypes/components/RoSGNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ export class RoSGNode extends BrsComponent implements BrsValue, BrsIterable {
readonly defaultFields: FieldModel[] = [
{ name: "change", type: "roAssociativeArray" },
{ name: "focusable", type: "boolean" },
{ name: "focusedChild", type: "node", alwaysNotify: true },
{ name: "focusedchild", type: "node", alwaysNotify: true },
{ name: "id", type: "string" },
];
m: RoAssociativeArray = new RoAssociativeArray([]);
Expand All @@ -327,6 +327,7 @@ export class RoSGNode extends BrsComponent implements BrsValue, BrsIterable {
this.keys,
this.items,
this.lookup,
this.lookupCI,
],
ifSGNodeField: [
this.addfield,
Expand Down Expand Up @@ -801,6 +802,9 @@ export class RoSGNode extends BrsComponent implements BrsValue, BrsIterable {
},
});

/** Given a key, returns the value associated with that key. This method is case insensitive. */
private lookupCI = new Callable("lookupCI", this.lookup.signatures[0]);
alimnios72 marked this conversation as resolved.
Show resolved Hide resolved

/** Adds a new field to the node, if the field already exists it doesn't change the current value. */
private addfield = new Callable("addfield", {
signature: {
Expand Down
4 changes: 2 additions & 2 deletions src/interpreter/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
}

try {
return source.get(index);
return source.get(index, true);
} catch (err) {
return this.addError(new BrsError(err.message, expression.closingSquare.location));
}
Expand Down Expand Up @@ -1537,7 +1537,7 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
let value = this.evaluate(statement.value);

try {
source.set(index, value);
source.set(index, value, true);
} catch (err) {
return this.addError(new BrsError(err.message, statement.closingSquare.location));
}
Expand Down
39 changes: 39 additions & 0 deletions test/brsTypes/components/RoAssociativeArray.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,5 +374,44 @@ describe("RoAssociativeArray", () => {
expect(result2).toBe(v2);
});
});

describe("lookupCI", () => {
alimnios72 marked this conversation as resolved.
Show resolved Hide resolved
it("changes lookups to case sensitive mode", () => {
let v1 = new BrsString("value1");
let v2 = new BrsString("value2");
let v3 = new BrsString("value3");

let aa = new RoAssociativeArray([{ name: new BrsString("key1"), value: v1 }]);

let setModeCaseSensitive = aa.getMethod("setmodecasesensitive");
expect(setModeCaseSensitive).toBeTruthy();
setModeCaseSensitive.call(interpreter);

let addreplace = aa.getMethod("addreplace");
expect(addreplace).toBeTruthy();
addreplace.call(interpreter, new BrsString("KEY1"), v2);

let lookup = aa.getMethod("lookup");
expect(lookup).toBeTruthy();

// check lookupCI method
let lookupCI = aa.getMethod("lookupCI");
alimnios72 marked this conversation as resolved.
Show resolved Hide resolved
// RBI uses last value for key when uses lookupCI, if do not set new value it will use old v2 value
addreplace.call(interpreter, new BrsString("Key1"), v3);
expect(lookupCI).toBeTruthy();

let result = lookup.call(interpreter, new BrsString("key1"));
expect(result).toBe(v1);

result = lookup.call(interpreter, new BrsString("KEY1"));
expect(result).toBe(v2);

result = lookupCI.call(interpreter, new BrsString("kEy1"));
expect(result).toBe(v3);

result = lookupCI.call(interpreter, new BrsString("KeY1"));
expect(result).toBe(v3);
});
});
});
});
2 changes: 1 addition & 1 deletion test/brsTypes/components/RoSGNode.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ describe("RoSGNode", () => {
let expected = new RoAssociativeArray([
{ name: new BrsString("change"), value: new RoAssociativeArray([]) },
{ name: new BrsString("focusable"), value: BrsBoolean.False },
{ name: new BrsString("focusedChild"), value: BrsInvalid.Instance },
{ name: new BrsString("focusedchild"), value: BrsInvalid.Instance },
{ name: new BrsString("id"), value: new BrsString("") },
]);
result.elements.forEach((value, name) => {
Expand Down
12 changes: 12 additions & 0 deletions test/e2e/BrsComponents.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ describe("end to end brightscript functions", () => {
"true",
"can look up elements (brackets): ",
"true",
"can case insensitive look up elements: ",
"true",
"can check for existence: ",
"true",
"items() example key: ",
Expand All @@ -72,8 +74,18 @@ describe("end to end brightscript functions", () => {
"value1",
"lookup uses mode case too",
"value1",
"lookupCI ignore mode case",
"value1",
"can empty itself: ",
"true",
"saved key: ",
"DD",
"saved key after accessing by dot: ",
"dd",
"saved key after accessing by index: ",
"Dd",
"AA keys size: ",
"1",
]);
});

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/Iterables.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describe("end to end iterables", () => {
// iterate through keys
"has-second-layer",
"level",
"secondlayer",
"secondLayer",

// twoDimensional.secondLayer.level
"2",
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/RoSGNode.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ describe("components/roSGNode", () => {
"true",
"can look up elements (brackets): ",
"true",
"can case insensitive look up elements: ",
"true",
"can check for existence: ",
"true",
"can empty itself: ",
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/Syntax.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe("end to end syntax", () => {

expect(allArgs(outputStreams.stdout.write).filter((arg) => arg !== "\n")).toEqual([
// note: associative array keys are sorted before iteration
"createobject",
"createObject",
"in",
"run",
"stop",
Expand Down
11 changes: 11 additions & 0 deletions test/e2e/resources/components/roAssociativeArray.brs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ sub main()
print "can delete elements: " aa.delete("baz") ' => true
print "can look up elements: " aa.lookup("foo") = "foo" ' => true
print "can look up elements (brackets): " aa["foo"] = "foo" ' => true
print "can case insensitive look up elements: " aa.lookupCI("foO") = "foo" ' => true
lkipke marked this conversation as resolved.
Show resolved Hide resolved
print "can check for existence: " aa.doesExist("bar") ' => true
print "items() example key: " aa.items()[0].key ' => bar
print "items() example value: " aa.items()[0].value ' => 5
Expand All @@ -21,7 +22,17 @@ sub main()
print "key is not found if sensitive mode is enabled" aa.doesExist("key1") ' => false
print "key exits with correct casing" aa["KeY1"] ' => value1
print "lookup uses mode case too" aa.lookup("KeY1") ' => value1
print "lookupCI ignore mode case" aa.lookupCI("kEy1") ' => value1

aa.clear()
print "can empty itself: " aa.isEmpty() ' => true

' check mimic of RBI AA keys saving logic
aa = {"dd": 0, "DD":1}
print "saved key: "aa.keys()[0] ' => DD
aa.dd = 2
print "saved key after accessing by dot: "aa.keys()[0] ' => dd
aa["Dd"] = 3
print "saved key after accessing by index: "aa.keys()[0] ' => Dd
print "AA keys size: " aa.keys().count() ' => 1
end sub
1 change: 1 addition & 0 deletions test/e2e/resources/components/roSGNode/roSGNode.brs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ sub init()
print "can delete elements: " node1.delete("baz") ' => true
print "can look up elements: " node1.lookup("foo") = "foo" ' => true
print "can look up elements (brackets): " node1["foo"] = "foo" ' => true
print "can case insensitive look up elements: " node1.lookupCI("foO") = "foo" ' => true
print "can check for existence: " node1.doesExist("bar") ' => true

node1.clear()
Expand Down