python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.17k stars 2.77k forks source link

Mypy shoould not suggest Foo[...] if annotation contains Foo(arg=...) etc. #16506

Open gordiig opened 9 months ago

gordiig commented 9 months ago

Hi! I've been developing something kinda like dependency injection mechanism and for it I've decided to create functions that return typing.Annotated with needed data and annotate di function arguments with this function. Something like that:

Provider-providable data code:

class _EnvironmentVariables(IProvidableData, Mapping):
    """
    Class storing environment variables from dump file.
    Should be used as Mapping
    """

    def __init__(self, variables: dict[str, bytes]) -> None:
        self.__variables = variables

    def as_json_obj(self) -> JSON:
        return {k: v.decode('ascii') for k, v in self.__variables.items()}

    def __getitem__(self, __key: str) -> bytes:
        return self.__variables.__getitem__(__key)

    def __len__(self) -> int:
        return self.__variables.__len__()

    def __iter__(self) -> Iterator[str]:
        return self.__variables.__iter__()

    def __str__(self):
        return str(self.__variables)

class EnvironmentVariablesProvider(ILinuxProvider[_EnvironmentVariables]):
    """
    Provides environment variables from Linux dump file
    """
    def __init__(self, coredump: Coredump, sort: bool) -> None:
        super().__init__(coredump=coredump)
        self.sort = sort

    def provide(self) -> _EnvironmentVariables:
        env_names = self.coredump.env.keys()
        if self.sort:
            env_names = sorted(env_names)

        envs = {env_name: self.coredump.getenv(env_name) for env_name in env_names}
        return _EnvironmentVariables(variables=envs)

def EnvironmentVariables(sort: bool):  # noqa (This func is imitating a type name, so upper-camel-case is ok)
    """
    Gets all environment variables from the core dump.

    These environment variables were captured by the process that generated core dump.

    Args:
        sort: Whether to sort variables by name or not

    Returns:
        Mapping with env name as a key and env value as a mapping value.

    Notes:
        This docstring is for user, so it is not covering or describing real implementation details!
    """
    return Annotated[_EnvironmentVariables, ArgsKwargs(sort=sort)]

End user code

@scenario(name='Unsorted env variables')  # Decorator for DI
@linux  # DI filter, doesn't matter in this example
def unsorted_env_variables(variables: EnvironmentVariables(sort=False)) -> JSON:
    return variables.as_json_obj()

The issue

The issue I have is that mypy is giving me error like:

error: Invalid type comment or annotation  [valid-type]
note: Suggestion: use EnvironmentVariables[...] instead of EnvironmentVariables(...)

I understand that static in "static code analysis" means that code won't run during this process and there is no (or almost no) way for mypy to see that EnvironmentVariables() returns typing.Annotated. But I think that mypy should be capable of differentiating function name from type name in type annotation.

This style of annotations might be kinda unintuitive, but I think that typing.Annotated is a mechanism that might be used dynamically by some users. And in this cases it is can be very handy to annotate argumens like that.

Environment:

files = "src/,test/"

Stop errors for pytest_lazy_fixtures about untyped import

[[tool.mypy.overrides]] module = "pytest_lazy_fixtures" ignore_missing_imports = true

JelleZijlstra commented 9 months ago

I'm not really sure what you'd expect to happen here. You used an invalid annotation, and mypy gave an error that indicated as much.

gordiig commented 9 months ago

You used an invalid annotation, and mypy gave an error that indicated as much.

Why is this annotation invalid?

I'm not really sure what you'd expect to happen here.

At least not giving me a hint that will lead to syntax error

erictraut commented 9 months ago

Why is this annotation invalid?

Call expressions are not allowed in type annotations, so EnvironmentVariables(sort=False) will be flagged as an error by mypy, pyright, and other static type checkers. You're using dynamic expressions in places where static type expressions are required. This approach won't work if you want your code to work with static type checkers.

gordiig commented 9 months ago

Sounds fair, but problem with hint that leads to SyntaxError is still there

mingminggl commented 9 months ago

@gordiig Is this problem specific to exclusively to functions named "EnvironmentVariables(sort: bool)", or is this something that happens to all functions regardless of what you name it? For example, if I name it "Foo(sort: bool)" will mypy still flag it as a syntax error.

gordiig commented 9 months ago

@mingminggl This error does not depend on function names

mingminggl commented 9 months ago

@gordiig thanks for clarifying, do you have any suggestions on what the new suggestion message for this should be?

mingminggl commented 9 months ago

Here is the progress we made with the issue. In mypy/mypy/fastparse.py, we made the following changes to visit_Call function:

        # Parse the arg constructor
        f = e.func
        constructor = stringify_name(f)

        if isinstance(self.parent(), ast3.FunctionDef):
            note = "Suggestion: Don't use function calls in type annotations"
            return self.invalid_type(e, note=note)
        if not isinstance(self.parent(), ast3.List):
            note = None
            if constructor:
                note = "Suggestion: use {0}[...] instead of {0}(...)".format(constructor)
            return self.invalid_type(e, note=note)
        if not constructor:
            self.fail(message_registry.ARG_CONSTRUCTOR_NAME_EXPECTED, e.lineno, e.col_offset)

In this change, we assumed that self.parent would give us the Type of EnvironmentVariables but that is not the case.

The issue we are facing is we can't determine the type of _EnvironmentVariables inside of the Annotated function call

from __future__ import annotations
from typing import Annotated

class _EnvironmentVariables():
    def __init__(self, variables: dict[str, bytes]) -> None:
        self.__variables = variables

def EnvironmentVariables(sort: bool):  # noqa (This func is imitating a type name, so upper-camel-case is ok)
    return Annotated[_EnvironmentVariables, dict]

def unsorted_env_variables(variables: EnvironmentVariables(sort=False)) -> None:
    return variables.as_json_obj()
gordiig commented 9 months ago

Message "Suggestion: Don't use function calls in type annotations" sounds good to me.

As a food for thought. Mypy will throw an error if function will return Annotated[SomeType, ...], but annotation is -> SomeType. Maybe Annotated[T, ...] and type[T] should be the same in mypy's view (or with some option enabled like allow_annotated_as_type).

Or less radical thought: mypy could treat functions that return types as a valid annotation. With such change mypy could statically determine that function is returning type, so, if it is called in the annotation, it is the same as if user would've typed type name itself.

def ret_int() -> type[int]:
   return int

def foo(x: ret_int()) -> None: ...

Now, mypy returns this message:

main.py:4: error: Invalid type comment or annotation  [valid-type]                                                                                                                                                                                  
main.py:4: note: Suggestion: use ret_int[...] instead of ret_int(...)
Found 1 error in 1 file (checked 1 source file)

Now I'm thinking that 2nd thought is more radical...

keshavt3 commented 5 months ago

I want to work on this.