New with spread #3066

tinganho · 2015-05-07T09:07:40Z

Implements #2369. Adds ES5 support for spread operator in new expressions. ES3 targets should still output warning as before, because it doesn't support Function.prototype.bind which I use in this ES5 implementation. I refactored some of the code from previous call with spread implementation #1931 so I could reuse it to on my implementation.

I have also manually tested the permutations of the outputted JavaScript.

tinganho · 2015-05-07T12:35:47Z

I had some serious trouble with crlf and lf line endings.

I have to turn off autocrlf:

git config --global core.autocrlf false

In order for my test to pass. Have there been any experiencing the same problem as me? using Mac?

CyrusNajmabadi · 2015-05-07T13:46:29Z

Yes. Don't use autocrlf :) It's not appropriate for our project because line endings are an important thing we track and use in our tests.

CyrusNajmabadi · 2015-05-07T13:49:02Z

.gitignore

@@ -21,6 +21,7 @@ tests/services/baselines/local/*
 tests/baselines/prototyping/local/*
 tests/baselines/rwc/*
 tests/baselines/test262/*
+tests/baselines/reference/projectOutput/*


why was this added to the .gitignore?

I got multiple files outputted there every time I run jake runtests:

@CyrusNajmabadi i think this is a fair one to add, ppl have made this mistake multiple times already. mainly because of a mocha issue on node 0.10.*

yeah I don't think there's any real harm in having it

mhegazy · 2015-05-07T23:15:11Z

👍
@vladima and @JsonFreeman can you take a look.

tinganho · 2015-05-08T00:05:15Z

I probably should re-style my tests to use foo, bar, baz, x, y, z and A, B, C.

JsonFreeman · 2015-05-08T17:45:48Z

src/compiler/emitter.ts

@@ -1881,13 +1860,77 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                }
            }

+            function emitCallOrNewExpressionTarget(node: Expression): CallTarget {


I think it is a little subtle that this function emits one thing (the target), and then returns two things, one of which it emitted and one of which it did not. Can you make it just return the target and the accessor, and then the caller will emit the target?

+1. It should be named properly as well to convey that it's just computing hte call target, and not actually emitting anything.

@JsonFreeman sound like a good idea. I will address this later.

Just tried to address this. The reason I did it was so that I don't need to have multiple node kind checks — one for the getter and one for the emitter. We already have in emitCallOrNewExpressionTarget() the node kind checks:

if (node.kind === SyntaxKind.PropertyAccessExpression) { ... } else if (node.kind === SyntaxKind.ElementAccessExpression) { ... } else if (node.kind === SyntaxKind.SuperKeyword) { ... } else { ... }

If we would have one for getter and emitter the same node kind checks need to be applied mulitple times.

I don't have any perfect solution to this. Any idea how to fix this?

I just found a better way to do this. We can just call it emitCallTarget() and return the target and have a separate function for the emit of accessor or just inline the emit logic in emitNewExpression since we only use it there.

The problem is though, that it already exists one function with emitCallTarget(). Any chance I can just rename it to emitCallTargetWithOptionalTempAssignment()?

function emitCallTarget(node: Expression): Expression { if (node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.ThisKeyword || node.kind === SyntaxKind.SuperKeyword) { emit(node); return node; } let temp = createAndRecordTempVariable(TempFlags.Auto); write("("); emit(temp); write(" = "); emit(node); write(")"); return temp; }

Well the functionality below looks a lot like emitCallWithSpread, which calls the emitCallTarget you've mentioned above, and you want to now call this emitCallTarget, so we should find some unifying scheme.

Also, on the issue of subtlety in behavior - while the name is important, I'd rather see documentation, which we are currently lacking in. Regardless of what this is called, please add a nice JSDoc comment to point out its peculiar behavior.

I just tested my latest idea. It also requires additional node checks. emitCallWithSpread() also emits the accessor and writing the extra node checks is not worth it IMO. Can't we just rename it to emitCallTargetAndAccessor()? It optionally emits and optionally returns the target and accessor. It is the best in terms of DRY:ness and also number of operations/comparison for the job.

@tinganho I don't think we're opposed to it, but let's see what the change looks like; it's hard to get a feel of it without seeing the end result. Worst-case scenario, we'll just revert the commits

tinganho · 2015-05-12T07:56:45Z

Revision 2

I've just updated the function name to emitCallTargetAndAccessor to reflect the accessor too. It optionally emits and optionally returns the call target and accessor. I also added some comments about some very specific cases with the implementation. This also addresses a bug I found. I also changed the passed in object for the bind method from null to void 0. And the outputted JavaScript have been tested manually from my side.

Bug1 (accessor is a string literal)

new object["a-b"](1, 2, a..)

new (a_ = object)["a-b"].bind.apply(a_."a-b", [null]); // should be a["a-b"];

Bug2 (accessor is a numeric literal)

new object[1](1, 2, a..)

new (a_ = object)[1].bind.apply(a_.1, [null]); // should be a[1];

Old comments:
.gitignore
emitter.ts
types.ts

tinganho · 2015-05-13T02:25:39Z

@JsonFreeman I addressed the problems in the old revision.

I only use Function now and it works.

Also there was no wrong with the new expression call syntax new object.Array()(). But I added a test for it just in case.

Travis is pulling in a wrong commit that's why it is failing. But this one has the right commit. (I think I can only fix this with a new PR though) I contacted travis to address this.

JsonFreeman · 2015-05-13T17:47:53Z

@tinganho Thanks for iterating on this implementation so well. One thing that would be helpful in the future is when you change something, push the changes as new commits instead of amending your old commits. That way it is very easy to see what changed over the course of the discussion.

JsonFreeman · 2015-05-13T17:55:03Z

tests/baselines/reference/newWithSpreadES5.js

+// Call expression
+new f(1, 2, "string")();
+new (Function.bind.apply(f, [void 0].concat([1, 2].concat(a))))();
+new (Function.bind.apply(f, [void 0].concat([1, 2].concat(a, ["string"]))))();


I do think that these two lines are incorrect because these really translate to

new f(1, 2, ...a);

and not

new f(1, 2, ...a)();

I don't think I understand here can you elaborate? They have () at the end? Or does the outer parentheses (Function...) messing with it?

Yes, the outer parentheses are messing with it, because it causes the final () to be consumed as a part of the new expression. You need an extra () to make it a call expression. So you should emit:

new (Function.bind.apply(f, [void 0].concat([1, 2].concat(a))))()();

@JsonFreeman I guess if the call have arguments it should be compiled to:

new (Function.bind.apply(f, [void 0].concat([1, 2].concat(a))))()(a, b);

JsonFreeman · 2015-05-13T18:01:24Z

The one caveat with using Function as I suggested is that there could be a local declaration called Function that shadows the global function. Maybe the Function idea won't work then 😞. Unless we emit some sort of capture, which is overkill I think. Do you think we should go back to the temp idea? Sorry to be fickle.

tinganho · 2015-05-14T00:19:50Z

Do you think we should go back to the temp idea? Sorry to be fickle.

No problem, Ok I can change to your other idea with using temp for "target+accessor".

I will push them as new commits instead of cleaned ones.

JsonFreeman · 2015-05-14T00:39:14Z

Thanks. In terms of the implementation code, I guess my suggestion is that instead of using emitCallTargetAndAccessor, you just call emitCallTarget (the already existing one), and pass in node.expression as the argument (where node is the new expression).

That way you don't need to worry about the target and accessor.

tinganho · 2015-05-14T00:49:46Z

hmm, I will try it out and see if there are any problem.

JsonFreeman · 2015-05-14T00:52:48Z

I think it should still give you (a_ = object.array) as the temp

…ion in ES5

tinganho · 2015-05-15T00:41:47Z

@JsonFreeman it is still pulling in the wrong commit bde2491 instead of b88d542

JsonFreeman · 2015-05-15T00:51:07Z

Let's ask @DanielRosenwasser, since he knows more about Travis

DanielRosenwasser · 2015-05-15T00:51:50Z

@tinganho I pulled down your branch and I'm getting the same error:

node bin/tsc.js --module commonjs -noImplicitAny --preserveConstEnums --out built\local\tsc.js -sourcemap -mapRoot file:///C:\Users\drosen\tinganho\built\local src\compiler\core.ts src\compiler\sys.ts src\compiler\types.ts src\compiler\scanner.ts src\compiler\parser.ts src\compiler\utilities.ts src\compiler\binder.ts src\compiler\checker.ts src\compiler\declarationEmitter.ts src\compiler\emitter.ts src\compiler\program.ts src\compiler\commandLineParser.ts src\compiler\tsc.ts src\compiler\diagnosticInformationMap.generated.ts

src/compiler/emitter.ts(1916,21): error TS2346: Supplied parameters do not match any signature of call target.

This is related to some recent changes; you need to add the parameter alwaysCopy for emitListWithSpread.

DanielRosenwasser · 2015-05-15T00:52:22Z

src/compiler/emitter.ts

+                    write(".bind.apply(");
+                    emit(target);
+                    write(", [void 0].concat(");
+                    emitListWithSpread(node.arguments, /*multiline*/false, /*trailingComma*/false);


Can you add spaces after each of these comments?

tinganho · 2015-05-15T00:52:32Z

@DanielRosenwasser I could get the errors now. Let me just fix it.

JsonFreeman · 2015-05-15T01:02:35Z

Oh wait, I noticed that

new f(...a, ...b);

will emit as

new (f.bind.apply(f, [void 0].concat(a.concat(b)))();

And with alwaysCopy set to true, it would do

new f(...a)

as

new (f.bind.apply(f, [void 0].concat(a.slice()))();

Can you confirm that this is what happens? This is not what we want, we don't want the slice or the second concat.

Can you add tests for this if it is not already affected?

DanielRosenwasser · 2015-05-15T01:05:02Z

src/compiler/emitter.ts

+                    write(".bind.apply(");
+                    emit(target);
+                    write(", [void 0].concat(");
+                    emitListWithSpread(node.arguments, /*alwaysCopy*/true, /*multiline*/false, /*trailingComma*/false);


Can you actually rename alwaysCopy to needsUniqueCopy and replace the relevant comments elsewhere? I realized that we don't actually always copy if the initial element is an array literal.

Yes I can rename it.

JsonFreeman · 2015-05-15T01:06:23Z

I think the reason the tests did not catch this is because all the tests have at least one non-spread argument before the spread.

tinganho · 2015-05-15T01:07:46Z

@JsonFreeman I was a little bit fast with the the push button. I think alwaysCopy should be set to false instead of true. Because it was false before.

I can add a test for it.

JsonFreeman · 2015-05-15T01:09:27Z

Sure, but you will still have the double concat problem.

tinganho · 2015-05-15T01:15:07Z

@JsonFreeman is that doable to do f(...a, ...b)? It throws an error now that a spread parameter can only be at the end of a parameter list.

If it is doable what should the outputted code be like?

JsonFreeman · 2015-05-15T06:41:29Z

Regarding f(...a, ...b). It is not legal to define a function with a rest parameter that is not at the end. So you can't define a function like this:

function f(...a, ...b) { }

But I meant if you have a function called f, and you call it with two spread arguments:

function f(...p: any[]) { }
var a: string[];
var b: string[];
f(...a, ...b);

And that is legal.

tinganho · 2015-05-15T07:34:13Z

Sure, but you will still have the double concat problem.

@JsonFreeman I just tested this and it is outputting:

new (f.bind.apply(f, [void 0].concat(a.concat(b)))();

I can't see what's wrong with it? What is the proposed emit? Or should b not be concatenated?

JsonFreeman · 2015-05-15T18:47:10Z

That emit works, but I think it's nicer to emit:

new (f.bind.apply(f, [void 0].concat(a, b)))();

You can achieve this by passing a boolean to emitListWithSpread to make it suppress the concat.

tinganho · 2015-05-16T08:50:31Z

Ok I fixed it on a new commit.

JsonFreeman · 2015-05-16T23:45:58Z

Sweet! Thanks @tinganho! I sign off. Please fix my one nit, and then I can merge it in.

tinganho · 2015-05-17T01:36:15Z

OK @JsonFreeman I fixed it on a new commit. Sorry for not seeing it earlier.

New with spread

JsonFreeman · 2015-05-18T17:23:48Z

Thanks @tinganho!

msftclas added the cla-already-signed label May 7, 2015

tinganho force-pushed the newWithSpread branch 3 times, most recently from d34674a to ce95286 Compare May 7, 2015 12:23

CyrusNajmabadi reviewed May 7, 2015
View reviewed changes

JsonFreeman reviewed May 8, 2015
View reviewed changes

tinganho force-pushed the newWithSpread branch 7 times, most recently from acfae7d to 94dc37f Compare May 13, 2015 02:07

JsonFreeman reviewed May 13, 2015
View reviewed changes

tinganho added 3 commits May 15, 2015 08:38

Corrects old tests.

a97ea4b

Changes to emitCallTarget

1be34f1

Adds an always emit of pair of parens in spread syntax in new express…

b88d542

…ion in ES5

tinganho force-pushed the newWithSpread branch from 59c2ad7 to b88d542 Compare May 15, 2015 00:39

DanielRosenwasser reviewed May 15, 2015
View reviewed changes

Adds alwaysCopy argument

fb63aea

DanielRosenwasser reviewed May 15, 2015
View reviewed changes

Renamed alwaysCopy to needsUniqueCopy

e9f8a6f

Saved

629b4bb

Fixes concat style

78a36c0

Combining else blocks

ccd3de3

JsonFreeman added a commit that referenced this pull request May 18, 2015

Merge pull request #3066 from tinganho/newWithSpread

5bea6a9

New with spread

JsonFreeman merged commit 5bea6a9 into microsoft:master May 18, 2015

microsoft locked and limited conversation to collaborators Jun 18, 2018

New with spread #3066

New with spread #3066

Conversation

tinganho commented May 7, 2015

tinganho commented May 7, 2015

CyrusNajmabadi commented May 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented May 7, 2015

tinganho commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinganho commented May 12, 2015

Revision 2

tinganho commented May 13, 2015

JsonFreeman commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsonFreeman commented May 13, 2015

tinganho commented May 14, 2015

JsonFreeman commented May 14, 2015

tinganho commented May 14, 2015

JsonFreeman commented May 14, 2015

tinganho commented May 15, 2015

JsonFreeman commented May 15, 2015

DanielRosenwasser commented May 15, 2015

Choose a reason for hiding this comment

tinganho commented May 15, 2015

JsonFreeman commented May 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsonFreeman commented May 15, 2015

tinganho commented May 15, 2015

JsonFreeman commented May 15, 2015

tinganho commented May 15, 2015

JsonFreeman commented May 15, 2015

tinganho commented May 15, 2015

JsonFreeman commented May 15, 2015

tinganho commented May 16, 2015

JsonFreeman commented May 16, 2015

tinganho commented May 17, 2015

JsonFreeman commented May 18, 2015