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

Fix typing errors on ATAV #542

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Fix typing errors on ATAV #542

merged 1 commit into from
Nov 2, 2021

Conversation

stroxler
Copy link
Contributor

@stroxler stroxler commented Nov 1, 2021

Summary

We have type hints on LibCST but pyre doesn't seem to be fully hooked up.

As a result, the code was fully annotated, and was type-error-free at
runtime, but had multiple places where pyre complained either due to
type refinement not working or type errors coming from branches of
the code that weren't reachable but couldn't be statically ruled out.

Test Plan

I ran unit tests, and vendored the changes into the pyre project to see that they
typecheck.

@facebook-github-bot 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 Nov 1, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #542 (72005de) into main (a352d56) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #542      +/-   ##
==========================================
- Coverage   94.76%   94.75%   -0.01%     
==========================================
  Files         235      235              
  Lines       23199    23205       +6     
==========================================
+ Hits        21984    21989       +5     
- Misses       1215     1216       +1     
Impacted Files Coverage Δ
libcst/codemod/visitors/_apply_type_annotations.py 94.39% <85.71%> (-0.22%) ⬇️

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 a352d56...72005de. Read the comment docs.

Copy link
Contributor

@thatch thatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@stroxler stroxler merged commit 3ccfc4a into Instagram:main Nov 2, 2021
facebook-github-bot pushed a commit to facebook/pyre-check that referenced this pull request Nov 2, 2021
Summary:
The new version has:
- some fixes to reduce edge cases that sometimes prevented
  us from picking up required imports (this should lead to a
  couple percent drop in reject rates)
- the ability to add `from __future__ import annotations`
  via a flag on the codemod instead of by picking it up from
  the stub file
- the ability to count how many annotations were actually added,
  which we could hook into to get quick feedback about how
  we are doing
- the ability to suppress all import statements if no annotations
  were added, which should greatly reduce the number of junk
  "dangling import" diffs that we reject.

I also discovered that LibCST doesn't actually have pyre support,
and as a result the `main` branch of LibCST had typecheck errors
due to type refinement mismatches and edge cases that never happen
but the type system picks up. I had to put up a new PR with fixes
to avoid getting pyre errors here:
Instagram/LibCST#542

Reviewed By: grievejia

Differential Revision: D32053591

fbshipit-source-id: 913631b3a8e76490e187f1a5d22ffae36bc4e610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants