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

no-unused-vars #33

Closed
g-plane opened this issue Feb 13, 2019 · 48 comments · Fixed by #41
Closed

no-unused-vars #33

g-plane opened this issue Feb 13, 2019 · 48 comments · Fixed by #41

Comments

@g-plane
Copy link
Collaborator

g-plane commented Feb 13, 2019

文档:https://eslint.org/docs/rules/no-unused-vars

仅仅移除 ES6 的 import

@aladdin-add
Copy link
Owner

仅仅移除 ES6 的 import。

为什么呢?我觉得未使用的局部变量和函数也可以移除啊

@aladdin-add
Copy link
Owner

这里有一个问题是和编辑器集成:用户开发过程中,不希望这些被移除。但是这种情况下,用户自己可以选择不要开启这个fix

@g-plane
Copy link
Collaborator Author

g-plane commented Feb 13, 2019

我考虑到的是,有时候变量只是「临时性」的未被使用(也就是还没写将会使用这个变量的代码),如果此时被移除,让用户再补上就比较烦。

而移除 import 则是借鉴了 Go 语言下的一个工具(goimports)。在 Go 里,未被使用的变量会导致编译错误,但目前好像没有工具会把变量声明语句自动移除,所以我是由此参考、借鉴而来。

@aladdin-add
Copy link
Owner

嗯,所以这条规则的autofix不推荐在开发过程中使用,类似于 no-debugger.

我觉得可以考虑都移除掉,而且这个fix是安全的,不会影响执行结果。

@g-plane
Copy link
Collaborator Author

g-plane commented Feb 13, 2019

函数参数应该不能移除,否则会改变函数签名(参数个数发生了变化,进而影响调用者)。

@g-plane
Copy link
Collaborator Author

g-plane commented Feb 13, 2019

我们应该考虑用户什么时候会开启 --fix 选项,什么时候不开启。

@aladdin-add
Copy link
Owner

aladdin-add commented Feb 13, 2019

用户选择不开启这个fix,可以不打开这个规则啊,使用内置的就可以。

通过和 eslint-plugin-no-autofix的配合, 用户可以精确控制fix的行为:

{
"rules": {
  "autofix/rule1": "error",
  ....
   "no-autofix/rule2": "error"
   ...
}
}

@merlinstardust
Copy link

Has this been implemented? I would like to remove unused vars, imports, etc automatically

@g-plane
Copy link
Collaborator Author

g-plane commented May 30, 2019

Currently no, just a plan.

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

Working for removing imports now.

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

@aladdin-add 对于这种情况您怎么考虑?

let a = b()

@aladdin-add
Copy link
Owner

可以考虑移除a的定义(没有被使用的话)?

let a = b();
// =>
b();

并不一定在一个pr里面实现,最多的场景可能就是 import/require -- 可以先实现这个。

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

这样可能不好处理:

let a = b(), c = d()

遇到多个定义的就不修复?

@aladdin-add
Copy link
Owner

aladdin-add commented Jun 7, 2019

理论上可以fix,不过可以后面再实现。thoughts?

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

现在实现也没问题的,应该不会太复杂,就是针对上面那种情况不知道思路。

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

比如就刚才那个,有什么好的方案吗?

let a = b(), c = d()

@aladdin-add
Copy link
Owner

b();
d();

有什么问题么?

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

假定 c 是已被 used 的呢?

@aladdin-add
Copy link
Owner

b();
let c = d();

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

感觉这对于某些代码来说,修复后的结果可能有点怪:(但符合语法)

// before
let a = "";

// after
"";

@aladdin-add
Copy link
Owner

haha, interesting!

let s = "use strict";
=>
"use strict";

如果 "=" 后面是字面量的话,可以把整个语句移除掉。也就是说:
let a = b:

  • 当b包含函数调用时, fixed to b;
  • 当b不包含函数调用时, fixed to ;

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

也许可以考虑直接删除没有副作用的表达式:

// before
let a = 123

// after
// (no code)

// before
let b = c

// after
// (no code)

如何?

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

@aladdin-add
Copy link
Owner

我重新想了下,还是允许这种代码:

// before
let a = "";

// after
"";

如果启用 no-unused-expression 这个规则的话,应该可以被移除。

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

no-unused-expression 在下面这种情况也会报告:

[a()];

@aladdin-add
Copy link
Owner

有一个例外就是, let s = "use strict;" => "use strict;"会影响到执行结果. 但是极少有用户会这样写,也可以接受。

@aladdin-add
Copy link
Owner

no-unused-expression 在下面这种情况也会报告:

[a()];

so?

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

所以,如果开启 no-unused-expression,就会:

// before
let array = [a()]

// after
// (no code)

@aladdin-add
Copy link
Owner

首先,这里有2个问题:

  1. no-unused-var: let array = [a()]; => [a()]; (这个issue的内容。)
  2. no-unused-expression: [a()]; => ; (我不太清楚实际结果,如果真的是这样的话,好像是另外一条规则的bug。

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

对于第一点,我觉得意义不大,因为 auto fixing 本来就是方便用户,减少用户手工修改代码,但仅仅把表达式提取出来,最终还是要让用户去处理。

@corysimmons
Copy link

Following 😰

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 7, 2019

那最后的方案是怎样?

@aladdin-add
Copy link
Owner

aladdin-add commented Jun 7, 2019

最多的场景可能就是 import/require -- 可以先实现这个。我建议的优先级是这样:

  1. 移除 unused import/require. 预计可以解决50%+的case

  2. 移除 var s = "xxx"; 这样的,预计可以解决 30%+ case

  3. 移除 var s = ....; 不太好判断的,个人对这种实现没有很大的兴趣 -- 收益不大,要考虑的边界情况又很多。但是有贡献者感兴趣的话,也会接受。 😸

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 8, 2019

@merlinpatt @corysimmons That PR was merged. Please wait for a new release.

@corysimmons
Copy link

Will this convert...

import {Foo, Bar} from 'foobar'

const x = Foo

...to...

import {Foo} from 'foobar'

const x = Foo

?

@aladdin-add
Copy link
Owner

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 8, 2019

Non-related: it seems that our releases don't follow Semver.

@corysimmons
Copy link

If it's pre-1.0.0, then you don't have to follow semver.

That said... a lot of projects keep their releases pre-1.0.0 just because they're lazy. 😇

If you guys like the current API then you should go ahead and bump to 1.0.0 and start following semver.

@corysimmons
Copy link

corysimmons commented Jun 8, 2019

This is great! Thank you for your work @g-plane and @aladdin-add !

Combined with https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#newlines-between-ignorealwaysalways-and-inside-groupsnever this does everything I want!

This config replaces https://twitter.com/john_papa/status/991111269674569733 and turns it into a CLI you can run like $ yarn lint --fix

{
  "scripts": {
    "lint": "eslint '{,!(node_modules)/**/}*.js'"
  },
  "eslintConfig": {
    "plugins": [
      "autofix"
    ],
    "rules": {
      "autofix/no-unused-vars": "error",
      "import/order": [
        "error",
        {
          "groups": [
            [
              "builtin",
              "external"
            ],
            [
              "parent",
              "sibling",
              "index"
            ]
          ],
          "newlines-between": "always"
        }
      ]
    }
  }
}

^ 😍😍😍😍😍😍

Thank you!

--->>> https://gist.github.com/corysimmons/d6ee1c9c4a5e557980e4da20ce01e1d3

@aladdin-add
Copy link
Owner

nice! the credit goes to @g-plane 💯

@corysimmons
Copy link

corysimmons commented Jun 8, 2019

Oh, sorry @g-plane

I can't read your language so I have no idea who did what, but thanks to whoever did this! <3 <3

@serhiipalash
Copy link

serhiipalash commented Jun 8, 2019

Great work! Thank you @g-plane and @aladdin-add !

One issue I noticed is that autofix/no-unused-vars doesn't work well with object destructuring.

before autofix

const { name, email } = this.props // "name" is unused

after autofix

const { , email } = this.props

coma is left

With unused imports everything works great.

@serhiipalash
Copy link

const { , email } = this.props

This will crash the app

@corysimmons
Copy link

@serhiipalash Good catch!

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 8, 2019

#44 .

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 9, 2019

@serhiipalash @corysimmons This has been fixed at #45 .

@adam1985
Copy link

Auto fixing produces wrong syntax

before autofix

const connect = (data, { name }) => ({ data, });>  // "name" is unused

after autofix

const connect = (data, { }) => ({ data, });

@g-plane
Copy link
Collaborator Author

g-plane commented Jun 25, 2019

@adam1985 This is valid syntax.

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

Successfully merging a pull request may close this issue.

6 participants