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

_TSource in Option should be covariant #171

Closed
denghz opened this issue Aug 25, 2023 · 8 comments · Fixed by #190
Closed

_TSource in Option should be covariant #171

denghz opened this issue Aug 25, 2023 · 8 comments · Fixed by #190

Comments

@denghz
Copy link
Contributor

denghz commented Aug 25, 2023

Describe the bug
_TSource type in Option should be covariant.

class A:
  pass
class B(A):
  pass

x: Option[A] = Some(B())

Now since the _TSource is invariant, pyright will complain that B is not the same as A, but actually Option type should be covariant. Is Some[Dog] a Some[Animal]? I think you'll agree that the answer is yes. Also Option type is not actually mutable, so there is no way to assign a Some[Dog] to a variable typed Some[Aninmal] and then make it become Some[Cat]

Additional context
Add any other context about the problem here.

  • OS Linux
  • Expression version 4.2.4
  • Python version 3.11
@brendanmaguire
Copy link
Contributor

brendanmaguire commented Sep 15, 2023

I didn't see the above failing the type check in either mypy or pyright but the following did fail:

from expression import Some, Option

class A:
    pass

class B(A):
    pass

def test_covariance() -> None:
    x: Option[B] = Some(B())
    y: Option[A] = x

@brendanmaguire
Copy link
Contributor

This isn't so easy to fix because covariant types can't be passed as parameters to other functions.

@phi-friday
Copy link
Contributor

It also looks good to use infer_variance, which eliminates the need to consider covariant.
ex:

from typing_extensions import TypeVar

_T = TypeVar("_T", infer_variance=True)

This parameter is supported by typing-extensions>=4.4.0 and is the default behavior in python>=3.12.

@dbrattli
Copy link
Owner

dbrattli commented Nov 9, 2023

This looks really interesting. Will try. Look forward to when we can eventually be >= Python 3.12 😀

@brendanmaguire
Copy link
Contributor

My understanding of infer_variance is that it only aids in not having to specify the variance of the TypeVar. See the related spec docs:

type checker should use inference to determine whether the type variable is invariant, covariant or contravariant

So I don't think it helps here. The problem still exists that covariant types can't be passed as parameters to other functions.

@brendanmaguire
Copy link
Contributor

I locally updated typing-extensions to 4.8.0 and tried with infer_variance. My above code example still fails the type checker.

@dbrattli
Copy link
Owner

dbrattli commented Jan 15, 2024

I've tried to make some progress on this one by making methods that takes _TSource static methods and use another type when generating e.g:

    @staticmethod
    def Some(value: _T1) -> Option[_T1]:
        """Create a Some option."""
        return Option(some=value)

Then for some more tricky methods like default_value I return a union with the default arg:

def default_value(self, value: _T1) -> _TSource | _T1:    

I'm just unsure if the rest is ok, or false negatives. E.g:

def default_with(self, getter: Callable[[], _TSource]) -> _TSource:

It type checks, but is it ok to use _TSource in an argument that is a callable that returns _TSource? I could do the same trick as with default_value but then there's all the other methods like map, bind, but they are probably ok since they return a differnet type in the callable?

@ShravanSunder
Copy link

I locally updated typing-extensions to 4.8.0 and tried with infer_variance. My above code example still fails the type checker.

From what i understand, I don't think we can have input paramters in python type hints as covariant. i think they usually are contravariant while returns are covariant. The type hints don't seem to let you know this and just has a generic error.

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 a pull request may close this issue.

5 participants