Fix CloudPathMeta.__call__ return type

[ringohoffman]

Since pyright==1.1.307, instances of the subclasses of CloudPath stopped being recognized as instances of their own class; a possible fix is to type hint CloudPathMeta.__call__, but it creates a host of mypy errors (5)...

Here is the typing bug starting in the newest version of pyright and the fix with this proposed change:

import cloudpathlib


# before
x = cloudpathlib.S3Path("s3://bucket/key")
# reveal_type(x)  # Pylance: Type of "x" is "CloudPath | Unknown | Self@__new__"
y = cloudpathlib.CloudPath(x)
# reveal_type(y)  # Pylance: Type of "y" is "CloudPath | Unknown | Self@__new__"

# after
x = cloudpathlib.S3Path("s3://bucket/key")
reveal_type(x)  # Pylance: Type of "x" is "S3Path"
y = cloudpathlib.CloudPath(x)
reveal_type(y)  # Pylance: Type of "y" is "S3Path"

These are the mypy bugs that arise:

$ mypy cloudpathlib
cloudpathlib/cloudpath.py:127: error: Too many arguments for "__new__" of "object"  [call-arg]
cloudpathlib/cloudpath.py:129: error: Value of type variable "Self" of "__init__" of "CloudPath" cannot be "object"  [type-var]
cloudpathlib/cloudpath.py:130: error: Incompatible return value type (got "CloudPath", expected "CloudPathT")  [return-value]
cloudpathlib/cloudpath.py:141: error: Argument 1 to "__new__" of "ABCMeta" has incompatible type "CloudPathMeta"; expected "Type[<nothing>]"  [arg-type]
cloudpathlib/cloudpath.py:141: error: Argument 2 to "__new__" of "ABCMeta" has incompatible type "Union[str, CloudPathT]"; expected "str"  [arg-type]

I'm really not sure how to begin to solve these... it seems like they were there to begin with but weren't being raised (due to the lack of typing present on the signature, I assume)... I could definitely use some help in solving these. Would we consider type: ignore?

[ringohoffman]

@jayqi @pjbull

[pjbull]

@ringohoffman Thanks for looking in to this. Are these new problems because of #327? Do things work as expected on an earlier release? Looking at that PR may help with the mypy issues you are seeing.

[ringohoffman]

No, the errors are there for pyright==1.1.307 even on the commit immediately before #327 (a0dd927).

[codecov]

Codecov Report

Merging #330 (af7c3d7) into master (28b2eb5) will decrease coverage by 0.4%.
The diff coverage is 84.2%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #330     +/-   ##
========================================
- Coverage    94.3%   93.9%   -0.4%     
========================================
  Files          22      22             
  Lines        1466    1473      +7     
========================================
+ Hits         1383    1384      +1     
- Misses         83      89      +6     

Impacted Files	Coverage Δ
cloudpathlib/cloudpath.py	92.4% <84.2%> (-0.5%)	⬇️

... and 2 files with indirect coverage changes

[ringohoffman]

@pjbull let me know how this works for you, it seems to give us the better behavior than we saw before:

import cloudpathlib


# pyright 1.1.306 (master)
x = cloudpathlib.S3Path("s3://bucket/key")
# reveal_type(x)  # pyright:Type of "x" is "S3Path"
y = cloudpathlib.CloudPath(x)
# reveal_type(y)  # pyright: Type of "y" is "CloudPath"
z = cloudpathlib.CloudPath("s3://bucket/key")
# reveal_type(z)  # pyright: Type of "z" is "CloudPath"

# pyright 1.1.307 (master)
x = cloudpathlib.S3Path("s3://bucket/key")
# reveal_type(x)  # pyright: Type of "x" is "CloudPath | Unknown | Self@__new__"
# reveal_type(x)  # mypy: Revealed type is "cloudpathlib.s3.s3path.S3Path"
y = cloudpathlib.CloudPath(x)
# reveal_type(y)  # pyright: Type of "y" is "CloudPath | Unknown | Self@__new__"
# reveal_type(y)  # mypy: Revealed type is "cloudpathlib.cloudpath.CloudPath"
z = cloudpathlib.CloudPath("s3://bucket/key")
# reveal_type(z)  # pyright: Type of "z" is "CloudPath | Unknown | Self@__new__"
# reveal_type(z)  # mypy: Revealed type is "cloudpathlib.cloudpath.CloudPath"

# pyright 1.1.307 (this PR)
x = cloudpathlib.S3Path("s3://bucket/key")
# reveal_type(x)  # pyright: Type of "x" is "S3Path"
# reveal_type(x)  # mypy: Revealed type is "cloudpathlib.s3.s3path.S3Path"
y = cloudpathlib.CloudPath(x)
# reveal_type(y)  # pyright: Type of "y" is "S3Path"
# reveal_type(y)  # mypy: Revealed type is "cloudpathlib.cloudpath.CloudPath"
z = cloudpathlib.CloudPath("s3://bucket/key")
# reveal_type(z)  # pyright: Type of "z" is "CloudPath"
# reveal_type(z)  # mypy: Revealed type is "cloudpathlib.cloudpath.CloudPath"

It's probably worth pointing out that mypy throws errors in all cases for y and z:

Cannot instantiate abstract class "CloudPath" with abstract attributes "drive", "is_dir", "is_file", "mkdir" and "touch"  [abstract]mypy(error)

I do feel like this pattern is pretty abnormal and I wonder if we couldn't employ a more standard design that doesn't involve this custom metaclass. Alternate constructor classmethods might be a slightly more standard and better supported approach: https://stackoverflow.com/a/141777/7059681. We could also write a decorator to copy the docstrings of pathlib methods as well.

[pjbull]

Thanks @ringohoffman! This change looks good except removing one assert. When that is done, I'll bring it over to a local branch to run the cloud tests.

#328 is tracking the mypy issue as you note, which seems like an actual bug with mypy: python/mypy#14122 🤞 for an upstream fix there.

I do feel like this pattern is pretty abnormal and I wonder if we couldn't employ a more standard design that doesn't involve this custom metaclass. Alternate constructor classmethods might be a slightly more standard and better supported approach: https://stackoverflow.com/a/141777/7059681. We could also write a decorator to copy the docstrings of pathlib methods as well.

May be worth opening an issue to discuss this, but my general opinion is that we shouldn't sacrifice what feels like a ergonomic API design because of type hinting. It's nice to have CloudPath(p) dispatch to the right subclass no matter the value of p, and I wouldn't be in favor of changing it just because the type hinting can go wonky. There's no implementation in which a static type checker could ever know the dispatched class at statically. If people are only dealing with one provider, I would recommend they use the provider-specific class (e.g., S3Path) if they want type checking guarantees. Otherwise, best case scenario is they get the generic CloudPath hints.

This use case is the reason that metaclasses exist as a language feature, so I don't think it is really abnormal—just a tension between features for a dynamic language and static type hinting. I'm not sure there's another implementation that let's us have our cake and eat it too, but open to discussing options if there are.

[ringohoffman]

@pjbull I went ahead and applied your suggestion. I also found this example of metaclass hinting that gives the same behavior as before without any needing any type: ignore[return-value] comments: https://mypy.readthedocs.io/en/stable/metaclasses.html#metaclass-usage-example

I think the latest way is the best way to hint this method.

[ringohoffman]

@pjbull any thoughts about this latest change?