Skip to content

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented May 8, 2018

Fixes #23732

looks a easy fix
but something need to consider: some temporary variable has swapped and name has changed

@Kingwl Kingwl force-pushed the emit-var-at-top branch from e20fcd8 to a553d3e Compare May 8, 2018 06:16
@Kingwl
Copy link
Contributor Author

Kingwl commented May 8, 2018

is that my fault?

@@ -677,8 +677,8 @@ namespace ts {
const trailingStatements = endLexicalEnvironment();
if (statementOffset > 0 || some(statements) || some(trailingStatements)) {
const block = convertToFunctionBody(body, /*multiLine*/ true);
addRange(statements, block.statements.slice(statementOffset));
addRange(statements, trailingStatements);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the change in position, leadingStatements would be a more appropriate term now.

@Kingwl
Copy link
Contributor Author

Kingwl commented May 9, 2018

why travis-ci fault again😂

Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

@Kingwl this is a good change, but upon further review it seems like its not comprehensive. There are a number of other places where we are still adding hoisted declarations at the end of the scope rather than at the beginning.

If you search for all references to endLexicalEnvironment you'll see a number of additional cases in:

  • src/compiler/transformers/es2015.ts
  • src/compiler/transformers/es2017.ts
  • src/compiler/transformers/esnext.ts
  • src/compiler/transformers/generators.ts
  • src/compiler/transformers/module.ts
  • src/compiler/transformers/system.ts
  • src/compiler/transformers/ts.ts

Most (but not all) of these cases look something like this:

addRange(statements, endLexicalEnvironment());

To fully resolve these (and consider the linked issue closed), I would recommend adding the following function to src/compiler/core.ts (after the existing addRange function):

export function prependRange<T>(to: T[], from: ReadonlyArray<T> | undefined) {
    if (some(from)) to.unshift(...from);
}

Then I would replace these specific addRange calls with prependRange.

@Kingwl Kingwl force-pushed the emit-var-at-top branch 2 times, most recently from 87903a9 to b6d06ef Compare May 10, 2018 07:54
@Kingwl Kingwl force-pushed the emit-var-at-top branch from b6d06ef to ad5a4c7 Compare May 10, 2018 07:55
@Kingwl Kingwl changed the title emit templory vars at the top of the scope emit temporary vars at the top of the scope May 10, 2018
@Kingwl
Copy link
Contributor Author

Kingwl commented May 10, 2018

what the hell is that ci test

@rbuckton
Copy link
Contributor

Thank you for the contribution!

@rbuckton rbuckton merged commit f7311ef into microsoft:master May 10, 2018
@Kingwl Kingwl deleted the emit-var-at-top branch May 10, 2018 23:24
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transpilation generates unreachable code
3 participants