Skip to content

[Transforms] fix _this = this capture emitted before "use strict" directives in AMD module output #7953

Merged
yuit merged 4 commits into
transformsfrom
transforms_fix7913
Apr 14, 2016
Merged

[Transforms] fix _this = this capture emitted before "use strict" directives in AMD module output #7953
yuit merged 4 commits into
transformsfrom
transforms_fix7913

Conversation

@yuit
Copy link
Copy Markdown
Contributor

@yuit yuit commented Apr 8, 2016

Fix issue #7913

Comment thread src/compiler/factory.ts Outdated
}

target.push(source[i]);
target.unshift(source[i]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this reverse all your prologues?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will put the prologues as the first statement in the result array. I use unshift here because there is no guarantee in the order of this function when it gets call.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but if you have existing 3 prologues, they'll be reversed.

var x = [];
[1,2,3].forEach(e => x.unshift(e))

x will have [3, 2, 1].

This function should only be getting called when adding to the beginning of an empty array anyway, so just document that and add an assertion if that's your concern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean now. But there are the same and it will not matter right? also if the prologue is sprinkle around the body function, it will be aggregate to the top so that should be ok as well?

The function is not necessary call at the beginning of an empty array. Like below the target statements-array may not be empty

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But prologues need to go at the beginning, so what situations will you have where you're adding prologues other than to the beginning of the array? Any time this function gets called after anything, that's probably a bad sign.

But there are the same and it will not matter right?

Apart from preserving what the user wrote the best we can, that's unfortunately not necessarily true. From the spec:

Implementations may define implementation specific meanings for ExpressionStatement productions which are not a Use Strict Directive and which occur in a Directive Prologue.

So you don't actually know what your engine will want to do for some prologues and whether they are order-dependent. For instance, there used to be uncertainty on whether the "use asm directive should be allowed to come before a "use strict". https://bugzilla.mozilla.org/show_bug.cgi?id=877908

… the first statement in the result statements array
@yuit yuit changed the title formTransforms fix7913 [Transforms] fix _this = this capture emitted before "use strict" directives in AMD module output Apr 11, 2016
@yuit yuit added the Domain: API: Transforms Relates to the public transform API label Apr 11, 2016
@sandersn
Copy link
Copy Markdown
Member

👍

Comment thread src/compiler/factory.ts Outdated

/**
* Add any necessary prologue-directives into target statement-array.
* The function needs to be called during each transformation steps.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*step
Also, you should probably modified the comment to mention that it needs to be called whenever we transform the statement list of a source file, namespace, or function-like body.

@rbuckton
Copy link
Copy Markdown
Contributor

👍

1 similar comment
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Apr 13, 2016

👍

@yuit yuit merged commit d56ac44 into transforms Apr 14, 2016
@yuit yuit deleted the transforms_fix7913 branch April 14, 2016 16:41
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Domain: API: Transforms Relates to the public transform API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants