Skip to content

Use Callable[..., Any] instead of Callable[..., object] in unittest #8399

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

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

ShaneHarvey
Copy link
Contributor

@ShaneHarvey ShaneHarvey commented Jul 25, 2022

This is a follow up to resolve #8372 by using Callable[..., Any] instead of Callable[..., object] in all apis. This is needed to workaround the mypy bug: python/mypy#13220

Note I made these changes with this command:

grep -rl 'Callable.*, object' . | xargs sed -i '' 's/, object]/, Any]/g'

Edit: Updated to only change unittest.TestCase (and the DocTestCase subclass).

@ShaneHarvey
Copy link
Contributor Author

If this approach seems reasonable, I'll work on fixing the "Any" is not defined errors (caused by Any not being imported in some files).

@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 25, 2022

Thanks for the PR. I'd like to know what other maintainers think here, but I'm somewhat unhappy with this change tbh, and I'd like to sink some time into trying to fix the mypy bug before we go ahead with something like this.

The fault is really with mypy here, not typeshed. Morally, the more correct type in basically all of these cases is Callable[<parameters>, object], since Callable is covariant in its return type, and since Any should generally only be used as an "escape hatch" or for when an argument's "true" type is currently inexpressible in the type system.

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 25, 2022

I would prefer not merging this. This specific find-replace also makes me uneasy; there are definitely places where Callable[..., object] -> Callable[..., Any] can result in false negatives.
Moreover, while typeshed is often happy to make small changes to accommodate specific type checkers, a blanket change like this isn't great and is an unhappy precedent (I would still be okay with just changing the unittest functions or changing a few other locations in response to specific user complaints).

I spent a little bit of time trying to fix the mypy issue, you can see my attempt and list of other things that didn't work out of the box at python/mypy#13221 All the listed things fixed the issue, but caused other regressions and I didn't spend that much time trying to figure everything out. PR should at least be useful in terms of pointers to the relevant code.

@ShaneHarvey
Copy link
Contributor Author

I made the blanket change because users will inevitably run into the same bug on other APIs leading to more confusion, PRs, and overall more wasted time. Ideally mypy would fix the issue but in the meantime this kind of workaround seems appropriate to give users a better experience. That said, I'm happy to only change the unittest methods for now and leave the rest up to you.

@ShaneHarvey ShaneHarvey changed the title Use Callable[..., Any] instead of Callable[..., object] Use Callable[..., Any] instead of Callable[..., object] in unittest Jul 25, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@ShaneHarvey
Copy link
Contributor Author

Updated to only change unittest (and the DocTestCase subclass of TestCase). Ready for another look.

@hauntsaninja hauntsaninja added the status: deferred Issue or PR deferred until some precondition is fixed label Jul 25, 2022
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 25, 2022

Marking this as deferred for now, in case we're able to find a good mypy fix in the next several days :-)
(Will definitely make sure there's something in place, in typeshed or mypy, before the next mypy release)

@srittau
Copy link
Collaborator

srittau commented Jul 26, 2022

I am opposed to merging this. If there isn't a fix for mypy until the next release, I suggest that mypy ships a patched version of typeshed.

@ShaneHarvey
Copy link
Contributor Author

ShaneHarvey commented Sep 15, 2022

there are definitely places where Callable[..., object] -> Callable[..., Any] can result in false negatives.

This may have been true before but I don't think it is anymore now that this PR only changes unittest. My understanding is that there's no typing difference between using Callable[..., Any] vs Callable[..., object] in these stubs (besides working around the mypy bug).

Could we merge this PR or just close it if it will never be accepted?

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 15, 2022

I am fine with merging this, since none of us has yet managed to find a mypy fix, and given that this now only changes the unittest functions.

@AlexWaygood AlexWaygood requested a review from srittau September 15, 2022 21:46
@srittau
Copy link
Collaborator

srittau commented Sep 16, 2022

What makes unittest special here?

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 16, 2022

What makes unittest special here?

Well, the ones in unittest have caused a known problem for somebody. That hasn't been shown for any of the other stdlib functions.

@srittau
Copy link
Collaborator

srittau commented Sep 16, 2022

I'm still opposed to this. At this point I don't think we should regress our stubs due to type checker bugs that can be worked around with "# type: ignore". (Unless it's something really fundamental.)

@AlexWaygood
Copy link
Member

I'm still opposed to this. At this point I don't think we should regress our stubs due to type checker bugs that can be worked around with "# type: ignore". (Unless it's something really fundamental.)

I completely understand this perspective!

@srittau
Copy link
Collaborator

srittau commented Sep 16, 2022

I would be more open to this if we had some process that would ensure that we restore the "correct" stubs once the bug is fixed in mypy. And by process I don't mean "someone needs to remember to do this". :)

@ShaneHarvey
Copy link
Contributor Author

regress our stubs

Can you explain how this would be a regression? IIUC Any and object are functionally identical here.

@srittau
Copy link
Collaborator

srittau commented Sep 16, 2022

It's not a functional regression in this particular case, but stylistic one. We invested effort in streamlining callable annotations to use object return types, instead of Any. Changing it back to use Any means potentially more work in the future when investigating why we don't use object instead of Any.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2022

regress our stubs

Can you explain how this would be a regression? IIUC Any and object are functionally identical here.

Reintroducing Any instead of object would be a functional regression for mypy users who use the --disallow-any-expr option. But, since this is a fairly obscure and (I believe) little-used mypy option, I agree that this would be a relatively minor regression, which is why I'd be inclined to say "practicality beats purity" for this specific case.

But, I can see it both ways, and agree that the "fault" ultimately lies with mypy here.

@AlexWaygood
Copy link
Member

I don't think we'll be merging this — @srittau opposes it, and I don't think any other maintainers are going to come forward to argue in favour of it. Sorry @ShaneHarvey — thanks for the PR anyway.

@ShaneHarvey
Copy link
Contributor Author

No worries. Thanks for the quick responses!

@srittau
Copy link
Collaborator

srittau commented Sep 20, 2022

One idea I have is to merge this and just create the reverting PR already, marking it as "deferred". This would mean that there's less chance we forget if mypy fixes the bug. @AlexWaygood Do you think this would be a good idea?

@AlexWaygood
Copy link
Member

One idea I have is to merge this and just create the reverting PR already, marking it as "deferred". This would mean that there's less chance we forget if mypy fixes the bug. @AlexWaygood Do you think this would be a good idea?

I like that idea! We tend to go through open PRs on a fairly regular basis, so I think it would serve as an effective reminder.

@srittau srittau reopened this Sep 20, 2022
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood removed the status: deferred Issue or PR deferred until some precondition is fixed label Sep 21, 2022
@AlexWaygood
Copy link
Member

Merged! PR to revert is here: #8779.

Thanks for your patience @ShaneHarvey :)

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.

Incorrect hints for TestCase.assertRaises
4 participants