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

Possible latedef regression #2628

Closed
jugglinmike opened this issue Aug 20, 2015 · 3 comments
Closed

Possible latedef regression #2628

jugglinmike opened this issue Aug 20, 2015 · 3 comments
Assignees

Comments

@jugglinmike
Copy link
Member

@lukeapage The following code issues a warning on master (but not v2.8.0):

// jshint latedef: true
var a = function() { return a; };

Although we can't statically differentiate patterns like that from patterns like

// jshint latedef: true
var a = function() { return a; }();

I think we need to be lax in this case (e.g. forgive all references in the right-hand side of binding statements). Patterns like this are actually fairly common (I discovered it while running the master branch against a co-worker's code base):

var myModule = {
  method1: function() { /* etc. */ },
  method2: function() { myModule.method1(); }
};

If included in a minor release, this change in application of latedef would likely cause trouble for our consumers. I think we ought to be conservative in this case: continue to allow references to the identifier being initialized within the right hand side of a binding. For example:

// jshint latedef: true
void a; // warning
var a;
var b;
void b; // accepted
var c = c; // accepted

What do you think?

@lukeapage
Copy link
Member

I agree it is a common pattern that needs a fix.

The easy fix is just to move the binding in the var statement before running through the value. A better fix would be to add it with a flag that it is as yet undeclared for use in this function e.g.

var c = c; // warning
var d = function() { d; }; // accepted

which would make it match let and const behaviour (though in that case its an error in the case c above). I might have time later tonight to look at it, but I may not... maybe better go to the quick fix for this release.

@jugglinmike
Copy link
Member Author

I'm also a bit strapped for time. If/when you are able to get to this, self-assign this issue (I'll do the same).

I know we talked about releasing soon, but I'm happy to wait as long as it takes, so don't feel pressured!

@nicolo-ribaudo
Copy link
Contributor

The regression was introduced in 6d2cc19

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.

3 participants