Fix global objects attributes #624

54k1 · 2020-08-11T01:39:57Z

This Pull Request fixes/closes #606 .

It changes the following:

Rename insert_property to insert.
Remove insert_field.
Add new insert method which takes Value, Attribute and inserts a property.

1. Rename insert_property to insert. 2. Remove insert_field. 3. Add new insert method which takes Value, Attribute and inserts a property.

codecov · 2020-08-11T01:47:25Z

Codecov Report

Merging #624 into master will decrease coverage by 0.25%.
The diff coverage is 70.18%.

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
- Coverage   72.50%   72.24%   -0.26%     
==========================================
  Files         179      179              
  Lines       13376    13411      +35     
==========================================
- Hits         9698     9689       -9     
- Misses       3678     3722      +44

Impacted Files	Coverage Δ
boa/src/builtins/array/mod.rs	`76.13% <50.00%> (-0.32%)`	⬇️
boa/src/builtins/bigint/mod.rs	`67.74% <50.00%> (-2.26%)`	⬇️
boa/src/builtins/boolean/mod.rs	`34.37% <50.00%> (-2.30%)`	⬇️
boa/src/builtins/console/mod.rs	`29.05% <50.00%> (-0.33%)`	⬇️
boa/src/builtins/date/mod.rs	`56.13% <50.00%> (-0.22%)`	⬇️
boa/src/builtins/error/mod.rs	`29.62% <50.00%> (-2.38%)`	⬇️
boa/src/builtins/error/range.rs	`70.83% <50.00%> (-6.44%)`	⬇️
boa/src/builtins/error/reference.rs	`70.83% <50.00%> (-6.44%)`	⬇️
boa/src/builtins/error/syntax.rs	`33.33% <50.00%> (-3.04%)`	⬇️
boa/src/builtins/error/type.rs	`70.83% <50.00%> (-6.44%)`	⬇️
... and 17 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 14d7791...df2dd48. Read the comment docs.

boa/src/builtins/function/mod.rs

boa/src/builtins/math/mod.rs

boa/src/builtins/object/internal_methods.rs

boa/src/builtins/mod.rs

boa/src/builtins/array/mod.rs

boa/src/builtins/nan/mod.rs

boa/src/builtins/infinity/mod.rs

HalidOdat

This looks ready to merge, just needs a rebase :)

Lan2u · 2020-08-22T11:45:42Z

Are there any tests demonstrating the new behaviour?

HalidOdat

This looks almost ready :) Check my comments on how to improve it

Also as @Lan2u said having some test to prove that it the object attributes are set would be nice, something like:

Infinity = 3;

and asserting that Infinity did not change (this tests that its readonly), another test is to do:

delete Infinity;

And asserting its sill there (tests that it is permanent)

globalThis.propertyIsEnumerable("Infinity")

and asserting that it is false (tests that it is non_enumerable)

HalidOdat · 2020-08-28T20:08:40Z

boa/src/builtins/value/mod.rs

@@ -486,7 +486,8 @@ impl Value {
        let _timer = BoaProfiler::global().start_event("Value::update_property", "value");

        if let Some(ref mut object) = self.as_object_mut() {
-            object.insert_property(field, new_property);
+            // FIXME overwrite the property


Could you explain this comment? doesn't insert overwrite it?

HalidOdat · 2020-08-28T20:10:06Z

boa/src/builtins/object/internal_methods.rs

+    pub(crate) fn insert<K>(&mut self, key: K, property: Property)
    where
        K: Into<PropertyKey>,
    {
        match key.into() {
-            PropertyKey::Index(index) => self.indexed_properties.insert(index, property),
+            PropertyKey::Index(index) => {
+                self.indexed_properties.insert(index, property);
+            }
            PropertyKey::String(ref string) => {
-                self.string_properties.insert(string.clone(), property)
+                self.string_properties.insert(string.clone(), property);
            }
            PropertyKey::Symbol(ref symbol) => {
-                self.symbol_properties.insert(symbol.clone(), property)
+                self.symbol_properties.insert(symbol.clone(), property);
            }
        }
    }


it would be better if .insert returned an Option<Property>

HalidOdat · 2020-08-28T20:11:58Z

boa/src/builtins/object/internal_methods.rs

+    pub(crate) fn insert_property<K, V>(
+        &mut self,
+        key: K,
+        value: V,
+        attribute: Attribute,
+    ) -> Option<Property>
    where
        K: Into<PropertyKey>,
+        V: Into<Value>,
    {
-        self.insert_property(
-            key.into(),
-            Property::data_descriptor(
-                value,
-                Attribute::WRITABLE | Attribute::ENUMERABLE | Attribute::CONFIGURABLE,
-            ),
-        )
+        let property = Property::data_descriptor(value.into(), attribute);
+        match key.into() {
+            PropertyKey::Index(index) => self.indexed_properties.insert(index, property),
+            PropertyKey::String(ref string) => {
+                self.string_properties.insert(string.clone(), property)
+            }
+            PropertyKey::Symbol(ref symbol) => {
+                self.symbol_properties.insert(symbol.clone(), property)
+            }
+        }
    }


If insert retruns an Option<Property> we could do:

let property = Property::data_descriptor(value.into(), attribute); self.insert(key.into(), property)

This would eliminate some code duplication.

HalidOdat · 2020-10-01T19:13:08Z

Closing this in favor of #722

Alter property insert API

2385950

1. Rename insert_property to insert. 2. Remove insert_field. 3. Add new insert method which takes Value, Attribute and inserts a property.

HalidOdat reviewed Aug 11, 2020

View reviewed changes

Make insert private and prettify

0098063

54k1 marked this pull request as ready for review August 11, 2020 15:12

HalidOdat reviewed Aug 16, 2020

View reviewed changes

boa/src/builtins/mod.rs Outdated Show resolved Hide resolved

HalidOdat added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution labels Aug 16, 2020

HalidOdat added this to the v0.10.0 milestone Aug 16, 2020

54k1 and others added 2 commits August 17, 2020 11:29

Update init functions

3b67025

Merge branch 'master' into issue/606

79af0aa

HalidOdat reviewed Aug 17, 2020

View reviewed changes

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved

boa/src/builtins/nan/mod.rs Outdated Show resolved Hide resolved

boa/src/builtins/infinity/mod.rs Outdated Show resolved Hide resolved

Change init signature

daa36ca

HalidOdat approved these changes Aug 19, 2020

View reviewed changes

HalidOdat requested review from Razican, jasonwilliams and Lan2u August 19, 2020 21:30

Merge branch 'master' into issue/606

df2dd48

54k1 requested a review from HalidOdat August 22, 2020 06:35

HalidOdat changed the title ~~Alter property insert API~~ Fix global objects attributes Aug 28, 2020

HalidOdat reviewed Aug 28, 2020

View reviewed changes

jasonwilliams modified the milestones: v0.10.0, v0.11.0 Sep 28, 2020

HalidOdat closed this Oct 1, 2020

HalidOdat removed this from the v0.11.0 milestone Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix global objects attributes #624

Fix global objects attributes #624

54k1 commented Aug 11, 2020

codecov bot commented Aug 11, 2020 •

edited

Loading

HalidOdat left a comment

Lan2u commented Aug 22, 2020

HalidOdat left a comment

HalidOdat Aug 28, 2020

HalidOdat Aug 28, 2020

HalidOdat Aug 28, 2020

HalidOdat commented Oct 1, 2020

Fix global objects attributes #624

Fix global objects attributes #624

Conversation

54k1 commented Aug 11, 2020

codecov bot commented Aug 11, 2020 • edited Loading

Codecov Report

HalidOdat left a comment

Choose a reason for hiding this comment

Lan2u commented Aug 22, 2020

HalidOdat left a comment

Choose a reason for hiding this comment

HalidOdat Aug 28, 2020

Choose a reason for hiding this comment

HalidOdat Aug 28, 2020

Choose a reason for hiding this comment

HalidOdat Aug 28, 2020

Choose a reason for hiding this comment

HalidOdat commented Oct 1, 2020

codecov bot commented Aug 11, 2020 •

edited

Loading