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

unused variable or function #515

Closed
knobo opened this issue Mar 5, 2019 · 16 comments
Closed

unused variable or function #515

knobo opened this issue Mar 5, 2019 · 16 comments

Comments

@knobo
Copy link

knobo commented Mar 5, 2019

3 problems here.

  1. only var0 is not used, but there is not warning for that.
  2. Variable ’var1’ referenced but never initialized But it fact it is destructed
  3. Unused variable or function ’var2’ even tough it is used 2 times.

js2-warning

@dgutov
Copy link
Collaborator

dgutov commented Mar 5, 2019

/Cc @lelit

@lelit
Copy link
Contributor

lelit commented Mar 5, 2019 via email

@knobo
Copy link
Author

knobo commented Mar 6, 2019

And on line 82 var4 should have a warning about not beeing initialized.

@lelit
Copy link
Contributor

lelit commented Mar 6, 2019

@dgutov, about var4: trying out the code in NodeJS, it errors out with ReferenceError: var4 is not defined, but that's not catched by js2-highlight-undeclared-vars(). Is that a bug by its own?

NodeJS raises a ReferenceError for both x and z in the following code (executing the functions, I mean):

function foo(a) {
    const x = {a, x};
}

function bar() {
    const y = z;
    const z = 1;
}

Should js2-highlight-undeclared-vars() consider the position of the declaration?

@UwUnyaa
Copy link

UwUnyaa commented Mar 6, 2019

@lelit considering that declaring a const without an initial value throws an error, it should be marked as an error.

While we're at it, it would be nice to take care of temporal dead zone for variables declared with let and const.

@dgutov
Copy link
Collaborator

dgutov commented Mar 6, 2019

@lelit, I think so. Unless there's evidence that NodeJS behaves against the spec, which is unlikely.

lelit added a commit to lelit/js2-mode that referenced this issue Mar 7, 2019
Destructured function parameters are not properly recognized.
lelit added a commit to lelit/js2-mode that referenced this issue Mar 7, 2019
@lelit
Copy link
Contributor

lelit commented Mar 7, 2019

The above changes seems to cure one half of the reported problem.

I wasn't so lucky trying to solve the other half, the undefined reference in the const assignment. First I checked with both Firefox and Chrome, and they raise an error executing the simple functions above. Then I tried to tweak js2-get-defining-scope to handle also js2-CONST assignment, something like:

diff --git a/js2-mode.el b/js2-mode.el
index 567d629..0c2e881 100644
--- a/js2-mode.el
+++ b/js2-mode.el
@@ -2314,7 +2314,7 @@ Returns nil if there is no enclosing scope node."
 Returns `js2-scope' in which NAME is defined, or nil if not found.
 
 If POINT is non-nil, and if the found declaration type is
-`js2-LET', also check that the declaration node is before POINT."
+`js2-LET' or `js2-CONST', also check that the declaration node is before POINT."
   (let ((sym (if (symbolp name)
                  name
                (intern name)))
@@ -2325,7 +2325,7 @@ If POINT is non-nil, and if the found declaration type is
            (let ((entry (cdr (assq sym (js2-scope-symbol-table scope)))))
              (and entry
                   (or (not point)
-                      (not (eq js2-LET (js2-symbol-decl-type entry)))
+                      (not (memq (js2-symbol-decl-type entry) (list js2-LET js2-CONST)))
                       (>= point
                           (js2-node-abs-pos (js2-symbol-ast-node entry))))))
            (and (eq sym 'arguments)

but that has not the expected outcome... Will try harder, but maybe you can point me in the right direction :-)

@lelit
Copy link
Contributor

lelit commented Mar 7, 2019

Please let me know if you prefer splitting that into a separate issue, opening a PR with these changes alone, or if instead it would be better to wait for a fix to the const thingie.

@knobo
Copy link
Author

knobo commented Mar 7, 2019

I'll open a seperate issue for the missing var4 warning/error.

@lelit
Copy link
Contributor

lelit commented Mar 7, 2019

If you can test my branch and tell if it effectively cures your case, it would be great!

@knobo
Copy link
Author

knobo commented Mar 7, 2019

Yes, I have tested it a bit now. And it looks like it works.
But how do I override elpa dependencies to load your git version instead of elpa version?

@lelit
Copy link
Contributor

lelit commented Mar 7, 2019

Sorry, can't help there. But usually Dmitry is very responsive in releasing bug-fixes :-)

@knobo
Copy link
Author

knobo commented Mar 7, 2019

I consider myself done testing now :)

@lelit lelit mentioned this issue Mar 7, 2019
@lelit
Copy link
Contributor

lelit commented Mar 7, 2019

Thanks a lot!

@knobo
Copy link
Author

knobo commented Mar 7, 2019

Thank you for fixing the bugs :)

@dgutov
Copy link
Collaborator

dgutov commented Mar 7, 2019

Merged, thanks for the report.

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

4 participants