Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Fix var binding to scope #581

Closed
wants to merge 1 commit into from
Closed

Conversation

yassere
Copy link
Contributor

@yassere yassere commented Jun 5, 2020

The recent scope-tracking improvements changed the kind of scope that var bindings are attached to. This fixes the JSVariableDeclaration evaluator so that it can add var bindings to the scope.

It also changes JSVariableDeclarationStatement so that it doesn't attempt to add var bindings, because that should be handled by addVarBindings on the relevant block. Without this change, var declarations inside a nested block scope would cause an error to be thrown.

Closes #574
Closes #535

@@ -44,7 +44,7 @@ export default createScopeEvaluator({

if (
node.kind === "var" &&
(scope.kind === "program" || scope.kind === "function")
(scope.kind === "program" || scope.kind === "block")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct because:

function foo() {
  {
    var bar;
  }
  bar;
}

The bar binding should be set on the function but I believe with this change it would be on the inner block.

Copy link
Contributor Author

@yassere yassere Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the bar var binding in your example is on the block scope that's the direct child of the function declaration, which is what I thought your intention was with the recent changes. It looks like addVarBindings in the JSBlockStatement evaluator is trying to add var bindings to the "block" scope, but nothing gets added because of this conditional. Sorry if I misunderstood.

If I use this branch to lint the sample:

function foo() {
  {
    var bar;
    let baz;
  }
  bar;
  let rome;
}

with the rule:

if (node.type === "JSBlockStatement") {
  console.log(scope.bindings);
}

I see that the rome and bar bindings are in the same scope and the baz binding is in a different scope by itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. I can't really make a lot of sense of the existing code (that I wrote) so I took a look and rethought the whole var injection strategy. I'll have a PR in a minute.

@yassere
Copy link
Contributor Author

yassere commented Jun 7, 2020

Resolved by #589

@yassere yassere closed this Jun 7, 2020
@yassere yassere deleted the fix/var-binding branch June 7, 2020 23:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Var bindings not being attached to scope Lint rule noFunctionAssign has false positive
2 participants