Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Downlevel for 'let' doesn't account for nested declarations #2449

Closed
JsonFreeman opened this issue Mar 21, 2015 · 11 comments · Fixed by #2471
Closed

Downlevel for 'let' doesn't account for nested declarations #2449

JsonFreeman opened this issue Mar 21, 2015 · 11 comments · Fixed by #2471
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@JsonFreeman
Copy link
Contributor

var x;
function foo() {
    let x = 0;
    (function () {
        var _x = 1;
        console.log(x); // Prints 1, should print 0
    })();
}

The rename of the 'let' does not account for the _x declared in the nested function. Consequently, it chooses _x as the rename, but now the inner _x shadows it.

@JsonFreeman JsonFreeman added the Bug A bug in TypeScript label Mar 21, 2015
@vladima vladima self-assigned this Mar 21, 2015
@JsonFreeman
Copy link
Contributor Author

Maybe it is not so bad to just rename all value declarations during emit. We can keep the scope tracking, and just use that for aliases as well, and then we could get rid of getGeneratedNamesForSourceFile. This is nice because it avoids a pre-pass, and it results in nicer names. And you wouldn't have to descend to find references in nested containers.

The key here would be that only let and const have to check resolveName. Vars, functions and parameters only have to make sure they are not colliding with a generated name in any of the ancestor scopes, but they can certainly shadow source names from ancestor scopes.

@vladima
Copy link
Contributor

vladima commented Mar 21, 2015

I'm not a big fan of idea of cascading renames especially for something that user might not expect to be renamed (i.e. presence of let higher in the scope triggers renaming of the function). An alternative proposal that I have is to move name generation logic entirely to emitter and generate names on demand. Names can be generated using two ways:

  • makeUniqueName - creates a name that does not exist in global scope and unique to current file. This name will never be generated in the future nor by makeUniqueName neither by makeTempVariable
  • makeTempVariable - creates a name that does not exists anywhere higher in the scope (in user code) and unique among generated names in current scope. The same name however can be generated by calls to makeTempVariable above or below in the scope and it is up to the user to protect yourself from this fact.

I'll send the pull request with this proposal shortly

@vladima
Copy link
Contributor

vladima commented Mar 22, 2015

Proposed fix #2455

@CyrusNajmabadi
Copy link
Contributor

@vladima "I'm not a big fan of idea of cascading renames especially for something that user might not expect to be renamed (i.e. presence of let higher in the scope triggers renaming of the function)."

Hey Vlad, i'm not sure the user has any expectation that anything might be renamed. Renaming is an internal implementation decision we've taken in order to work around the collision problem inherent in downlevel emitting of language constructs. In that regard, i think we have nearly full control over what we do or do not rename. About the only things we can't rename are things that are part of your public surface area shape. i.e. if we export anything, then that entity to keep the same name. However, for all the code that isn't part of your public API shape, we have full control over what we emit, and we can decide what should/shouldn't get renamed.

The approach Jason has listed seems sensible to me. Right now we're trying to come up with an algorithm where we don't pick a name because something lower in the code might collide with it. But i see no reason why that code that that we hit later is any more important than the code we're hitting now. We're dealing with the variable at this point, we should give it the best name we can, given what we know so far. Then, when we hit code later on, we may have to rename those entities appropriately.

This has several nice properties:

  1. It seems simple. And i do appreciate having a simple algorithm for the compiler.
  2. In the absence of collisions, entities keep the name the user wrote. That's very nice given the current limitations of debuggers today.
  3. It seems cheaper than the approach where we have to aggressively walk down the tree.

The only 'downside' (which i don't necessarily agree is a downside) is that, in the presence of collisions, the entities we hit later are renamed, not necessarily the 'let'. But i see no reason why that's at all undesirable behavior, or that users will dislike these types of renames any more than our current approach of renames.

@vladima
Copy link
Contributor

vladima commented Mar 22, 2015

User might have certain expectations about what should not be changed in their code, no other transpilers change names of value declarations besides let\const and this effect can be very surprising for the user. This was my original concern when I've suggested this approach on my discussion with Jason on Friday

@vladima
Copy link
Contributor

vladima commented Mar 22, 2015

@JsonFreeman maybe I'm not reading it right but I cannot agree with it

The key here would be that only let and const have to check resolveName. Vars, functions and parameters only have to make sure they are not colliding with a generated name in any of the ancestor scopes, but they can certainly shadow source names from ancestor scopes.

consider this case:

declare function use(a: any);
var x;
var _x_1;
{
    let x; // _x;
    function foo() {
        var _x; // (3) should be renamed but skipping next generated name '_x_1'
        use(x);
        use(_x);
        use(_x_1);
    }
}

var _x // (3) will conflict with generated name but in order to generate proper new name you need to consult resolveName otherwise generated name can shadow exising name from outer scope

@JsonFreeman
Copy link
Contributor Author

But the cases where a var or a function would be changed are exceedingly rare! They would have to name the function something like _name or _name_0 or something like that. I admit the former case is somewhat common, but in general I don't think that many function scope constructs will get renamed.

@JsonFreeman
Copy link
Contributor Author

Oh I see. Looking at your example, you are right. But still, you would only need to check resolveName to determine if one of the unique name candidates collides with an outer scope. You would be able to skip this step when checking uniqueness of the original name though. You'd only need to do it for names that you are trying that you have invented.

Also, when determining uniqueness, a block scoped declaration can be treated just like a function scoped declaration if it is declared in a function scope.

@JsonFreeman
Copy link
Contributor Author

I'm bringing these up because they are ways that we could reduce the number of places where a rename actually happens, and it will lead to a better experience. I think renaming declarations that are not traditionally renamed by transpilers is still better than renaming almost every 'let' in the program and giving them all different names.

@CyrusNajmabadi
Copy link
Contributor

@vladima "User might have certain expectations about what should not be changed in their code, no other transpilers change names of value declarations besides let\const and this effect can be very surprising for the user. This was my original concern when I've suggested this approach on my discussion with Jason on Friday"

That's definitely a reasonable concern to have.

However, given the choice between:

  1. a compiler that will literally rewrite all lets/consts
  2. a compiler that rewrites some other entity types only in the presence of collisions

I think teh former would be far more surprising. I would be surprised if users even ran into the second situation that often. And, even in the case where they shadowed a 'let' with a 'var', i'm not really buying that a user would be that surprised that their 'var' got renamed.

(The case with a function being renamed seems so exceedingly rare, that i don't think it's even valuable to discount this approach just because of the concern over it).

Also, i think i'd be ok with users being slightly surprised (which i'm still skeptical about), but having a much better debugging experience with sourcemaps, versus a user not being surprised, but discovering that they pretty much could not debug any 'let's with sourcemaps.

At the end of the day, if we want people to use let+downlevel then we have to make sure that debugging is ok most of the time. Right now, with the PR offered, debugging will be bad nearly all of the time. That seems like a super bad place to go to and strongly negates the value prop offered by TS to use ES6 features with downlevel emit.

Thanks!

@JsonFreeman
Copy link
Contributor Author

Yes, I agree with @CyrusNajmabadi in that we have to look at this in terms of what will happen most of the time. I feel that renaming a var or a function will be such a corner case, that I don't really care about it. Whereas debugging with let downlevel may become exceedingly common.

@vladima vladima added this to the TypeScript 1.5 milestone Mar 23, 2015
vladima added a commit that referenced this issue Mar 24, 2015
@vladima vladima added the Fixed A PR has been merged for this issue label Mar 24, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants