Object.assign() should copy undefined properties, I think

[dcitron]

Hi! It looks like Object.assign() should copy undefined properties:

console.log(Object.assign({"a":undefined}, {"b":undefined}))
Object { a: undefined, b: undefined }

However, the Rhino code is skipping undefined values:

          case ConstructorId_assign:
          {
            if (args.length < 1) {
              throw ScriptRuntime.typeError1("msg.incompat.call", "assign");
            }
            Scriptable targetObj = ScriptRuntime.toObject(cx, thisObj, args[0]);
            for (int i = 1; i < args.length; i++) {
              if ((args[i] == null) || Undefined.isUndefined(args[i])) {
                continue;
              }
              Scriptable sourceObj = ScriptRuntime.toObject(cx, thisObj, args[i]);
              Object[] ids = sourceObj.getIds();
              for (Object key : ids) {
                if (key instanceof String) {
                  Object val = sourceObj.get((String) key, sourceObj);
                  if ((val != Scriptable.NOT_FOUND) && !Undefined.isUndefined(val)) { // <=== HERE
                    targetObj.put((String) key, targetObj, val);
                  }
                } else if (key instanceof Number) {
                  int ii = ScriptRuntime.toInt32(key);
                  Object val = sourceObj.get(ii, sourceObj);
                  if ((val != Scriptable.NOT_FOUND) && !Undefined.isUndefined(val)) {  // <=== AND HERE
                    targetObj.put(ii, targetObj, val);
                  }
                }
              }
            }
            return targetObj;
          }

Is there a reason for those !Undefined.isUndefined(val) checks?

Thanks!

[tonygermano]

https://www.ecma-international.org/ecma-262/11.0/index.html#sec-object.assign

Section 19.1.2.1, step 4.a.iii.2 seems to indicate the check is appropriate.

[dcitron]

Thanks for replying, Tony! I'm not sure that's a correct reading of that spec ...

From earlier in the doc, "[[GetOwnProperty]]" is described as:

Return a Property Descriptor for the own property of this object whose key is propertyKey, or undefined if no such property exists.

So when reading this section:

iii. For each element nextKey of keys in List order, do

1. Let desc be ? from.[[GetOwnProperty]](nextKey).
2. If desc is not undefined and desc.[[Enumerable]] is true, then
   a. Let propValue be ? Get(from, nextKey).
   b. Perform ? Set(to, nextKey, propValue, true).

I believe that it is correct to say that desc is defined, but propValue is undefined, and in that case, the property should still be copied. Am I confused?

SpiderMonkey and V8 both copy properties with undefined values, too (not that that's necessarily definitive).

Thoughts?
Thanks!

[gbrail]

Thanks for the analysis of the spec. You may indeed be right.

I'm sure that there are tests in the "test262" directory for this. Some of them may be disabled (in test262.properties) which is usually a sign that we missed something. Perhaps if you try looking to see what's enabled and enabling anything that might be disabled regarding Object.assign, we might be able to quickly zero on on the correct behavior.

[tonygermano]

@dcitron I'm not experienced with reading these specs nor am I an expert on javascript internals. I do think your interpretation is correct. I am still a bit confused about how desc could be undefined when nextKey is an element of keys, which in turn is the list of from.OwnPropertyKeys. Wouldn't the property have to exist, and therefore desc would never be undefined, or is there some case that I'm not thinking of?

[tuchida]

Perhaps there aren't tests in the latest "test262".
https://github.com/tc39/test262/tree/b2e9dff2816cceb5ee84c0c226c50a31d01a7297/test/built-ins/Object/assign
I greped undefined in this directory.

[p-bakker]

My reading of the spec is that the If desc is not undefined is just a spec safeguard for the unlikely scenario where OwnPropertyKeys returns a key for which GetOwnProperty returns undefined. Not sure if that is an invariant against which the spec has put measures in place, but otherwise such a scenario could occur with a Proxy object

Regardless, the spec is pretty clear that there should NOT be a check on the value of the property, so assign should copy over properties with an undefined value

[rbri]

Sorry but i have to reopen this issue. tuchida's fix seems to introduce a new problem. Have applied this change to HtmlUnit and one library tests failed.
Spend some time to find the reason. Will make a pr with a test case.

Maybe @tuchida can have a look (please).

[p-bakker]

@tuchida are you looking into this?

[tuchida]

Yes, I look it lightly. However, I could not proceed because I was not sure which specification Scriptable.put, ScriptableObject.putProperty, etc. each corresponded to (e.g. [[Set]]).
Sorry, but if anyone can fix this, it would be helpful.

[dcitron]

Hi, @tuchida -- since this was all due to me in the first place, lemme take a look and see if I can come to any conclusions. I'll start with @rbri 's test case :)

[rbri]

@tuchida @dcitron great - thanks

[dcitron]

Apologies for the delay! I haven't forgotten! I'll try to get to it this coming week.

[dcitron]

Hi @tuchida and @rbri ! I have a fix that passes all ./gradelw test tests against rbri:assign_with_prototoype -- in other words, it should fix #1195

Here's what I found: the current code is not perfectly following the flow specified by https://262.ecma-international.org/12.0/#sec-ordinarysetwithowndescriptor

What's happening right now (if I'm correct!) is:

• the two new AbstractEcmaObjectOperations.put() functions that were added in fix #780 fix Object.assign when undefined value and inextensible #1186 are implementing Step 2 above, and walking up to the parent (prototype) object to call parent.putImpl(..., Receiver, ...)
• but ScriptableObject.putImpl() is not creating/setting the requested property on the Receiver
  • in the case that this != start it is grabbing the parent (prototype) slot and doing nothing with it -- the ultimate call to slot.setValue() is a no-op if the owner and the "start" (Receiver) are not the same

My fix involves doing the right thing at step 3.d.iv and 3.d.ii in the picture above, which means:

• if this is not the Receiver
  • if the property is a value (not a setter) and is not read-only, invoke Receiver.putImpl() to allow the correct (start) object to get the intended property.

I hope any of that makes any kind of sense! In short, my change is to add this code here:

I also made a small change to Slot.java to factor out isReadOnly() into a function to be called there.

Question!!! How should I proceed? I think I cannot push changes directly to rbri:assign_with_prototoype, right?

Should I just make my own fork and my own branch and push my changes plus @rbri's test case and make a whole new PR? What's the correct procedure here?

[p-bakker]

Have no pointers on what the best way to proceed is, just wanted to say: great analyses!

Note that this fix might also fix some test262 tests that are currently not passing. You could check that by running the test262 TestSuite and see if any such tests are reported. If yes, you can regenerate test262.properties and include the up[dated version in the eventual PR. See https://github.com/mozilla/rhino/tree/master/testsrc for the docs about doing so

[rbri]

Many thanks!

Should I just make my own fork and my own branch and push my changes plus @rbri's test case and make a whole new PR? What's the correct procedure here?

I think this will be the right way.

[dcitron]

Great! I'll do that and I'll see about the test262 suite as well.

[dcitron]

Note that this fix might also fix some test262 tests that are currently not passing. You could check that by running the test262 TestSuite and see if any such tests are reported.

Hi, @p-bakker -- I tried to do that, but it didn't report anything. Did I miss a step or does this mean that my fix didn't fix any of the test262 tests?

$ ./gradlew test --tests org.mozilla.javascript.tests.Test262SuiteTest --rerun-tasks -DupdateTest262properties

> Task :compileTestJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

BUILD SUCCESSFUL in 4m 36s
5 actionable tasks: 5 executed

$ git status testsrc/test262.properties 
nothing to commit, working tree clean

[dcitron]

Do I need to do anything else or is it sufficient that the PR is currently open?

[rbri]

Sorry but we are all overloaded with work - will have a look at the weekend

[dcitron]

Sorry but we are all overloaded with work - will have a look at the weekend

No rush at all! It took me forever and a day to even fix it! I just wanted to make sure I wasn't still blocking anything :)

[rbri]

@gbrail, @p-bakker, @tuchida what do you think? Can i merge this?

[rbri]

Juste made another PR for this (#1294) will try both suggested solutions using the HtmlUnit test suite and report the results here.

[rbri]

Have done some tests with the new PR (#1294) in HtmlUnit

Works fine - the related problem is fixed with this and the performance is unchanged.
From my point of view we should go with this one.

@gbrail @p-bakker @tuchida @dcitron what do you think?

[rbri]

PR #1294 fixes this (hopefully) finally