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 consider-merging-isinstance #1009

Merged
merged 5 commits into from
Dec 3, 2022

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Dec 3, 2022

Comment on lines +5 to +20
def isinstances():
"Examples of isinstances"
var = range(10)

# merged
if isinstance(var[1], (int, float)):
pass
result = isinstance(var[2], (int, float))

# not merged
if isinstance(var[3], int) or isinstance(var[3], float) or isinstance(var[3], list) and True: # [consider-merging-isinstance]
pass
result = isinstance(var[4], int) or isinstance(var[4], float) or isinstance(var[5], list) and False # [consider-merging-isinstance]

result = isinstance(var[5], int) or True or isinstance(var[5], float) # [consider-merging-isinstance]

Copy link
Contributor Author

@harupy harupy Dec 3, 2022

Choose a reason for hiding this comment

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

image

I'm not sure if this is intended, but the start location of a BoolOp expression is the start location of or.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RustPython's code:

https://github.com/RustPython/RustPython/blob/18af44b7c7410c3448df1f3b080bd358e586d265/compiler/parser/python.lalrpop#L758

OrTest: ast::Expr = {
    <e1:AndTest> <location:@L> <e2:("or" AndTest)*> <end_location:@R> => {
        if e2.is_empty() {
            e1
        } else {
            let mut values = vec![e1];
            values.extend(e2.into_iter().map(|e| e.1));
            ast::Expr {
                location,
                end_location: Some(end_location),
                custom: (),
                node: ast::ExprKind::BoolOp { op: ast::Boolop::Or, values }
            }
        }
    },
};

Copy link
Contributor Author

@harupy harupy Dec 3, 2022

Choose a reason for hiding this comment

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

Moving <location:@L> before <e1:AndTest> seems to work?

https://github.com/harupy/RustPython/blob/3835575a831dafe48fa4ddbae895960a377f1227/compiler/parser/python.lalrpop#L759

OrTest: ast::Expr = {
    <location:@L> <e1:AndTest> <e2:("or" AndTest)*> <end_location:@R> => {
    ^^^^^^^^^^^^^
        if e2.is_empty() {
            e1
        } else {
            let mut values = vec![e1];
            values.extend(e2.into_iter().map(|e| e.1));
            ast::Expr {
                location,
                end_location: Some(end_location),
                custom: (),
                node: ast::ExprKind::BoolOp { op: ast::Boolop::Or, values }
            }
        }
    },
};

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import ast

# Visitor to print out node locations
class V(ast.NodeVisitor):
    def visit_BoolOp(self, node: ast.BoolOp) -> None:
        print(
            node.__class__.__name__,
            (node.lineno, node.col_offset),
            (node.end_lineno, node.end_col_offset),
        )
        self.generic_visit(node)

V().visit(ast.parse("a or b or c"))

prints out:

BoolOp (1, 0) (1, 11)

Copy link
Member

Choose a reason for hiding this comment

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

Oh cool. Do you want to submit an upstream PR for this? Or I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charliermarsh charliermarsh merged commit e05e1cd into astral-sh:main Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants