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

Qualify imported symbols when the dequalified form would cause a conflict #674

Merged
merged 4 commits into from
Apr 12, 2022
Merged

Qualify imported symbols when the dequalified form would cause a conflict #674

merged 4 commits into from
Apr 12, 2022

Conversation

martindemello
Copy link
Contributor

Summary

Adds a preliminary pass that scans the stub file for all imported
symbols, and collects the ones that cannot be safely dequalified.

Fixes #673

Test Plan

Added a test case to test_apply_type_annotations.

@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 Apr 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022

Codecov Report

Merging #674 (23416dc) into main (0a8ae91) will increase coverage by 0.00%.
The diff coverage is 94.26%.

@@           Coverage Diff            @@
##             main     #674    +/-   ##
========================================
  Coverage   94.80%   94.81%            
========================================
  Files         246      246            
  Lines       25453    25591   +138     
========================================
+ Hits        24131    24263   +132     
- Misses       1322     1328     +6     
Impacted Files Coverage Δ
...emod/visitors/tests/test_apply_type_annotations.py 100.00% <ø> (ø)
libcst/codemod/visitors/_apply_type_annotations.py 95.36% <93.91%> (-0.22%) ⬇️
libcst/codemod/visitors/_gather_imports.py 96.15% <100.00%> (+3.13%) ⬆️

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 0a8ae91...23416dc. Read the comment docs.

…lict.

Adds a preliminary pass that scans the stub file for all imported
symbols, and collects the ones that cannot be safely dequalified.

Fixes #673
Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

This looks good to me, up to being a bit too abbreviation-heavy for the existing style.

But it looks to me like it probably only handles collisions within the stub - that's all that's tested, and as far as I can tell we're only running import_gatherer on the stub.

If that's the case, we should consider also handling collisions with the existing code - I'm fine with deferring that (I could try adding it later) but curious if it would be easy to add right now since you already have context.

libcst/codemod/visitors/_apply_type_annotations.py Outdated Show resolved Hide resolved
libcst/codemod/visitors/_gather_imports.py Show resolved Hide resolved
@martindemello
Copy link
Contributor Author

handling collisions in the existing code looks simple enough to add to this PR after all, will push an update.

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Nice, this looks good.

Pyre is complaining about a couple of lines. We don't yet have proper github integration (hopefully in a couple of weeks!) so it's a pain to find the problem, I commented on the relevant lines

libcst/codemod/visitors/_apply_type_annotations.py Outdated Show resolved Hide resolved
libcst/codemod/visitors/_apply_type_annotations.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Looks good!

@stroxler stroxler merged commit e6f208c into Instagram:main Apr 12, 2022
@stroxler
Copy link
Contributor

Thanks for the fix @martindemello - I've been wanting to handle this edge case for a long time, we actually hit it pretty often in pyre infer

@martindemello martindemello deleted the qualify branch April 12, 2022 19:19
@martindemello
Copy link
Contributor Author

i've been wanting to handle it for a while too :) imports are the last thing blocking us from using this in pytype.

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.

ApplyTypeAnnotationsVisitor: imported symbols should remain qualified when they conflict
4 participants