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

Allow users to provide custom diagnostic messages when unwrapping calls #13597

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

charliermarsh
Copy link
Member

Summary

You can now call return_ty_result to operate on a Result directly thereby using your own diagnostics, as in:

return dunder_getitem_method
    .call(self.db, &[slice_ty])
    .return_ty_result(self.db, value.as_ref().into(), self)
    .unwrap_or_else(|err| {
        self.add_diagnostic(
            (&**value).into(),
            "call-non-callable",
            format_args!(
                "Method `__getitem__` is not callable on object of type '{}'.",
                value_ty.display(self.db),
            ),
        );
        err.return_ty()
    });

@charliermarsh charliermarsh added the red-knot Multi-file analysis & type inference label Oct 1, 2024
(&**value).into(),
"call-non-callable",
format_args!(
"Method `__getitem__` is not callable on object of type '{}'.",
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 possible that we'll want to incorporate more information here from err. The nice thing is we can if we want.

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.

In fact I would like to :) I think the diagnostic should include the type of __getitem__ that we found to not be callable. This could really help someone in debugging why their __getitem__ isn't callable. I think that type is already available in err. I would suggest the phrasing "Method __getitem__ of type {} is not callable on object of type {}."

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will require some decisions about how to handle the UnionElement and UnionElements cases; I think we could just use the full union type and omit the detail of which element(s) were not callable, in this case? Don't have strong feelings though, open to options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll play around with it. I want something flexible but not too verbose or onerous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I went with printing the full union type for now.

Copy link
Contributor

github-actions bot commented Oct 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Lovely, thank you!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
(&**value).into(),
"call-non-callable",
format_args!(
"Method `__getitem__` is not callable on object of type '{}'.",
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.

In fact I would like to :) I think the diagnostic should include the type of __getitem__ that we found to not be callable. This could really help someone in debugging why their __getitem__ isn't callable. I think that type is already available in err. I would suggest the phrasing "Method __getitem__ of type {} is not callable on object of type {}."

(&**value).into(),
"call-non-callable",
format_args!(
"Method `__class_getitem__` is not callable on object of type '{}'.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, let's include the type we inferred for __class_getitem__.

@charliermarsh charliermarsh enabled auto-merge (squash) October 1, 2024 21:19
@charliermarsh charliermarsh merged commit ef45185 into main Oct 1, 2024
17 checks passed
@charliermarsh charliermarsh deleted the charlie/d branch October 1, 2024 21:22
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.

2 participants