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

constructor renaming parameters #4865

Open
crisward opened this issue Jan 24, 2018 · 25 comments · May be fixed by #5398
Open

constructor renaming parameters #4865

crisward opened this issue Jan 24, 2018 · 25 comments · May be fixed by #5398

Comments

@crisward
Copy link

If you do this

class Blah
  constructor:(@thing)->
     @thing()
      
  method:->
    thing = "hi"

Coffeescript compiles to

var Blah;

Blah = class Blah {
  constructor(thing1) {
    this.thing = thing1;
    this.thing();
  }

  method() {
    var thing;
    return thing = "hi";
  }

};

Why the 1 after the thing?? I'm using an dependency injection system, and the rename is playing havoc with it. Coffeescript 1 didn't do this.

@vendethiel
Copy link
Collaborator

Coffeescript 1 didn't do this.

It's been doing this since 1.9.0, 29jan 2015.

@crisward
Copy link
Author

Is there any documentation or issue as to why coffeescript does this? I can understand why it may be necessary if the variable is declared outside the class, but inside the function doesn't seem to make any sense.

@lydell
Copy link
Collaborator

lydell commented Jan 24, 2018

@crisward Why does it matter to you what a generated variable name looks like? Could you show an example?

@vendethiel
Copy link
Collaborator

8ab15d7

@lydell probably the DI system looks up the name of the parameter to inject. I know AngularJS1 did that.

@crisward
Copy link
Author

crisward commented Jan 24, 2018

We use a dependency injection system which parses the output of the constructor to know what modules to inject (a bit like Angularjs did). This can't find a module with a 1 at the end.

@crisward
Copy link
Author

Just checked and coffeescript 1.9.1 didn't do this.

@lydell
Copy link
Collaborator

lydell commented Jan 24, 2018

Just search for “angular” in this issue tracker and you’ll find everything you need.

@vendethiel
Copy link
Collaborator

➜  coffeescript git:(533ad8a) ./bin/coffee -bce '(@a) -> a'
// Generated by CoffeeScript 1.9.1
(function(a1) {
  this.a = a1;
  return a;
});
➜  coffeescript git:(533ad8a) ./bin/coffee -v
CoffeeScript version 1.9.1

CoffeeScript 1.9.1 did do that.

@vendethiel
Copy link
Collaborator

Also, how does your DI deal with minified code? I know that was an issue in AngularJS and that's why people ultimately stopped doing it (... or developed modules to do it automatically).

@crisward
Copy link
Author

crisward commented Jan 24, 2018

It's node, so doesn't need to be minified. Also you can just not mangle in uglify for client side stuff.

I have a codebase I'm converting from 1.9.1 to 2.* and it's breaking in lots of places. So there is a difference somewhere. I'll take a look around the issues and see if I can get a work around. The other option I have is to avoid using a lib name in my code, just seems a bit unnecessary.

BTW thanks for the rapid response.

@vendethiel
Copy link
Collaborator

Just to be clear: if you write (@a) -> @a, then the compiler doesn't have to reserve a, and names the parameter a. I would recommend against depending on this, but it should solve your outstanding issue.

@lydell
Copy link
Collaborator

lydell commented Jan 24, 2018

@vendethiel That is super fragile though becase if you name a variable a anywhere else in the same file (even if not in the same method) the generated variable will become a1.

@crisward
Copy link
Author

@vendethiel thanks for the help, I looked at some of the other issues. I'm injecting @request, and using the variable name request in a method, which causes @request to become @Request1.

I think the simplest solution as I'm migrating old code is just to add an alias to my injector for generic library names to be added with the additional '1' on the end. Request is the main one as it's a commonly used variable name in node code.

Thanks.

@vendethiel
Copy link
Collaborator

@vendethiel That is super fragile though becase if you name a variable a anywhere else in the same file (even if not in the same method) the generated variable will become a1.

It definitely is. That's why I recommend against it. It's just to help migrate code.

@lydell
Copy link
Collaborator

lydell commented Jan 24, 2018

@crisward It is not necessarily a 1 at the end. It could be larger numbers, too, depending on your code.

A more robust solution is:

class A
  m: (a) ->
    @a = a

But you’ll probably get away with the 1 check, too I guess.

@crisward
Copy link
Author

If I had to do that, I'd probably just give up and use plain js 😢

@connec
Copy link
Collaborator

connec commented Jan 24, 2018

I seem to recall that when this change was made it was reduced to only cover cases where the same name is used within the function (e.g. (@a) -> a)? It looks like there might be a scoping issue, I can't see why constructor's scope should be influenced by method's scope.

@lydell
Copy link
Collaborator

lydell commented Jan 24, 2018

@connec The code simply gets a list of all used identifiers in the whole file and then avoids using those for generated variable names. Why? Because it’s simple. Tracking scope is more complicated for no gain. And even if we did that it wouldn’t help if the name is in the same scope.

@connec
Copy link
Collaborator

connec commented Jan 24, 2018

I see, we only keep track of assignments in Scope, not usage, I guess that makes sense.

@crisward
Copy link
Author

In my example the variable name is not in the same scope, but I understand the need for simplicity.

@phil294
Copy link

phil294 commented Jan 25, 2022

Hi @GeoffreyBooth, maybe this should be reopened: The above comments said narrowing down the scope tracking would be unnecessary. However, @robert-boulanger brought up a very valid use case, explained here: JSDoc.

for example

class ClassTest
  ###*
   * @param {object} option
  ###
  constructor: (@option,@callMe, name)->

option = 1

@option works with option1, so the comment block becomes spaghetti.

@edemaine
Copy link
Contributor

edemaine commented Jan 25, 2022

I'm working on a fix to this (it's relatively simple), but am running into an issue with the current codebase: traverseChildren with a true first argument (crossScope) causes an infinite loop. I can try to track down where the issue is (maybe a loop in the children pointers?), but if anyone knows why this might be happening, let me know.

Update: the infinite loop is caused by tests failing in a bad way; never mind!

@lydell
Copy link
Collaborator

lydell commented Jan 25, 2022

What are you going to do in this case?

option = 1

class ClassTest
  ###*
   * @param {object} option
  ###
  constructor: (@option,@callMe, name)->
    console.log("sum", @option + option)
    
new ClassTest(5, "", "") # Should log `sum 6`

@edemaine
Copy link
Contributor

edemaine commented Jan 25, 2022

@lydell In that case, my code uses option1. You still need to avoid name conflicts with descendants, which is easier to control; conflicts outside this scope are ignored.

edemaine added a commit to edemaine/coffeescript that referenced this issue Jan 25, 2022
Instead of a global `referencedVars` option that is computed at the
lexer level (via all IDENTIFIER tokens), recurse through the AST to mark
each scope with all variables used within it and descendant scopes.

Likely also more efficient via `Set`s instead of `Array`s.

Also fix some missing `children` in `For` nodes, which otherwise caused
this rewrite to fail.

Fixes jashkenas#4865 by causing parameter renaming in fewer (more predictable)
scenarios.
@edemaine
Copy link
Contributor

For those following this issue, #5398 is the code I was describing above, which I believe fixes this issue where possible.

@GeoffreyBooth GeoffreyBooth reopened this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants