Apply type annotations from a stub. #265

pradeep90 · 2020-03-16T09:22:04Z

Basically move the apply_annotations code from Pyre. I had discussed this with Jimmy Lai. The main motivation for moving it to libcst is that the code deals purely with libcst concepts like CSTVisitor and CSTTransformer and we want to use it in two separate projects: Pyre and MonkeyType.

This is not implemented as a normal Codemod because, instead of working on just one module, it works on two modules: the stub module from which to collect annotations and the source module to which to apply the annotations.

Summary

Test Plan

Added the tests as well.

codecov-io · 2020-03-16T09:24:54Z

Codecov Report

Merging #265 into master will increase coverage by 0.04%.
The diff coverage is 96.58%.

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   93.88%   93.93%   +0.04%     
==========================================
  Files         214      216       +2     
  Lines       20818    21140     +322     
==========================================
+ Hits        19546    19857     +311     
- Misses       1272     1283      +11

Impacted Files	Coverage Δ
libcst/codemod/visitors/_apply_type_annotations.py	`96.38% <96.38%> (ø)`
libcst/codemod/visitors/__init__.py	`100.00% <100.00%> (ø)`
...emod/visitors/tests/test_apply_type_annotations.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8619a...0441b66. Read the comment docs.

libcst/codemod/commands/tests/test_apply_stub_annotations.py

libcst/codemod/commands/apply_stub_annotations.py

jimmylai

Thanks for submitting this!
The goal is to use this in Pyre and MonkeyType.
@carljm Can you take a look to see if this takes care the need from MonkeyType as well?

libcst/codemod/commands/apply_stub_annotations.py

carljm · 2020-03-19T19:16:15Z

@jimmylai Yes, this will meet MonkeyType's needs! Will let you and @thatch review the details since you've already done thorough reviews.

pradeep90 · 2020-03-20T20:18:23Z

Make it a ContextAwareTransformer, rename it to ApplyTypeAnnotationsVisitor, and move it to codemod/visitors/.
Use GatherImportsVisitor to gather imports.
Add a static method add_stub_to_context so that users can schedule a stub whose types are to be applied later.
Use assertCodemod and data_provider in the tests.
Add documentation.
Remove fixmes.

thatch · 2020-03-20T23:47:07Z

docs/source/codemods.rst

@@ -153,3 +153,5 @@ inside codemods. As of now, the list includes the following helpers.
  :exclude-members: CONTEXT_KEY, visit_Module, leave_ImportFrom, leave_Module
 .. autoclass:: libcst.codemod.visitors.RemoveImportsVisitor
  :exclude-members: CONTEXT_KEY, METADATA_DEPENDENCIES, visit_Module, leave_ImportFrom, leave_Import
+.. autoclass:: libcst.codemod.visitors.ApplyTypeAnnotationsVisitor


@jimmylai is there a reason these aren't alphabetized?

No, we basically just add them sequentially when we add new visitors.

libcst/codemod/visitors/_apply_type_annotations.py

thatch

Just flagged a couple of lines that might be under-tested. If there is good coverage around them, I'm happy with this. Will let Jimmy give the approve.

libcst/codemod/visitors/_apply_type_annotations.py

jimmylai

Thank you for working on all the changes. The test suite looks really good.
Just found the implementation can be simplified a lot by reusing AddImportsVisitor.

jimmylai · 2020-03-21T05:01:36Z

docs/source/codemods.rst

@@ -153,3 +153,5 @@ inside codemods. As of now, the list includes the following helpers.
  :exclude-members: CONTEXT_KEY, visit_Module, leave_ImportFrom, leave_Module
 .. autoclass:: libcst.codemod.visitors.RemoveImportsVisitor
  :exclude-members: CONTEXT_KEY, METADATA_DEPENDENCIES, visit_Module, leave_ImportFrom, leave_Import
+.. autoclass:: libcst.codemod.visitors.ApplyTypeAnnotationsVisitor


No, we basically just add them sequentially when we add new visitors.

libcst/codemod/visitors/_apply_type_annotations.py

pradeep90 · 2020-03-23T05:05:54Z

Use get_full_name_for_node.
Use AddImports instead of manually adding imports.
Remove old import code and ImportStatement.

Note: I discovered a couple of edge cases regarding imports with the same name, which I'll address in a follow-up PR.

pradeep90 · 2020-03-23T05:07:22Z

libcst/codemod/visitors/tests/test_apply_type_annotations.py

+            # Keep the existing `import A` instead of using `from A import B`.
+            (
+                """
+                import bar
+
+                def foo() -> bar.Baz: ...
+                """,
+                """
+                import bar
+
+                def foo():
+                    return returns_baz()
+                """,
+                """
+                import bar
+
+                def foo() -> bar.Baz:
+                    return returns_baz()
+                """,
+            ),


I'm keeping the use of GatherImports to get and check against the imports that already exist in the source module, because otherwise it adds an extra import from bar import Baz and makes the return type of foo be Baz.

jimmylai · 2020-03-23T06:31:09Z

libcst/codemod/visitors/_apply_type_annotations.py

+        if self.is_generated:
+            return original_node


@generated is FB specific logic which is not generic.
Our CLI includes the option to skip generated file.

LibCST/libcst/tool.py

Line 481 in c21ae59

"--include-generated", action="store_true", help="Codemod generated files."

LibCST/libcst/tool.py

Lines 657 to 659 in c21ae59

"generated_code_marker": _StrSerializer(

"String that LibCST should look for in code which indicates "

+ "that the module is generated code."

We should probably reuse those existing logic or at least make the current implementation configurable (e.g. add skip_generated: bool = True parameter to add_stub_to_context).

Ok. I removed our ad hoc handling since it looks like your tool already supports it.

jimmylai

Other than the comments, looks great!
Look forward to the follow up edge case fixes.

Basically move the apply_annotations code from Pyre. Make it a ContextAwareTransformer named ApplyTypeAnnotationsVisitor. Use GatherImportsVisitor to collect imports. Add a static method `add_stub_to_context` so that users can schedule a stub whose types are to be applied later. Use `assertCodemod` and `data_provider` in the tests. Add documentation. Remove fixmes.

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 Mar 16, 2020

thatch reviewed Mar 16, 2020

View reviewed changes

libcst/codemod/commands/tests/test_apply_stub_annotations.py Outdated Show resolved Hide resolved

libcst/codemod/commands/apply_stub_annotations.py Outdated Show resolved Hide resolved

libcst/codemod/commands/apply_stub_annotations.py Outdated Show resolved Hide resolved

jimmylai reviewed Mar 19, 2020

View reviewed changes

libcst/codemod/commands/apply_stub_annotations.py Outdated Show resolved Hide resolved

libcst/codemod/commands/apply_stub_annotations.py Outdated Show resolved Hide resolved

jimmylai requested a review from carljm March 19, 2020 17:24

pradeep90 force-pushed the master branch from 3e99494 to 19c681b Compare March 20, 2020 20:16

thatch reviewed Mar 20, 2020

View reviewed changes

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

thatch reviewed Mar 20, 2020

View reviewed changes

pradeep90 force-pushed the master branch from 19c681b to 0441b66 Compare March 21, 2020 01:50

jimmylai reviewed Mar 21, 2020

View reviewed changes

pradeep90 force-pushed the master branch 2 times, most recently from a9294b3 to 8ffd760 Compare March 23, 2020 04:54

pradeep90 commented Mar 23, 2020

View reviewed changes

jimmylai reviewed Mar 23, 2020

View reviewed changes

jimmylai approved these changes Mar 23, 2020

View reviewed changes

pradeep90 force-pushed the master branch from 8ffd760 to b8c49ef Compare March 23, 2020 16:09

jimmylai merged commit 1b9a52b into Instagram:master Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply type annotations from a stub. #265

Apply type annotations from a stub. #265

pradeep90 commented Mar 16, 2020

codecov-io commented Mar 16, 2020 •

edited

Loading

jimmylai left a comment

carljm commented Mar 19, 2020

pradeep90 commented Mar 20, 2020

thatch Mar 20, 2020

jimmylai Mar 21, 2020

thatch left a comment

jimmylai left a comment

jimmylai Mar 21, 2020

pradeep90 commented Mar 23, 2020

pradeep90 Mar 23, 2020

jimmylai Mar 23, 2020

pradeep90 Mar 23, 2020

jimmylai left a comment

	"generated_code_marker": _StrSerializer(
	"String that LibCST should look for in code which indicates "
	+ "that the module is generated code."

Apply type annotations from a stub. #265

Apply type annotations from a stub. #265

Conversation

pradeep90 commented Mar 16, 2020

Summary

Test Plan

codecov-io commented Mar 16, 2020 • edited Loading

Codecov Report

jimmylai left a comment

Choose a reason for hiding this comment

carljm commented Mar 19, 2020

pradeep90 commented Mar 20, 2020

thatch Mar 20, 2020

Choose a reason for hiding this comment

jimmylai Mar 21, 2020

Choose a reason for hiding this comment

thatch left a comment

Choose a reason for hiding this comment

jimmylai left a comment

Choose a reason for hiding this comment

jimmylai Mar 21, 2020

Choose a reason for hiding this comment

pradeep90 commented Mar 23, 2020

pradeep90 Mar 23, 2020

Choose a reason for hiding this comment

jimmylai Mar 23, 2020

Choose a reason for hiding this comment

pradeep90 Mar 23, 2020

Choose a reason for hiding this comment

jimmylai left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 16, 2020 •

edited

Loading