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

[flake8-pyi] Allow overloaded __exit__ and __aexit__ definitions (PYI036) #11057

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 20, 2024

Summary

Fixes #11044

We should definitely allow overloaded definitions for __exit__ and __aexit__. Nonetheless, validating overloads is pretty tricky without the full machinery of a type checker, so this PR deliberately takes a pretty conservative approach. If there are exactly two overloads, both overloads have exactly three annotated non-self non-keyword-only arguments each, and both overloads have no keyword-only arguments or variadic arguments -- then, for both overloads, we check to see if the overload matches one of two recognised overload variants (permitting small variations such as if one parameter is hinted with object or _typeshed.Unused). If there's an unexpected number of arguments, however, we just don't check the overload.

Test plan

cargo test.

@AlexWaygood AlexWaygood added bug Something isn't working rule Implementing or modifying a lint rule labels Apr 20, 2024
Copy link
Contributor

github-actions bot commented Apr 20, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood marked this pull request as draft April 20, 2024 14:12
@bswck
Copy link
Contributor

bswck commented Apr 20, 2024

Oh, good to see it! I've been having an issue with this recently. 👍👍👍

Comment on lines +136 to +147
let non_self_positional_args: SmallVec<[&ParameterWithDefault; 3]> = parameters
.posonlyargs
.iter()
.chain(parameters.posonlyargs.iter())
.collect::<SmallVec<[&ParameterWithDefault; 4]>>();
.chain(parameters.args.iter())
.skip(1)
.collect();
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 think the logic here has been wrong for a while. It should have always been parameters.posonlyargs.iter().chain(parameters.args.iter()), not parameters.args.iter().chain(parameters.posonlyargs.iter()). I think it didn't come up because it the bug lead to false negatives rather than false positives, so nobody noticed!

@AlexWaygood AlexWaygood marked this pull request as ready for review April 20, 2024 21:24
@AlexWaygood AlexWaygood force-pushed the pyi036-overloads branch 4 times, most recently from 21769ae to a72e54f Compare April 22, 2024 11:14
};

// Collect all the overloads for this method into a SmallVec
let function_overloads: SmallVec<[&StmtFunctionDef; 2]> = parent_class_def
Copy link
Member

Choose a reason for hiding this comment

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

I would just use a normal Vec here personally. It's not a very hot path, right? This is exactly if you're defining one of a small set of methods, and it has an overload?

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 shouldn't be very hot, no. I mainly used a SmallVec because it was already being used elsewhere in this file, and it is exceedingly unlikely that anybody would use more than two overloads for __exit__ or __aexit__. Is there a disadvantage to using a SmallVec here?

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

I think if it were me making this decision on a desert island, I'd probably just skip overloads since I wouldn't find the complexity-to-value tradeoff to be good enough (we're trading ~400 lines of code for a scenario that seems somewhat rare). But I defer to you as the owner of the rule, and it looks like you have intuition that this is worth supporting.

@AlexWaygood
Copy link
Member Author

I think if it were me making this decision on a desert island, I'd probably just skip overloads since I wouldn't find the complexity-to-value tradeoff to be good enough

Yeah I know what you mean. My initial thought when I saw the issue was "well, that's going to add extra complexity to the check, and it's a less common case, so we may as well skip it". But (although we rarely bother with it in typeshed, it seems like extra complexity for little gain), Adam's recent blog post is strictly-speaking correct that it's more accurate to use overloads for this case, and that blog seems to be making the rounds at the moment. And if you are going to use overloads, then I think it's really easy to get it subtly wrong by e.g. using Exception instead of BaseException, the same as if you don't use overloads. So I do think there is value to checking overloads.

This may be partly sunk-cost fallacy on my part though, given that I've gone ahead an implemented it now...

I think I'll add a few more test cases to make sure that every conceivable scenario is covered, then go ahead with this.

@AlexWaygood AlexWaygood enabled auto-merge (squash) April 24, 2024 15:43
@AlexWaygood AlexWaygood merged commit e3fde28 into main Apr 24, 2024
18 checks passed
@AlexWaygood AlexWaygood deleted the pyi036-overloads branch April 24, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bad-exit-annotation (PYI036) triggers on typing.overload definitions
3 participants