I think LexicalEnvironmentKind would be more consistent with TypeScript's internal naming conventions.

Previously this was just a variable declaration environment, and not a block scope. I'm concerned that introducing let declarations here will be a problem for down-level transformations to ES5 as they won't have been visited by the type checker and won't have the necessary NodeCheckFlags to check for shadowing and per-iteration environments when transforming block-scoped variables down-level. This becomes even more complicated if this is part of the public API for TypeScript, as those things won't "just work" if someone builds a custom transformer.

Thanks for pointing this out - latest changes should rely on the checker flag being set to determine if it should be a block-hoisted variable - so there's no API to say "hoist this variable into the block scope."

I'm not sure this should be part of the public API. It presents the possibility of a custom transformer introducing a let-scoped variable we won't be able to properly down-level since it wasn't visited by the type checker.

The new code changes don't expose a public API for starting a block scope and rely on the checker flags being set to determine which scope it should hoist a variable into.

We're doing something similar already in checkNestedBlockScopedBinding. Is there overlap here that could be refactored into a separate method?

Refactored this into getEnclosingIterationStatement

This is something we would normally handle in checkNestedBlockScopedBinding, except it seems that function exits early if the target is es2015 or later. Perhaps we should instead add a parameter to checkNestedBlockScopedBinding to force it to execute when this condition is reached?

If we are going to add these to TransformationContext, we should enforce them by default in visitEachChild (in visitor.ts), similar to how we enforce calls to startLexicalEnvironment and endLexicalEnvironment.

For example: in visitEachChild, we have the following switch case for a MethodDeclaration:

            case SyntaxKind.MethodDeclaration:
                return updateMethod(<MethodDeclaration>node,
                    ...
                    visitParameterList((<MethodDeclaration>node).parameters, visitor, context, nodesVisitor),
                    visitNode((<MethodDeclaration>node).type, visitor, isTypeNode),
                    visitFunctionBody((<MethodDeclaration>node).body!, visitor, context));

If you look at visitParameterList, it contains the following at the top of the function:

    export function visitParameterList(nodes: NodeArray<ParameterDeclaration> | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor = visitNodes) {
        context.startLexicalEnvironment();
        ...

Conversely, visitFunctionBody contains the following:

    export function visitFunctionBody(node: ConciseBody | undefined, visitor: Visitor, context: TransformationContext): ConciseBody | undefined {
        ...
        const declarations = context.endLexicalEnvironment();
        if (some(declarations)) {
            const block = convertToFunctionBody(updated);
            const statements = mergeLexicalEnvironment(block.statements, declarations);
            return updateBlock(block, statements);
        }
        return updated;
    }

We should so something similar for any Block.

I would prefer if we had a separate function (i.e. addBlockVariableDeclaration) to add block-scoped variables rather than having hoistVariableDeclaration perform both. I'd rather avoid having to use the resolver for every temp variable we generate, and this would allow us to record net-new block-scoped temporary variables in other transformations.

NOTE: I suggest addBlockVariableDeclaration rather than hoistBlockVariableDeclaration above as "hoist" has a specific meaning that does not apply to block-scoped variables.

Also, for block-scoped variables we should probably always enforce GeneratedIdentifierFlags.ReserveInNestedScopes regardless of what was passed to createTempVariable.

How would you introduce a net-new block scoped variable correctly without the checker putting the appropriate flags on it? For example, the ES5 transformation relies on the checker flags to correctly determine whether there is a block scoped variable in a particular loop.

My primary concern is performance. I'd rather avoid lookups against the resolver for every generated identifier, if possible. The only case where this check seems like it is needed is here, so incurring the cost of the lookup for every hoisted variable is unnecessary overhead.

I would still recommend that we add addBlockVariableDeclaration for this case, but we can mark it as /* @internal */ for now and revisit the API for custom transformer consumption later.

If you make the changes to visitEachChild I mentioned above, this function would not be necessary.

If you make the changes to visitEachChild I mentioned above, you shouldn't need this closure as you can pass addBlockVariableDeclaration (or whatever we end up calling it) here and set the original node pointer after the fact.