Skip to content

Commit

Permalink
Fix unused variables sharing names of imports
Browse files Browse the repository at this point in the history
If you have code that looks something like the following:

```js
import uuid from 'uuid';

function bar() {
  return uuid.v4();
}

export default function foo() {
  const things = {
    uuid: bar(),
    henric: 'is cool',
  };

  const { uuid, henric } = things;
  return henric;
}
```

Running fix imports will toggle between adding and removing the `uuid`
import. This is because the error that eslint reports relates to the
`uuid` defined by `const { uuid, henric } = things;` line. However,
after it is removed, we get a second error saying that it is used but
never defined (by `return uuid.v4()`), so it gets added back again.

To fix this, I added some logic that verifies that the eslint error for
unused variables happens within the imports block.

While I was working on the fix_imports method, I found a couple of
opportunities to return early to avoid doing unnecessary work.

Fixes #203
  • Loading branch information
lencioni committed Mar 2, 2016
1 parent 73d6319 commit 9f552ed
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 7 deletions.
35 changes: 32 additions & 3 deletions lib/import_js/importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ def goto
end

REGEX_ESLINT_RESULT = /
:
(?<line>\d+) # <line> line number
:\d+:
\s
(?<quote>["']) # <quote> opening quote
(?<variable_name>[^\1]+) # <variable_name>
\k<quote>
Expand All @@ -73,22 +77,34 @@ def fix_imports
reload_config
eslint_result = run_eslint_command

unused_variables = Set.new
return if eslint_result.empty?

unused_variables = {}
undefined_variables = Set.new

eslint_result.each do |line|
match = REGEX_ESLINT_RESULT.match(line)
next unless match
if match[:type] == 'is defined but never used'
unused_variables.add match[:variable_name]
unused_variables[match[:variable_name]] ||= Set.new
unused_variables[match[:variable_name]].add match[:line].to_i
else
undefined_variables.add match[:variable_name]
end
end

return if unused_variables.empty? && undefined_variables.empty?

old_imports = find_current_imports

# Filter out unused variables that do not appear within the imports block.
unused_variables.select! do |_, line_numbers|
any_line_numbers_within_imports_range?(
line_numbers, old_imports[:range])
end

new_imports = old_imports[:imports].clone
new_imports.delete_variables!(unused_variables.to_a)
new_imports.delete_variables!(unused_variables.keys)

undefined_variables.each do |variable|
js_module = find_one_js_module(variable)
Expand Down Expand Up @@ -130,6 +146,19 @@ def message(str)
@editor.message("ImportJS: #{str}")
end

# @param line_numbers [Set]
# @param imports_range [Range]
# @return [Boolean]
def any_line_numbers_within_imports_range?(line_numbers, imports_range)
line_numbers.each do |line_number|
# Because the range uses line indexes, which are 0-based, and
# line_numbers are 1-based, we need to adjust one of them to make this
# comparison properly.
return true if imports_range.include?(line_number - 1)
end
false
end

ESLINT_STDOUT_ERROR_REGEXES = [
/Parsing error: /,
/Unrecoverable syntax error/,
Expand Down
35 changes: 31 additions & 4 deletions spec/import_js/importer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2151,7 +2151,7 @@ def self.current_selection=(index)
bar
EOS
let(:eslint_result) do
'stdin:1:4: "foo" is defined but never used [Error/no-unused-vars]'
'stdin:2:7: "foo" is defined but never used [Error/no-unused-vars]'
end

it 'removes that import' do
Expand Down Expand Up @@ -2216,7 +2216,7 @@ def self.current_selection=(index)
EOS

let(:eslint_result) do
'stdin:3:11: "bar" is defined but never used ' \
'stdin:1:11: "bar" is defined but never used ' \
"[Error/no-unused-vars]\n" \
'stdin:3:11: "foo" is not defined. [Error/no-undef]'
end
Expand All @@ -2238,7 +2238,7 @@ def self.current_selection=(index)
EOS

let(:eslint_result) do
'stdin:3:11: "foo" is defined but never used ' \
'stdin:1:11: "foo" is defined but never used ' \
"[Error/no-unused-vars]\n" \
end

Expand All @@ -2260,7 +2260,7 @@ def self.current_selection=(index)
EOS

let(:eslint_result) do
'stdin:3:11: "foo" is defined but never used ' \
'stdin:2:11: "foo" is defined but never used ' \
"[Error/no-unused-vars]\n" \
end

Expand All @@ -2272,6 +2272,33 @@ def self.current_selection=(index)
EOS
end
end

context 'when an unused variable that shares its name with an import exists' do
let(:text) { <<-EOS.strip }
import uuid from 'uuid';
function bar() {
return uuid.v4();
}
export default function foo() {
const things = {
uuid: bar(),
henric: 'is cool',
};
const { uuid, henric } = things;
return henric;
}
EOS
let(:eslint_result) do
'stdin:13:11: "uuid" is defined but never used [Error/no-unused-vars]'
end

it 'does not remove the import' do
expect(subject).to eq(text)
end
end
end

describe '#rewrite_imports' do
Expand Down

0 comments on commit 9f552ed

Please sign in to comment.