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

Unexpected error with bool overload (Xarray) #6069

Closed
jenshnielsen opened this issue Oct 3, 2023 · 7 comments
Closed

Unexpected error with bool overload (Xarray) #6069

jenshnielsen opened this issue Oct 3, 2023 · 7 comments
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request

Comments

@jenshnielsen
Copy link

Describe the bug
The code below unexpectedly raises an Error

Argument of type "bool" cannot be assigned to parameter "compute" of type "Literal[False]" in function "to_netcdf"
  "bool" cannot be assigned to type "Literal[False]"

However as can be seen below the method accepts both True and False.

Code or Screenshots

import xarray as xr

def myfunc(compute: bool) -> None:
    ds = xr.Dataset()
    ds.to_netcdf(path="myfile.nc",
                compute=compute)

Where the definition of to_netcdf contains variants both for Literal[True] and Literal[False]

    # path=None writes to bytes
    @overload
    def to_netcdf(
        self,
        path: None = None,
        mode: Literal["w", "a"] = "w",
        format: T_NetcdfTypes | None = None,
        group: str | None = None,
        engine: T_NetcdfEngine | None = None,
        encoding: Mapping[Any, Mapping[str, Any]] | None = None,
        unlimited_dims: Iterable[Hashable] | None = None,
        compute: bool = True,
        invalid_netcdf: bool = False,
    ) -> bytes:
        ...

    # default return None
    @overload
    def to_netcdf(
        self,
        path: str | PathLike,
        mode: Literal["w", "a"] = "w",
        format: T_NetcdfTypes | None = None,
        group: str | None = None,
        engine: T_NetcdfEngine | None = None,
        encoding: Mapping[Any, Mapping[str, Any]] | None = None,
        unlimited_dims: Iterable[Hashable] | None = None,
        compute: Literal[True] = True,
        invalid_netcdf: bool = False,
    ) -> None:
        ...

    # compute=False returns dask.Delayed
    @overload
    def to_netcdf(
        self,
        path: str | PathLike,
        mode: Literal["w", "a"] = "w",
        format: T_NetcdfTypes | None = None,
        group: str | None = None,
        engine: T_NetcdfEngine | None = None,
        encoding: Mapping[Any, Mapping[str, Any]] | None = None,
        unlimited_dims: Iterable[Hashable] | None = None,
        *,
        compute: Literal[False],
        invalid_netcdf: bool = False,
    ) -> Delayed:
        ...

    def to_netcdf(
        self,
        path: str | PathLike | None = None,
        mode: Literal["w", "a"] = "w",
        format: T_NetcdfTypes | None = None,
        group: str | None = None,
        engine: T_NetcdfEngine | None = None,
        encoding: Mapping[Any, Mapping[str, Any]] | None = None,
        unlimited_dims: Iterable[Hashable] | None = None,
        compute: bool = True,
        invalid_netcdf: bool = False,
    ) -> bytes | Delayed | None:

https://github.com/pydata/xarray/blob/main/xarray/core/dataset.py#L2130

Xarray: 2023.9.0

VS Code extension or command-line
Both. Pyright 1.1.329 on the cmd line

@jenshnielsen jenshnielsen added the bug Something isn't working label Oct 3, 2023
@rsokl
Copy link

rsokl commented Oct 3, 2023

I believe this is due to the fact that the only overload for which compute is annotated with bool is one where path is annotated to be None. Because you specify a non-None-type for path and a bool-type for compute, pyright is telling you that there is no viable overload.

@erictraut
Copy link
Collaborator

This behavior is by design. When performing overload matching, pyright will expand unions of argument types, but it does not expand a bool into Literal[True] | Literal[False]. Refer to this documentation for a full description of pyright's overload matching behavior.

Unfortunately, overload matching behaviors are not specified in any PEP, but I've tried very hard to match the behavior of mypy so developers (and library authors) see consistent behaviors. Mypy likewise does not expand bool into Literal[True] | Literal[False] when performing overload matching. So it's unlikely that we'd implement this in pyright unless/until a specification for overload matching is written.

For reference, here's a self-contained minimal repro of the issue. As you can see, both pyright and mypy generate an error for this code.

from typing import Literal, overload

@overload
def func1(x: Literal[True]) -> int:
    ...

@overload
def func1(x: Literal[False]) -> str:
    ...

def func1(x: bool) -> int | str:
    return ""

def func2(x: bool):
    func1(x)

If you want your overloaded function to accept bool, you'll need to provide an explicit overload for that case.

@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2023
@erictraut erictraut added the as designed Not a bug, working as intended label Oct 3, 2023
@jenshnielsen
Copy link
Author

@erictraut Thanks however part of the problem is that the error message does not really convey this.
According to the docs that you linked:

If no overloads remain and all unions have been expanded, a diagnostic is generated indicating that the supplied arguments are incompatible with all overload signatures.

but the error message does not convey that none of the overloads match. Only that one specific overload does not.

C:\Users\jeniels\Desktop\pyrightxarray.py
  C:\Users\jenielse\Desktop\pyrightxarray.py:6:46 - error: Argument of type "bool" cannot be assigned to parameter "compute" of type "Literal[False]" in function "to_netcdf"
    "bool" cannot be assigned to type "Literal[False]" (reportGeneralTypeIssues)
1 error, 0 warnings, 0 informations

@erictraut
Copy link
Collaborator

Yes, reporting cogent messages for overload mismatches is very difficult. Older versions of pyright simply said "no overloads match", but users (rightfully) said that this error message wasn't very helpful. We've gone through many rounds of refinements since then. The latest version attempts to find the overload that is "closest" to what the user probably intended (using a heuristic) and then emits an error based on this signature. Very few users understand how complex overload matching actually is — and would definitely be confused by the notion of union expansion if the error message attempted to explain that. So the current error message is the best we've been able to come up with for this situation. If you have other specific suggestions, let us know.

@jenshnielsen
Copy link
Author

jenshnielsen commented Oct 3, 2023

I see, I will leave it up to you to judge what it the least confusing but personally I read that error message as if pyright (due to a bug) only considered the Literal[False] overload and did not at all evaluate the others.

Personally, I would find the error message clearer if it was prepended with something like.

Could not find matching overload for supplied signature. Closest match was ...

If this cannot be done realistically be done better, it would be nice if the docs were updated to reflect the current state.

erictraut pushed a commit that referenced this issue Oct 3, 2023
…o find any applicable overloads. This addresses #6069.
@erictraut
Copy link
Collaborator

OK, I like that idea. I've implemented a variant of what you suggested. Pyright now emits two diagnostics. The first indicates that "no overloads match the provided arguments" and points to the closest overload match that it could identify. It then evaluates the call expression using the closest match and emits any errors associated with that call evaluation.

image

This will be included in the next release. I'll be curious to see what the feedback is.

@erictraut erictraut added enhancement request New feature or request addressed in next version Issue is fixed and will appear in next published version and removed bug Something isn't working as designed Not a bug, working as intended labels Oct 3, 2023
@jenshnielsen
Copy link
Author

Great Thanks. I have in the mean time added a pr to xarray to add the bool overload as you suggested.

erictraut added a commit that referenced this issue Oct 3, 2023
…o find any applicable overloads. This addresses #6069. (#6076)

Co-authored-by: Eric Traut <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants