Variable Hoisting Broken #431

Closed
DashBDev opened this Issue Jan 30, 2012 · 21 comments

Comments

Projects
None yet
4 participants

I have r05, and this seems to break hoisting logic:

var test = function() {
    var fun1 = function (){
        fun2();
    };

    var fun2 = function() {

    };

    fun1();
};

JSHint Returns:
Implied globals:
fun2: 3

Unused variables:
"test": fun2 (1)

Should this work? I'm running jshint through cscript.

Related closed issue: #424

Is there some setting I'm missing? I ran against the exact example mentioned in the changelog and get this:
Implied globals:
innerTwo: 6

Unused variables:
func: innerOne (3), innerTwo (3)

Owner

valueof commented Feb 1, 2012

Oops, I might have forgotten the implied/unused collections when implementing hoisting support—I will check that soon. Thanks!

valueof was assigned Feb 1, 2012

Contributor

domenic commented Feb 12, 2012

Is there any way we can help with this bug? It's a blocker for automated JSHint usage on our codebase.

Owner

valueof commented Feb 12, 2012

Sorry, I'll try to fix it on Monday (traveling all day tomorrow).

valueof closed this in 6326498 Feb 13, 2012

valueof reopened this Feb 13, 2012

Owner

valueof commented Feb 13, 2012

Stupid GitHub closes the issue even know the change is in a branch. Re-opened until reviewed by someone else.

Contributor

WolfgangKluge commented Feb 13, 2012

Cannot close the issue..
Please pretend as if it were closed now ;)

Owner

valueof commented Feb 13, 2012

Looks pretty closed to me. :-) Will make a release later today.

Contributor

domenic commented Feb 13, 2012

Sweet, thank you guys!

Contributor

WolfgangKluge commented Feb 13, 2012

OK, it never looks really reopend to me ;)
Should there be a release for every change now?

Owner

valueof commented Feb 13, 2012

Nope, just for big and/or critical changes/fixes.

Did you retest against my original use cases? I tried the jshint from master and still seem to have the issue.

Oddly enough, r06 works against our production code, but not against my simple test cases.

Contributor

WolfgangKluge commented Feb 14, 2012

The test is somewhat extended - but it's your simple test case (https://github.com/jshint/jshint/blob/63264980c0b2de6bb4f5d86b56919bb66912a339/tests/unit/fixtures/gh431.js).

Which options do you use? Maybe you can create a report on http://jshint.com/ - r06 is already there...

Ok, it works if I use undef:true, fails if I use undef:false. Is that the expected behavior?

Owner

valueof commented Feb 14, 2012

@DashBDev sounds like another bug—will check later today.

Contributor

WolfgangKluge commented Feb 14, 2012

Can you please create a report on http://jshint.com/
I cannot reproduce.

So far, I can't recreate it either on the website. I can only do it when running through wsh with no options.

cscript JSHint\env\wsh.js "test.js"
Contributor

WolfgangKluge commented Feb 14, 2012

OK, now I see what's wrong (I think). Puh (refer to the image above g).

WolfgangKluge reopened this Feb 14, 2012

Owner

valueof commented Feb 14, 2012

Care to share? (I'm at work right now but I'm curious)

Contributor

WolfgangKluge commented Feb 14, 2012

Sorry, yes, sure ;) But I didn't meant I know where the bug is - I can see the impact now ;)

E.g.

function test() {
    test2(); // test2 not found
}
function test2() { }

test2 is not known at the time it's called. So it's added to implied - but in case of undef: false it seems that it's never get out there...

I'm wandering around https://github.com/jshint/jshint/blob/master/jshint.js#L2709

Owner

valueof commented Aug 20, 2012

Seems to work now:

Original example: http://www.jshint.com/reports/660172

valueof closed this Aug 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment