Remove PropertyKey from Value, String, Integer, ... conversions in favour of const methods.

[aapoalas]

We rely on an assumption that number-like PropertyKeys show up as Integer and that property keys can be compared by-value so that eg. "0" == 0 but the From and TryFrom implementations of PropertyKey do not actually check if strings are number-like.

As a result, ProperyKey::from("0") != PropertyKey::from(0) which is of course not correct.

This sort of points to the direction that the From<Value> and From<String> type of conversions for PropertyKey are invalid: We cannot freely and cheaply convert a string into a PropertyKey without first checking its contents. It would thus be better to remove these String and Value conversions and instead implement functions that do the same but also take an &Agent parameter so as to be capable of checking the string data.

[nair-sreerag]

Hi @aapoalas, is this bug fixed? I tried it in js and it worked as expected. If its not, how can i reproduce it?

Below is the code I ran it on -

const obj = {};

obj["0"] = "string zero";
console.log("obj[0]:", obj[0]); // Should print "string zero"
console.log("obj['0']:", obj["0"]); // Should print "string zero"
console.log("obj[0] === obj['0']:", obj[0] === obj["0"]);  // should return true

[aapoalas]

This is mostly fixed; I think it will might still appear with numbers larger than 7 characters in length, like obj[12345678].

[nair-sreerag]

Will try to reproduce it then.
Is the approach in the sample code correct though?

[aapoalas]

Yeah, that seems correct. There is a further complication that the PropertyKey <-> Integer <-> String <-> Value paths are somewhat split here-and-there in the engine code, a lot of code will eg. just use PropertyKey::from(some_string) which can perform the correct conversion for up to 7 character integer strings but not for any others. So, it's possible that your test code will actually function correctly but in some more obscure case it will fail, eg. if the "12345678" string is first assigned to a variable:

const obj = {};
const str = "12345678"
const int = 12345678;

obj[str] = "string zero";
console.log("obj[int]:", obj[int]); // Should print "string zero"
console.log("obj[str]:", obj[str]); // Should print "string zero"
console.log("obj[int] === obj[str]:", obj[int] === obj[str]);  // should return true

[nair-sreerag]

This seems to be working too. I am getting true in the last console.log()
so just to be clear, I run the js file using the command -

cargo run eval test.js

[aapoalas]

Aight, then at least the immediate issue has been fixed. We might have some of this still lurking here and there, but probably not quite so much of it.

[nair-sreerag]

Seems to be the case.