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 arithmetic inference for binary expressions with float and int instances #13590

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 1, 2024

Following #13576 adds more inference for binary expressions including float and int instances rather than just IntLiteral

@zanieb zanieb added the red-knot Multi-file analysis & type inference label Oct 1, 2024
@@ -2407,6 +2407,104 @@ impl<'db> TypeInferenceBuilder<'db> {
}
}

(
Copy link
Member Author

Choose a reason for hiding this comment

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

I might futz with the organization of these and add some comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @carljm regarding the handling of IntLiteral without it being a special case

Copy link
Member

Choose a reason for hiding this comment

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

I think we probably want a general-purpose function like this for binary operations between instance types:

fn perform_binary_operation<'db>(
    db: &'db dyn Db,
    left: ClassType<'db>,
    right: ClassType<'db>,
    dunder_name: &str,
) -> Option<Type<'db>> {
    // TODO the reflected dunder actually has priority if the r.h.s. is a strict subclass of the l.h.s.
    // TODO Some other complications too!

    let dunder = left.class_member(db, dunder_name);
    if !dunder.is_unbound() {
        return dunder
            .call(db, &[Type::Instance(left), Type::Instance(right)])
            .return_ty(db);
    }

    let reflected_dunder = right.class_member(db, &format!("r{dunder_name}"));
    if !reflected_dunder.is_unbound() {
        return dunder
            .call(db, &[Type::Instance(right), Type::Instance(left)])
            .return_ty(db);
    }

    None
}

And then for e.g. inferring x * y, where x is an instance type and y is an IntLiteral type we'd just do something like

let Type::Instance(x_class) = x;
let int_class = builtins_symbol_ty(&db, "int");
perform_binary_operation(db, x_class, int_class, "__mul__")

Copy link
Member

Choose a reason for hiding this comment

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

What perform_binary_operation when passed "__mul__" does is:

  • Lookup the __mul__ function on the type of x
  • If it exists, call __mul__(x, y)
  • If type(x).__mul__ did not exist, lookup __rmul__ on the type of y
  • If type(y).__rmul__ exists, call __rmul__(y, x)
  • Else, return None

This is a generalised routine that will work for inferring binary operations between any two instance types. And unless we have a literal on both sides of the binary operation, we may as well treat an IntLiteral variant as "just an instance of int", and fallback to the generalised routine

Copy link
Member

Choose a reason for hiding this comment

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

The algorithm for inferring binary operations is unfortunately really really complicated in its totality, though. See https://snarky.ca/unravelling-binary-arithmetic-operations-in-python/ for all the gory details!

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! Thanks for the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, @AlexWaygood perfectly summarized what I was talking about in standup, and with lots more useful details, too!!

Copy link
Contributor

@carljm carljm Oct 1, 2024

Choose a reason for hiding this comment

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

Yeah, so either way we are going to have large match statements, but once we are in the realm of "things typeshed can tell us" (which is all of the cases added in this PR), we should avoid adding new special case match arms and just have a generic version that looks up and "calls" the appropriate dunder methods like Alex suggests. Pretty much we should only have special-case implementations for literal types if there is a possibility we can infer a literal type out of the operation (something typeshed clearly can't do), otherwise we should be falling back to the general case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I was thinking something was wrong here but wasn't aware typeshed does all of this.

Copy link
Member

@AlexWaygood AlexWaygood Oct 1, 2024

Choose a reason for hiding this comment

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

Oh yeah, we go to great lengths to accurately reflect all the dunders in typeshed: https://github.com/python/typeshed/blob/44aa63330b03bdacab731af2333ff9bf70855de3/stdlib/builtins.pyi#L227-L330

And we have quite extensive testing to check that none are omitted from the stub or have inconsistent signatures with what actually exists at runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants