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

Implement CollapseIsinstanceChecksRule #116

Merged
merged 8 commits into from
Sep 8, 2020

Conversation

isidentical
Copy link
Contributor

Summary

if isinstance(x, int) or isinstance(x, str): ...
elif isinstance(x, bytes) or isinstance(x, bytearray): ...

becomes

if isinstance(x, (int, str)): ...
elif isinstance(x, (bytes, bytearray)): ...

Test Plan

  • Added unittests

@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 Sep 4, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #116 into master will increase coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   83.75%   84.30%   +0.55%     
==========================================
  Files          73       77       +4     
  Lines        2905     3199     +294     
==========================================
+ Hits         2433     2697     +264     
- Misses        472      502      +30     
Impacted Files Coverage Δ
fixit/rules/chained_instance_check.py 100.00% <100.00%> (ø)
fixit/rule_lint_engine.py 95.29% <0.00%> (-4.71%) ⬇️
fixit/common/ignores.py 97.74% <0.00%> (-0.44%) ⬇️
fixit/cli/__init__.py 56.17% <0.00%> (ø)
fixit/cli/apply_fix.py 31.40% <0.00%> (ø)
fixit/cli/run_rules.py 41.53% <0.00%> (ø)
fixit/common/config.py 87.17% <0.00%> (ø)
fixit/common/document.py 23.94% <0.00%> (ø)
fixit/common/tests/test_ignores.py 100.00% <0.00%> (ø)
fixit/common/unused_suppressions.py 77.06% <0.00%> (ø)
... and 5 more

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 6dc3b44...e33c9ac. Read the comment docs.

Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

Looks like a great lint rule, thanks for working on it. I only have one comment.

fixit/rules/chained_instance_check.py Show resolved Hide resolved
Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

LGTM, awesome work :)

@isidentical
Copy link
Contributor Author

LGTM, awesome work :)

Glad to hear that, thanks!

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.

@isidentical , this is a great rule that makes code more efficient and more simpler.
I've used this to optimize some hot path at work.

The code is 100% tested. Awesome job!
https://codecov.io/gh/Instagram/Fixit/pull/116/diff?src=pr&el=tree#diff-Zml4aXQvcnVsZXMvY2hhaW5lZF9pbnN0YW5jZV9jaGVjay5weQ==

Other than isinstance, some other Python builtin methods can also take a Tuple to combine multiple checks e.g. startswith and endswith of str. The pattern is slightly different (it needs to be a str.startswith, so probably better to build as another lint rule).

fixit/rules/chained_instance_check.py Outdated Show resolved Hide resolved
fixit/rules/chained_instance_check.py Outdated Show resolved Hide resolved
fixit/rules/chained_instance_check.py Show resolved Hide resolved
@isidentical
Copy link
Contributor Author

Other than isinstance, some other Python builtin methods can also take a Tuple to combine multiple checks e.g. startswith and endswith of str. The pattern is slightly different (it needs to be a str.startswith, so probably better to build as another lint rule).

Exactly, that is what I was thinking, you basically read my mind!

@isidentical isidentical requested a review from jimmylai September 6, 2020 14:17
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.

CollapseIsInstanceChecksRule (with capital I) also looks good to me if you like it more.

Comment on lines 156 to 161
for context in self.get_metadata(QualifiedNameProvider, func):
if (
context.name != "builtins.isinstance"
or context.source is not QualifiedNameSource.BUILTIN
):
return None
Copy link
Contributor

@jimmylai jimmylai Sep 7, 2020

Choose a reason for hiding this comment

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

Why not simply use QualifiedNameProvider.has_name(self, name="builtins.isinstance", QualifiedNameSource.BUILTIN) to be simpler?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this checks looks redundant since we already check isinstance in _collect_isinstance_calls and we can simply use QualifiedNameProvider.has_name(self, name="builtins.isinstance", QualifiedNameSource.BUILTIN) in _collect_isinstance_calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK has_name check for any appearance, but we want to ensure that it is always the isinstance we found (just like all instead of any). I don't know though if it is the right way or not, can change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, has_name checks if any of the qualified name matches and that's how we usually checks the qualified names. https://libcst.readthedocs.io/en/latest/metadata.html#libcst.metadata.QualifiedNameProvider

The QualifiedName analysis is based on scope analysis to find assignments from parent scopes and it returns all possible assignments. Sometimes when the code has branches with redefinition of the same name, it could return multiple qualified names. Use all can miss lint suggestion on those cases while use any can provide the correct suggestion based on our experience.
E.g.
https://github.com/Instagram/LibCST/blob/7ca738bf3964ac48ebb36b1b362153c87c29a706/libcst/codemod/commands/rename.py#L262-L267
https://github.com/Instagram/LibCST/blob/c023fa7c4caff3fd2b3946080f9a58b539b10363/libcst/codemod/commands/convert_namedtuple_to_dataclass.py#L31-L47

@isidentical isidentical requested a review from jimmylai September 7, 2020 20:05
def foo():
def isinstance(x, y):
return _foo_bar(x, y)
if isinstance(x, y) or isinstance(x, z):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is not collapsed? This is basically the same as the first Invalid test case and should be collapsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you see the function def isinstance(x, y): that redefines isinstance name in the current scope?

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that. It makes sense now!

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.

Looks great!
Currently the rule only consider the bare isinstance and or operator. In some other collapse-able case like not isinstance(...) and not isinstance(...) which can be collapsed as not isinstance(..., [...]). I think that's a less common use case and can leave as a future enhancement if you like to improve it.

Found another builtin function issubclass can also takes a Tuple but it's an all check (unlike the any check in isinstance). So issubclass(x, y) and issubclass(x, z) can be collapsed. That can be another lint rule by reusing the unwrap and collect_targets helpers provided in this lint rule.

def foo():
def isinstance(x, y):
return _foo_bar(x, y)
if isinstance(x, y) or isinstance(x, z):
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that. It makes sense now!

@jimmylai jimmylai merged commit 2ae94c2 into Instagram:master Sep 8, 2020
@isidentical
Copy link
Contributor Author

Found another builtin function issubclass can also takes a Tuple but it's an all check (unlike the any check in isinstance). So issubclass(x, y) and issubclass(x, z) can be collapsed. That can be another lint rule by reusing the unwrap and collect_targets helpers provided in this lint rule.

I am aware of issubclass taking a tuple of classes, but AFAIK it is also an or gate. issubclass(x, (y, z)) == issubclass(x, y) or issubclass(x, z)

@jimmylai jimmylai changed the title Implement ChainedInstanceRule Implement CollapseIsinstanceChecksRule Sep 8, 2020
@jimmylai
Copy link
Contributor

jimmylai commented Sep 8, 2020

Found another builtin function issubclass can also takes a Tuple but it's an all check (unlike the any check in isinstance). So issubclass(x, y) and issubclass(x, z) can be collapsed. That can be another lint rule by reusing the unwrap and collect_targets helpers provided in this lint rule.

I am aware of issubclass taking a tuple of classes, but AFAIK it is also an or gate. issubclass(x, (y, z)) == issubclass(x, y) or issubclass(x, z)

Just confirmed that it's an OR. I was confused by the documentation in
https://docs.python.org/3/library/functions.html#issubclass
It says "in which case every entry in classinfo will be checked". If it'll return early when any subclass matches (which it should do to be efficient), then the description isn't fully accurate.

@isidentical would you like to refactor CollapseIsinstanceChecksRule to provide collapsing issubclass checks in the same rule or as another rule?

@isidentical
Copy link
Contributor Author

@isidentical would you like to refactor CollapseIsinstanceChecksRule to provide collapsing issubclass checks in the same rule or as another rule?

I think it suits to the purpose, so I would be OK with it. No need to mirroring the whole logic. If it sounds nice to you, I can just send a very small / simple PR for that.

@jimmylai
Copy link
Contributor

jimmylai commented Sep 8, 2020

@isidentical would you like to refactor CollapseIsinstanceChecksRule to provide collapsing issubclass checks in the same rule or as another rule?

I think it suits to the purpose, so I would be OK with it. No need to mirroring the whole logic. If it sounds nice to you, I can just send a very small / simple PR for that.

Yes, please go head, I'm happy to review and merge it.
Duplicate the code is not ideal.

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.

5 participants