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 parameters with @ sometime not recognized #5

Closed
robert-boulanger opened this issue Jan 25, 2022 · 6 comments
Closed

constructor parameters with @ sometime not recognized #5

robert-boulanger opened this issue Jan 25, 2022 · 6 comments

Comments

@robert-boulanger
Copy link

When prefixing constructor arguments with @

like

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

Coffeesense complains:

JSDoc '@param' tag has name 'option', but there is no parameter with that name.

The position of this error could not be mapped back to CoffeeScript, sorry. Here's the failing JavaScript context:

    /**
   * @constructor
   * @param {object} option
   * @param {Function} callMe
   * @param {string} name 

JSDoc '@param' tag has name 'callMe', but there is no parameter with that name.

The position of this error could not be mapped back to CoffeeScript, sorry. Here's the failing JavaScript context:

    /**
   * @constructor
   * @param {object} option
   * @param {Function} callMe
   * @param {string} name 

However changing the param name from option to options (in code and jsdoc) let the message disappear for the moment, but it pops up later again complaining about the new names

@phil294
Copy link
Owner

phil294 commented Jan 25, 2022

The example code you gave me gives no error. But I think I found your specific problem anyway: You most likely have another variable in the same scope, also called option (have you? if not, could you share the code?). Perhaps outside the class:

export class ClassTest
  constructor: (@option)->

option = 123

The official CoffeeScript compiler translates this into

var option;

export var ClassTest = class ClassTest {
  constructor(option1) {
    this.option = option1;
  }

};

option = 123;

Notice the option1. Apparently the compiler checks for the existence of a variable of the same name when creating the local constructor variable. In the example above, this does not make sense, but it does when option lives inside the constructor:

export class ClassTest
  constructor: (@option)->
  option = 123

becomes

export var ClassTest = (function() {
  var option;

  class ClassTest {
    constructor(option1) {
      this.option = option1;
    }

  };

  option = 123;

  return ClassTest;

}).call(this);

In this case, if it weren't option1 but option, it would overshadow the global option and thus making this.option = option1 useless.

So, to solve this issue, the CS compiler needs to look for usage the variable in the constructor itself, not the surrounding code. Until now, this was not a problem because who cares what the invisible variable is called. But with JSDoc, it is... I'll create a issue in a sec

It's a bit of an edge case, really. Besides removing the conflicting var from the same scope, there's at least two ways to workaround it:

class ClassTest
  ###*
   * @param {object} option
  ###
  constructor: (option)->
    @option = option
class ClassTest
  constructor: (###* @type {object} option ###)->

In future, with Coffee-Type-Script (there's no name yet), the best way will be

class ClassTest
  constructor: (@option ~ object) ->

but only if you will want to use TS anyway, so a solution to the JSDoc problem is desirable.

@phil294
Copy link
Owner

phil294 commented Jan 25, 2022

You could also change your * @param {object} option to * @param {object} option1 to fix the error, but this is a very bad idea because it will then surely break in the future if you edit the code again

Also, this The position of this error could not be mapped back to CoffeeScript, sorry. Here's the failing JavaScript context: is a bit stupid, it should at least try to go near the actual position, not the start of the file. I'll fix that soon

@robert-boulanger
Copy link
Author

You are right, there is an option outside the scope of the class, but not as you assumed :

the complete sample.coffee to reconstruct the problem:

outSideMethod = (s) ->
  option = s

export class ClassTest
  ###*
   * @constructor
   * @param {object} option
   * @param {Function} callMe
   * @param {string} name
  ###
  constructor: (@option,@callMe, name)->
    a = 2

and here the output of the compiler:

var outSideMethod;

outSideMethod = function(s) {
  var option;
  return option = s;
};

export var ClassTest = class ClassTest {
  /**
   * @constructor
   * @param {object} option
   * @param {Function} callMe
   * @param {string} name
   */
  constructor(option1, callMe, name) {
    var a;
    this.option = option1;
    this.callMe = callMe;
    a = 2;
  }

};

and here the output of coffeesense



JSDoc '@param' tag has name 'option', but there is no parameter with that name.

The position of this error could not be mapped back to CoffeeScript, sorry. Here's the failing JavaScript context:

  /**
   * @constructor
   * @param {object} option
   * @param {Function} callMe
   * @param {string} name

@phil294
Copy link
Owner

phil294 commented Jan 25, 2022

yeah that's even worse. there is an issue about this already: jashkenas/coffeescript#4865

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.

@robert-boulanger
Copy link
Author

Maybe it would be a good idea to collect all those things and add it to the documentation of coffeesense ?

phil294 added a commit that referenced this issue Jan 27, 2022
until now, these were shown at the *start* of the file with the note `The position of this error could not be mapped back to CoffeeScript, sorry. Here's the failing JavaScript context: ...`
I now moved the location to the next available follow up code line where there is any source map info available which is much more helpful

ref #5
@phil294
Copy link
Owner

phil294 commented Aug 6, 2023

I had added it to the readme last year.

other than that, as there's nothing to do from an extension standpoint, I'll close this

@phil294 phil294 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants