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

[ScopeProvider] Expose more granular Assignments and Accesses for dotted imports #284

Merged
merged 6 commits into from
Apr 21, 2020

Conversation

zsol
Copy link
Member

@zsol zsol commented Apr 16, 2020

Summary

This PR adds extra Assignments created by the ScopeProvider for dotted imports like import a.b.c. For such an import, there are now three assignments, keyed by a, a.b, and a.b.c. For accesses, only the most specific access is recorded and attached to the appropriate assignment.

Test Plan

Added unit tests.

Fixes #281.

@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 16, 2020
@zsol zsol requested a review from jimmylai April 16, 2020 15:35
@codecov-io
Copy link

Codecov Report

Merging #284 into master will increase coverage by 0.00%.
The diff coverage is 97.43%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #284   +/-   ##
=======================================
  Coverage   93.96%   93.96%           
=======================================
  Files         219      219           
  Lines       21245    21288   +43     
=======================================
+ Hits        19962    20004   +42     
- Misses       1283     1284    +1     
Impacted Files Coverage Δ
libcst/metadata/scope_provider.py 96.82% <94.87%> (-0.08%) ⬇️
libcst/codemod/visitors/_remove_imports.py 92.99% <100.00%> (-0.14%) ⬇️
...bcst/codemod/visitors/tests/test_remove_imports.py 100.00% <100.00%> (ø)
libcst/metadata/tests/test_scope_provider.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 621d9a9...7a69cd2. Read the comment docs.

@jimmylai
Copy link
Contributor

This is a pretty good enhancement. Import a.b.c not only imports module a.b.c but also a and a.b. It's a new concept added to scope analysis. Can you provide some example and documentation to explain it?
https://libcst.readthedocs.io/en/latest/metadata.html#scope-metadata

@zsol zsol requested a review from jimmylai April 18, 2020 13:17
"""

after = """
import a.b, a.b.c
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a.b.c is not removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand a.b.c may have side effect and thus is not safe to remove. My question is how do we not remove it?
The ScopeVisitor looks like will use name a.b to store access to assignment a.b.
Is it because import a.b.c creates three assignment lookup entries (a, a.b, a.b.c), a.b access is recorded to the second one so, the import is not removed?
In the case of x.y.z, we only associate access x.y.z to import x.y.z instead of import x.y (which are x and x.y).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because a.b.c is looked at in isolation (without considering the existence of the import for a.b).
So essentially because of the same reason this is not fixed:

import a
import a
a

This is a limitation of the RemoveUnusedImportsVisitor, it only removes unused imports, not duplicates.

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

Overall looks great.
One last thing, can you add a test case to cover this line?
https://codecov.io/gh/Instagram/LibCST/pull/284/diff?src=pr&el=tree#D2-642...643
Add a from module import * in a test case should make it covered.

@zsol zsol merged commit 477a03e into Instagram:master Apr 21, 2020
@zsol zsol deleted the dotted-import branch April 21, 2020 09:27
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.

[RFC] Analyze attribute access from imports to support dead import clean up better
4 participants