Make weakref.finalize generic per todo comment

[kkirsche]

Fix #8437

This merge request makes weakref.finalize generic:

1. _P is the ParamSpec of the function. This is used in the __init__ function and the detach / peek calls, as described here
2. _T1 is the captured object, which is used in the detach and peek returns
3. _T2 is the captured return value of the func callable, which is re-used in the detach and peek methods.
4. As _ is an unused bitbucket, change it from Any to object, as any object is accepted since it's an unused argument

[AlexWaygood]

Can you change the TypeVar used for the Callable return-type to be _R? I think that would make it more readable.

[AlexWaygood]

@sobolevn, could you possibly take a look? This looks reasonable to me, but I'm not 100% sure I understand all the complexities of this class.

[sobolevn]

I think this is a perfect candidate for a test case 🤔
I cannot evaluate this in my head 😆

[kkirsche]

Course. @AlexWaygood — I had simply tried to re-use the ones that were there for cleanliness.

@sobolevn: Can you provide information about how to actually do that? I haven't worked with typeshed's test case stuff

[AlexWaygood]

@kkirsche, I wrote a README here: https://github.com/python/typeshed/tree/master/test_cases. Could you take a look, and let me know if anything is unclear? If anything is, feel free to ask!

[AlexWaygood]

Also, looks like either pip or GitHub Actions is having a bad day.

[AlexWaygood]

Also, looks like either pip or GitHub Actions is having a bad day.

Yeah, it's PyPI: https://status.python.org

[kkirsche]

I've added a test case, I may need to make an update to it though based on how assert_type works to reduce the accuracy of the _ResultStructure type alias

[AlexWaygood]

Unlike in the rest of typeshed, the test cases are .py files, and they need to run on 3.7, so you'll need to use typing.Callable and typing.Tuple instead of collections.abc.Callable and tuple.

[kkirsche]

I think I have updated the test case properly for the imports, but apologies if Dict or Optional shouldn't have been imported in this way.

I will say, assert_type didn't behave how I expected it to from the name, but it's probably meant to verify the inferred type where I expected it to assert against the provided type against the value itself (so that you can assert that it returned the value rather than None for example, in the case of an Optional).

I think I had expected it to be named something like assert_revealed_type or assert_inferred_type for the way it seems to behave, as I understand it.

EDIT: Attempt to better clarify the intent of the statement

[AlexWaygood]

This test doesn't look very useful to me at the moment. The usage of finalize looks contrived, and the way the test is constructed means that it's just repeating the stub.

For this case, I think what would be more useful would be a test similar to test_logging or test_object -- a test that proves that idiomatic usage of this class doesn't cause the type checker to emit any false-positive errors. We need a minimal snippet of code that uses all the bits of this class that we're changing, and uses them in an idiomatic way.

[kkirsche]

I'd need to look around to find one. I've never seen this used for it's intended purpose, only because someone didn't understand the weak ref docs.

It may be worth noting that in the README though that they're meant to be idiomatic more so than basic smoke tests.

[AlexWaygood]

It may be worth noting that in the README though that they're meant to be idiomatic more so than basic smoke tests.

Different tests serve different purposes -- for pow(), the overloads are complicated enough that we really do want just basic smoke tests. But for something like this, each function signature in and of itself is fairly simple. Here, the concern is more that we might unexpectedly start cause a false-positive error to be emitted because we don't fully understand how this class is meant to be used.

But yeah, that's useful feedback! I'll try to improve the README.

[AlexWaygood]

But yeah, that's useful feedback! I'll try to improve the README.

I filed #8441 as a TODO for myself.

[kkirsche]

But for something like this, each function signature in and of itself is fairly simple. Here, the concern is more that we might unexpectedly start cause a false-positive error to be emitted because we don't fully understand how this class is meant to be used.

I'll make an attempt to implement this, but this comment raised more questions for me than it answered. Specifically, what is it you intend to test.

From looking at examples, many references this is used for are to close an object when something like a session or browser is no longer in play. In cases like the ones I'm finding, there is nothing to test for.

For example:

• https://github.com/MechanicalSoup/MechanicalSoup/blob/main/mechanicalsoup/browser.py#L45-L50
• https://github.com/dask/dask-cloudprovider/blob/main/dask_cloudprovider/aws/ecs.py#L1051

In a few cases, detach is used to check if it needs to manually clean up:

• https://github.com/sarugaku/vistir/blob/master/src/vistir/compat.py#L233

So I guess my question is what really is the intent of the test, as to me this sounds like we'll be testing the behavior of weakref rather than actually testing the types themselves?

EDIT: fix spell check errors while on phone

[AlexWaygood]

In cases like the ones I'm finding, there is nothing to test for.

If we can't find (or think of) a single example of idiomatic usage of this class where this PR would improve the user experience when using a type checker... then we probably shouldn't be complicating the stub like this, and should maybe instead just delete the TODO comment :)

[kkirsche]

But we can validate that the types are more accurate than they were before, regardless of idiomatic usage and we can verify using a non-idiomatic test that implementation behavior described in the official documentation is successfully reproduced by the API.

As I see it, the ask for a test case is the root issue, not the types.

[kkirsche]

With that said if you feel it's not worth it, we can just remove the todo.

Not trying to argue, I just don't find the argument convincing or the request for an idiomatic test to actually be what's necessary here based on the implementation of the class and documentation about its behaviors.

As I understand it, other tests would simply validate that None comes back when the ref goes out of scope or the arguments come back while it's in scope.

Either way, we can go in either direction you prefer. Just let me know 👍

[AlexWaygood]

Either way, we can go in either direction you prefer. Just let me know 👍

Mypy has only very recently added support for parameterising classes over a ParamSpec, and there are still known bugs in its implementation of this feature. As such, for a change like this, we don't know what the consequences might be of complicating the stub in this way. There's always a risk of accidentally introducing false-positive errors to someone's workflow, especially if we don't understand properly how the advanced features of this class are meant to be used.

With that in mind, given that we haven't received any user complaints regarding the stub for this class, I'd prefer we left it as it is for now. You could maybe change the TODO comment saying that it probably could be more accurately typed as a Generic[_P, _T] class, but that doing so doesn't really seem like it would give us much benefit :)

[kkirsche]

Reverted and removed via 8ee8ab4. Ultimately you all have more experience than me to know if there are problems, though the explanation hasn't convinced me that this is actually a risk.

[AlexWaygood]

though the explanation hasn't convinced me that this is actually a risk.

I think you're thinking about it the wrong way. Every change to the status quo in every code base is a risk of some kind, and you haven't convinced me that this change has tangible benefits to end users of type checkers :)

You need to ask "Why should we do this?", not "Why shouldn't we do this?", and the answer needs to be framed in terms of the benefits for end users rather than an abstract principle of "correctness".

[github-actions]

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

[kkirsche]

Closing — we can agree to disagree on this. The point about risk is exaggerated, not all changes actually impose some risk in practice, to make the point — but your point taken and discussing this isn't productive to continue.

[JelleZijlstra]

Sorry I missed this PR at the time, but I'm not sure I agree with the idea that we shouldn't make this change if we can't come up with an idiomatic usage that would be improved. To me, reducing Anys and making the stub more precise is itself a worthwhile goal. The stub from before 8ee8ab4 looks correct to me.

[AlexWaygood]

To me, reducing Anys and making the stub more precise is itself a worthwhile goal.

I agree with this sentiment. My concern was the fact that I didn't feel like I understood the way this class was meant to be used, and the fact that there are still some pretty serious known bugs in mypy's implementation of classes generic over ParamSpecs, leading to the potential for regressions (e.g. python/mypy#12011).

Anyway, I'm happy to back out if the consensus is against me and you'd like to merge this :)