[RemoveUnusedImports] Support string type annotations #353

zsol · 2020-07-28T16:14:57Z

Summary

This PR adds support for detecting imports being used by string type
annotations, as well as imports suppressed by comments.

It breaks up the existing visitor into multiple smaller, single-purpose
visitors, and composes them together.

TODO:

add docstrings
document behavioral changes to RemoveImportsVisitor

Test Plan

existing and new tests

This PR adds support for detecting imports being used by string type annotations, as well as imports suppressed by comments. It breaks up the existing visitor into multiple smaller, single-purpose visitors, and composes them together.

Kronuz · 2020-07-28T20:07:28Z

libcst/codemod/visitors/_gather_unused_imports.py

+
+
+DEFAULT_SUPPRESS_COMMENT_REGEX = (
+    r".*\W(lint-ignore: ?unused-import|noqa|lint-ignore: ?F401)(\W.*)?$"


I suggest this one instead:

r".*\W(noqa[^:]|noqa: ?F401|lint-ignore: ?unused-import|lint-ignore: ?F401)(\W.*)?$"

So it ignores things like # noqa, # noqa: F401, but not # noqa: IG61

Kronuz · 2020-07-28T20:13:27Z

libcst/codemod/visitors/_gather_unused_imports.py

+            self.context.scratch.get(
+                self.SUPPRESS_COMMENT_REGEX_CONTEXT_KEY, DEFAULT_SUPPRESS_COMMENT_REGEX,
+            ),


The thing with suppressing comments at GatherUnusedImportsVisitor level is that in some use cases (linter rules in frameworks that already silent lines with such comments) this ends up in doing more work than needed.

Maybe the suppression of comments could be moved to some other visitor which adds lines to be excluded, so one could choose whether to have them collected or not by using that one.

I suppose you can set the regex to something that will never match (like ^$), but we could add an explicit bool option to disable it. Would that help?

Well, faster code is code never executed. Best thing would be if we could put these in a whole different, also reusable, place so they're never executed when they're not needed. Something like GatherIgnoredLines or something like that. But that's only a suggestion for those use cases where this isn't really needed.

libcst/codemod/visitors/_gather_string_annotation_names.py

- move lint suppression out from visitor into codemod - change suppression regex

jimmylai · 2020-07-29T16:15:12Z

libcst/codemod/commands/remove_unused_imports.py

    )

+    METADATA_DEPENDENCIES = (
+        PositionProvider,
+        *GatherCommentsVisitor.METADATA_DEPENDENCIES,


We should only need to list the direct metadata dependency of RemoveUnusedImportsCommand here.

@jimmylai, so the line *GatherCommentsVisitor.METADATA_DEPENDENCIES, should be removed, right?

jimmylai · 2020-07-29T16:28:05Z

Overall looks good and the test coverage is high. I'll let @Kronuz to do another review and accept to see if he need more changes for reusability.

Kronuz · 2020-07-29T19:12:25Z

libcst/codemod/commands/remove_unused_imports.py

+DEFAULT_SUPPRESS_COMMENT_REGEX = (
+    r".*\W(noqa|lint-ignore: ?unused-import|lint-ignore: ?F401)(\W.*)?$"
+)


Also, don't forget about this one as well, I think it's better than the current one:

DEFAULT_SUPPRESS_COMMENT_REGEX = ( r".*\W(noqa[^:]|noqa: ?F401|lint-ignore: ?unused-import|lint-ignore: ?F401)(\W.*)?$" )

This doesn't match # noqa though, which is the most common case

You are right!

What do you think of:

r".*\W(noqa|lint-ignore)(: ?(F401|unused-import)|(?= )|$)(\W.*)?$"

Kronuz · 2020-07-29T19:15:33Z

libcst/codemod/commands/remove_unused_imports.py

    )

+    METADATA_DEPENDENCIES = (
+        PositionProvider,
+        *GatherCommentsVisitor.METADATA_DEPENDENCIES,


@jimmylai, so the line *GatherCommentsVisitor.METADATA_DEPENDENCIES, should be removed, right?

Kronuz · 2020-07-29T19:23:39Z

libcst/codemod/visitors/_gather_unused_imports.py

+
+    def visit_Module(self, node: cst.Module) -> bool:
+        export_collector = GatherExportsVisitor(self.context)
+        node.visit(export_collector)


@jimmylai, just wondering... won't calling node.visit(export_collector) and node.visit(annotation_visitor) here traverses the tree multiple times? wouldn't this be inefficient?

I was thinking maybe using multiple inheritance instead of this, so we make it that it only traverses the tree once.

Yes it will traverse the tree multiple times. I don't think that's a big deal for a codemod like this. Running the codemod on a bunch of large files is still not visibly slower than before. I don't have benchmarks for you though :)

If we use this in a linter, which will run over and over, while the user types, we want this as fast as possible, otherwise linting times will hit the experience. Although I’m not sure how expensive traversing the tree is, processing time would be exponential, potentially noticeable on big files. What do you think @jimmylai ?

Performance is a concern for lint but not for codemod. In lint, we try to avoid creating a visitor and calling node.visit(visitor).

The @m.visitused in this codemod is not supported in the lint framework since we worried about the the pattern can introduce some expensive checks. So we only use m.matches() in lint.

Kronuz · 2020-07-29T19:25:30Z

libcst/codemod/visitors/_gather_unused_imports.py

+        return True
+
+    @m.visit(
+        m.Import()


How could we add other exceptions to this reusable class, like if we wanted to ignore import __strict__, for example?

I think it's best to apply filtering on the results of this visitor (just like how you pointed out that the suppression filtering belongs outside of this class). So I would filter for special module names after running GatherUnusedImportsVisitor, and keep the exceptions here to a minimum (i.e. the ones implied by Python itself)

Yes, I agree :)

jimmylai

To make GatherCommentsVisitor, GatherNamesFromStringAnnotationsVisitor and GatherUnusedImportsVisitor reusable in lint framework and address the concerns on performance.
My proposal is to not use @m.visit and @m.call_if_inside in the implementation (just like GatherImportsVisitor and GatherExportsVisitor). Then we can configure to run those helper visitors in lint framework before running lint rules. So the computed data will be accessible in each lint rule. What do you think? @Kronuz @zsol

zsol · 2020-07-30T17:50:41Z

I'm ok with that but won't have time to implement it this week. How about we land this PR and open a new one for these proposed refactors?

Kronuz · 2020-07-30T22:21:02Z

@jimmylai, sounds good!

@zsol, thank you for working on this! It'll be super helpful!! 😀

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 28, 2020

Kronuz reviewed Jul 28, 2020

View reviewed changes

libcst/codemod/visitors/_gather_string_annotation_names.py Outdated Show resolved Hide resolved

zsol added 3 commits July 29, 2020 12:18

Address review feedback:

520abd0

- move lint suppression out from visitor into codemod - change suppression regex

lint

f436658

fix type errors

a763d7c

jimmylai reviewed Jul 29, 2020

View reviewed changes

Kronuz requested changes Jul 29, 2020

View reviewed changes

zsol added 2 commits July 30, 2020 11:43

review feedback

6d23154

remove unused variable

6e6037e

jimmylai reviewed Jul 30, 2020

View reviewed changes

jimmylai approved these changes Jul 30, 2020

View reviewed changes

add docs

ee6c812

zsol merged commit 6a5e739 into Instagram:master Jul 31, 2020

zsol deleted the better-unused-imports branch July 31, 2020 14:54

jimmylai mentioned this pull request Aug 14, 2020

Removal of future imports #344

Closed

camillol mentioned this pull request Apr 6, 2024

Fix Literal parse error in RemoveImportsVisitor #1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RemoveUnusedImports] Support string type annotations #353

[RemoveUnusedImports] Support string type annotations #353

zsol commented Jul 28, 2020 •

edited

Kronuz Jul 28, 2020

Kronuz Jul 28, 2020

zsol Jul 28, 2020

Kronuz Jul 28, 2020

jimmylai Jul 29, 2020

Kronuz Jul 29, 2020

jimmylai Jul 30, 2020

jimmylai commented Jul 29, 2020

Kronuz Jul 29, 2020

zsol Jul 30, 2020

Kronuz Jul 30, 2020 •

edited

Kronuz Jul 29, 2020

Kronuz Jul 29, 2020

zsol Jul 30, 2020

Kronuz Jul 30, 2020

jimmylai Jul 30, 2020

jimmylai Jul 30, 2020

Kronuz Jul 29, 2020

zsol Jul 30, 2020

Kronuz Jul 30, 2020

jimmylai left a comment

zsol commented Jul 30, 2020

Kronuz commented Jul 30, 2020



		DEFAULT_SUPPRESS_COMMENT_REGEX = (
		r".\W(lint-ignore: ?unused-import\|noqa\|lint-ignore: ?F401)(\W.)?$"

[RemoveUnusedImports] Support string type annotations #353

[RemoveUnusedImports] Support string type annotations #353

Conversation

zsol commented Jul 28, 2020 • edited

Summary

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kronuz Jul 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai left a comment

Choose a reason for hiding this comment

zsol commented Jul 30, 2020

Kronuz commented Jul 30, 2020

zsol commented Jul 28, 2020 •

edited

Kronuz Jul 30, 2020 •

edited