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;
}
77 changes: 63 additions & 14 deletions src/brsTypes/components/RoAssociativeArray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte

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 +34,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 +76,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 +92,51 @@ 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);
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) {
let useCaseSensitiveSearch = this.modeCaseSensitive && isCaseSensitiveFind;
elementKey = useCaseSensitiveSearch ? elementKey : elementKey.toLowerCase();
if (this.elements.has(elementKey)) {
return elementKey;
}

let realElementKey = undefined;
for (let key of this.elements.keys()) {
if ((useCaseSensitiveSearch ? key : key.toLowerCase()) === elementKey) {
realElementKey = key;
break;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This seems less than ideal, as it'll cause an O(n) lookup for every access once setCaseInsensitive is called. I'm curious if there's an alternative solution we can take that won't require us to iterate across every element every time. Could we maintain a case-insensitive Map next to the case-sensitive one, and write to both for every set()? It'd of course require twice as much memory, but would maintain constant time lookups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I even forget about it, a great catch.
So I've taken your idea of using a second map and used it for mapping lower case keys to case sensitive keys. so it will not consume so much memory, will not introduce additional computation and almost saves previous performance.

but there is still be O(n) when we delete keys in case sensitive mode, cuz we can delete key that was used as alias in KeysMap

return realElementKey;
}

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 @@ -125,7 +156,8 @@ 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;
return BrsBoolean.from(deleted);
},
});
Expand All @@ -142,7 +174,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, true);
lkipke marked this conversation as resolved.
Show resolved Hide resolved
return BrsInvalid.Instance;
},
});
Expand All @@ -166,7 +198,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 +215,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,8 +258,22 @@ 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;
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps I've missed something — why's this variable lKey while the rest are key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk why, but it was as is before my changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

return this.get(new BrsString(lKey), 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,
Expand All @@ -242,7 +291,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
36 changes: 36 additions & 0 deletions test/brsTypes/components/RoAssociativeArray.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,5 +374,41 @@ 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 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
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(v1);

result = lookupCI.call(interpreter, new BrsString("KeY1"));
expect(result).toBe(v1);
});
});
});
});
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
10 changes: 10 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 @@ -74,6 +76,14 @@ describe("end to end brightscript functions", () => {
"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
10 changes: 10 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 @@ -24,4 +25,13 @@ sub main()

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