Introduce PropertyKey for field acces, fix #172 (quotes around displayed strings)

[RageKnify]

No description provided.

[RageKnify]

I have no idea how, but changing the fmt method for ValueData::String broke something related to building Arrays, now new Array() returns undefined.
Any ideia on what I might be doing wrong?

[Razican]

I have no idea how, but changing the fmt method for ValueData::String broke something related to building Arrays, now new Array() returns undefined.
Any ideia on what I might be doing wrong?

This might be related to the fact that we use ToString in many places, and ToString is derived from Display. Maybe we somehow need to differentiate between our internal representation of a String and what we want to print in the screen on a console.log() for example.

@HalidOdat @jasonwilliams what do you think?

[jasonwilliams]

Maybe we somehow need to differentiate between our internal representation of a String and what we want to print in the screen on a console.log() for example.

Yes we certainly want this, even for debugging purposes.

[Razican]

Maybe we somehow need to differentiate between our internal representation of a String and what we want to print in the screen on a console.log() for example.

Yes we certainly want this, even for debugging purposes.

The question is why is this giving an error. Are we using a toString() somewhere to convert to a String type of value? I'll try to check it.

[IovoslavIovchev]

It doesn't seem to be related to toString(). For some reason when the change is made all of the globals are undefined. The issue might be the bindings don't get initialized at all or get_bindings_value isn't returning the proper value (both are wild guesses).

How and why this might even happen, however, is a mystery.

[Razican]

After reviewing what's happening here, I saw that many tests fail:

test result: FAILED. 111 passed; 89 failed; 6 ignored; 0 measured; 0 filtered out

So it seems that we use to_string() in many places to convert a string value to a string. This should probably be fixed somehow, but I don't think it will be easy. The current task is to fix the display of these values in the console, and I think that can be done by creating a ValueDisplay structure, that would also implement Display, and that we only use to show this in the console. It would just be a wrapper over Value, and could be created by calling a display() method in the Value.

What do you think?

[Razican]

Does this work now that #381 was merged?

[RageKnify]

Does this work now that #381 was merged?

No, the samer error remains, global objects are undefined.
a = 0; works, and I can do a+1.
But a = new Array() still returns undefined.

[RageKnify]

I have some free time again, I'd like to fix this.
Should I try to implement the solution proposed by @Razican ?

Create a wrapper struct, implementing Display that would be created upon calling display() on an instance of Value?
I'd then change the line:

boa/boa_cli/src/main.rs

Line 211 in d837e04

Ok(v) => println!("{}", v.to_string()),

to call v.display() instead, yes?

[Razican]

I have some free time again, I'd like to fix this.
Should I try to implement the solution proposed by @Razican ?

Create a wrapper struct, implementing Display that would be created upon calling display() on an instance of Value?
I'd then change the line:

boa/boa_cli/src/main.rs

Line 211 in d837e04

Ok(v) => println!("{}", v.to_string()),

to call v.display() instead, yes?

I think this is the best solution for now, yes. What do you think, @HalidOdat, @jasonwilliams ?

[HalidOdat]

I think this is the best solution for now, yes. What do you think, @HalidOdat, @jasonwilliams ?

I think so too!

[RageKnify]

Since log_string_from receives a &ValueData I made it a wrapper over &ValueData instead of over &Value so that it could be used in log_string_from.

boa/boa/src/builtins/value/mod.rs

Line 833 in d837e04

pub(crate) fn log_string_from(x: &ValueData, print_internals: bool) -> String {

I don't know in which places .display() should be invoked besides the main.rs of boa_cli.

Let me know what can be improved!

[HalidOdat]

Looks good!

About the log_string_from I think we should change it to take in a &Value instead of &ValueData

[RageKnify]

About the log_string_from I think we should change it to take in a &Value instead of &ValueData

I'm having an issue with this, the Display implementation of ValueData relies on log_string_from, and it doesn't have a &Value to give, it only has &ValueData, what should I do?

boa/boa/src/builtins/value/mod.rs

Line 979 in c8218dd

Self::Object(_) => write!(f, "{}", log_string_from(self, true)),

[HalidOdat]

I'm having an issue with this, the Display implementation of ValueData relies on log_string_from, and it doesn't have a &Value to give, it only has &ValueData, what should I do?

Hmmm. For now lets leave log_string_from how it is.

[RageKnify]

I have pushed it, but it leaves an issue, objects that have fields of type String won't be printed correctly:

let a = {0: "0"};
a

should display 0: "0" but instead shows 0: 0

[HalidOdat]

The problem is that we implement Display trait for Value (Values fmt function calls ValueDatas fmt function) and with display we we get ToString trait as an extra and we use that to_string which is meant for displaying, ulike the ECMASCript ToString function which returns a string representation. these are not the same that is why we get this weird behavior. The logic should be separate.

The ToStrings to_string method is not spec compliant!!

I'll create a PR addressing this issue ASAP.

[codecov]

Codecov Report

Merging #373 into master will increase coverage by 0.06%.
The diff coverage is 85.95%.

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
+ Coverage   71.04%   71.11%   +0.06%     
==========================================
  Files         177      176       -1     
  Lines       11439    11503      +64     
==========================================
+ Hits         8127     8180      +53     
- Misses       3312     3323      +11     

Impacted Files	Coverage Δ
boa/src/builtins/object/tests.rs	100.00% <ø> (ø)
boa/src/builtins/value/conversions.rs	53.33% <0.00%> (-4.21%)	⬇️
boa/src/realm.rs	65.21% <0.00%> (+2.71%)	⬆️
boa_cli/src/main.rs	5.88% <ø> (ø)
boa/src/builtins/string/mod.rs	42.66% <25.00%> (-0.36%)	⬇️
boa/src/builtins/property/mod.rs	41.74% <38.46%> (-2.56%)	⬇️
boa/src/builtins/object/internal_methods.rs	30.17% <39.47%> (-0.29%)	⬇️
boa/src/exec/object/mod.rs	46.66% <50.00%> (-0.40%)	⬇️
boa/src/builtins/value/mod.rs	69.14% <75.00%> (-0.50%)	⬇️
boa/src/exec/mod.rs	70.77% <90.90%> (+0.55%)	⬆️
... and 27 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a72ea...ae7fa2b. Read the comment docs.

[HalidOdat]

I followed @Razican's proposal for a PropertyKey enum, and propagated the change for functions that dealt with those.

This is definitely a step in the right direction :)

I don't we need a .set_str_field, why not just have set_field with the following signiture:

fn set_field(&mut self, field: F, value: V)
where
	F: Into<PropertyKey>,
	V: Into<Value>
{
	// ...
}

there are only a small amount of places where we call set_field with Value as field (GetField) and we should call .to_property_key() this is done in the spec so we should do the same in this case :)
see: https://tc39.es/ecma262/#sec-evaluate-property-access-with-expression-key

And also since we take a PropertyKey directly we don't need the interpreter anymore.

[HalidOdat]

@RageKnify I did the suggested changes, and fixed the test (it was a call to Display to_string that put the regex in ", which caused the error) here, RageKnify#1

[RageKnify]

Sorry, forgot to look at my notifications and tried doing what was done already.
I just left a comment to remove the dbg!() statements, they were just for you to find the bug, correct? @HalidOdat

[RageKnify]

I'll do some final touch ups and add some tests to increase coverage.

[RageKnify]

One thing I wanted to double check, when you applied your proposed changes you changed Value::set_field to return the field.into<Value>() rather than the value with which it was set @HalidOdat, on master it returns the value:

boa/boa/src/builtins/value/mod.rs

Line 658 in 5d4d8fe

val

Can I go ahead and set it to return the value while using a clone of the value as the argument for Object::set like it is in master?

[HalidOdat]

Can I go ahead and set it to return the value while using a clone of the value as the argument for Object::set like it is in master?

Good catch. Yes. I guess I made a mistake 😅

[RageKnify]

I wasn't sure how to test the more internal functions, if you think tests should be added and have any ideas let me know.

[HalidOdat]

I wasn't sure how to test the more internal functions, if you think tests should be added and have any ideas let me know.

What exactly do you mean by internal functions? if you mean display_obj, I don't think we need to, if it fails the other tests fails so it is covered.

[HalidOdat]

This looks great! Almost ready to merge :D

[RageKnify]

@HalidOdat should I inline the conversion functions that match on enums or only the ones without branching?
PS: I spotted some that did so I inlined all of the short ones.

[HalidOdat]

@HalidOdat should I inline the conversion functions that match on enums or only the ones without branching

Yes. If the function contains one condition/match (small) it is worth inlining than having function call. #[inline] is a hint to the compiler.

[HalidOdat]

Benchmarks

(index)	name	changesDuration	masterDuration	difference
0	Arithmetic operations (Execution)	368.8±23.77ns	365.7±19.90ns	+1.0
1	Arithmetic operations (Full)	176.4±10.00µs	176.6±8.54µs	0.0
2	Array access (Execution)	8.6±0.37µs	14.3±0.72µs	-40
3	Array access (Full)	190.3±10.93µs	201.0±10.18µs	-5.7
4	Array creation (Execution)	3.2±0.13ms	3.7±0.15ms	-13
5	Array creation (Full)	3.5±0.14ms	4.1±0.14ms	-14
6	Array pop (Execution)	1146.1±68.21µs	1451.0±62.37µs	-21
7	Array pop (Full)	1460.1±56.31µs	1818.3±103.17µs	-20
8	Boolean Object Access (Execution)	4.3±0.29µs	4.4±0.25µs	-2.9
9	Boolean Object Access (Full)	187.7±8.66µs	192.6±12.48µs	-2.9
10	Create Realm	164.3±35.47µs	159.0±14.39µs	+3.0
11	Dynamic Object Property Access (Execution)	5.5±0.27µs	7.0±0.38µs	-21
12	Dynamic Object Property Access (Full)	187.6±9.25µs	196.0±9.41µs	-3.8
13	Expression (Lexer)	2.4±0.16µs	2.4±0.11µs	0.0
14	Expression (Parser)	5.4±0.27µs	5.2±0.27µs	+4.0
15	Fibonacci (Execution)	1042.6±57.07µs	1097.6±64.93µs	-4.8
16	Fibonacci (Full)	1265.7±71.49µs	1314.8±74.64µs	-3.8
17	For loop (Execution)	23.6±1.36µs	23.8±1.22µs	-0.99
18	For loop (Full)	204.9±11.34µs	206.4±9.65µs	-0.99
19	For loop (Lexer)	5.3±0.26µs	5.3±0.27µs	-0.99

As expected there is a really nice increase in performance ~40 in array access and a ~20 dynamic object property access this is because we don't take the take a Value as the key but PropertyKey and we consume the string index in array instead of cloning :)

[HalidOdat]

This is good to go. this is going to make it easier to implementing #591

Thanks for the contribution @RageKnify :)