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

PEP 585 support in 3.7+ using __future__ #66

Closed
TylerYep opened this issue Mar 4, 2021 · 10 comments
Closed

PEP 585 support in 3.7+ using __future__ #66

TylerYep opened this issue Mar 4, 2021 · 10 comments

Comments

@TylerYep
Copy link

TylerYep commented Mar 4, 2021

Describe the bug
Vermin reports the minimum version is Python 3.9, even though the code will run correctly in Python 3.7+.

To Reproduce
Steps to reproduce the behavior.

Run the following code in Python 3.7, 3.8, 3.9

from __future__ import annotations

def fn(x: list[int]) -> list[int]:
    a: list[int] = []
    return a

fn([])

Output:

Detecting python files..
Analyzing using 12 processes..
!2, 3.9      /Users/tyler.yep/Documents/Github/probs/yo.py
  L1 C5: '__future__' module requires 2.1, 3.0
  L1 C5: '__future__.annotations' member requires !2, 3.7
  L4: annotations requires !2, 3.0
  L4: builtin generic type annotation (list[..]) requires !2, 3.9
  L5: variable annotations requires !2, 3.6
  L5: builtin generic type annotation (list[..]) requires !2, 3.9
  L6: builtin generic type annotation (list[..]) requires !2, 3.9

Minimum required versions: 3.9
Incompatible versions:     2

Expected behavior
Minimum version should be Python 3.7

Environment:

  • Vermin latest version (1.1.0)
@gousaiyang
Copy link
Contributor

I also discovered this earlier but didn't have time to organize it and create an issue :) Just want to point out some important points to consider for implementation:

  • PEP 563 (from __future__ import annotations) makes function and variable annotations stored in string form. They are not evaluated at definition time, but only evaluated when you manually call typing.get_type_hints or eval(obj.__annotations__). The following code snippet:
from __future__ import annotations

def foo() -> list[int]:
    return [0]

foo()

is 3.7+, but this one:

from __future__ import annotations
import typing

def foo() -> list[int]:
    return [0]

typing.get_type_hints(foo)

is 3.9+. Probably there could be a command line option for users to inform vermin whether the annotations in the code will be manually evaluated or not.

  • Local variable annotations are never evaluated or stored anywhere. So this:
def foo():
    x: list[int]

is 3.6+ (since variable annotations are 3.6+), even if from __future__ import annotations is not present.

  • Even if PEP 563 is enabled, annotations should still be syntactically valid Python expressions. New syntax used should still indicate a higher minimum version. For example,
from __future__ import annotations
x: list[int]  # PEP 585, 3.9+
y: int | str  # PEP 604, 3.10+

is 3.7+, since list[int] and int | str are valid syntax in Python 3.7. While this one:

from __future__ import annotations
def foo():
    x: (a := 1)

is still 3.8+.

  • PEP 563 is mandatory in Python 3.10. Let's assume that int | str were a 3.11 feature (instead of 3.10 in reality). The following code
x: int | str

will still be 3.10+ since the PEP 563 is always enabled (without having to write from __future__ import annotations) in 3.10 and int | str is not new syntax.

@netromdk
Copy link
Owner

Thanks for the feedback, @TylerYep and @gousaiyang! I will try to look into this soon.

@gousaiyang
Copy link
Contributor

Just want to add one more case:

When the target being annotated is not a simple name (e.g. x[0]: list[int]), the annotation is evaluated if it is in a class or module scope (not a function scope), but it will not be stored. When PEP 563 is enabled, it will not be evaluated and cannot be manually evaluated (because it is not stored anywhere).

@netromdk
Copy link
Owner

netromdk commented Apr 17, 2021

@gousaiyang I've added commit e231317 to branch issue-66 which introduces argument --eval-annotations and --no-eval-annotations (plus config), which now defaults to not evaluating those annotations mentioned above. Is this what you meant?

(I will have a look at 3.10 stuff later btw)

@gousaiyang
Copy link
Contributor

gousaiyang commented Apr 17, 2021

This is a good start which allows users to choose whether to evaluate annotations or not, and I think this will work for most cases. However, if better accuracy is desired, the implementation should still consider the corner cases mentioned above. Unfortunately I could tell that will require much more work than your current --eval-annotations and --no-eval-annotations switches.

Example:

The following code:

from __future__ import annotations
import typing

x: typing.Final[int] = 1

is actually 3.7+. Evaluation of typing.Final[int] is suppressed by from __future__ import annotations. The code is equivalent to the following code (as if the annotations are "stringized") when compatibility for Python < 3.7 is desired:

import typing

x: 'typing.Final[int]' = 1

(This is 3.6+)

Your current implementation only suppressed builtin generic type annotations (list[int]), while I think it should be done at the AST level (suppress all children nodes which are function or variable annotations). When list[int] is not within an annotation, it is still evaluated at runtime and is thus 3.9+. What's more tricky is that we should still look into syntax-level features in annotation nodes, as x: (y := int) = 1 is still 3.8+, although few people may really use this...

Also if a file does not have from __future__ import annotations, it is actually either 3.10+, or its annotations will be eagerly evaluated. By using your current implementation, users will need to correctly choose one of --eval-annotations and --no-eval-annotations, while in fact it can be deduced by vermin itself. Only when PEP 563 is enabled (i.e. either the code has from __future__ import annotations or it's 3.10+) will the user really need to choose one of the two options.

In addition, Python annotations really have a lot of corner cases, and my previous claim about annotating complex targets (not a simple name) is inaccurate. It turns out that they are evaluated even when PEP 563 is enabled, and people are proposing to suppress this in 3.10. There's even more to mention about the evaluation of targets themselves. In self.name: str, self is evaluated, regardless of whether PEP 563 is enabled. More details are described in PEP 526 and documentation.

It's also good to skip evaluating code under if TYPE_CHECKING: if the code does not contain new syntax.

Implementing all of those will be a ton of work. But I think if the AST-level exclusion (ignore annotation nodes) is added to your current implementation, it should be useful enough for many people.

@netromdk
Copy link
Owner

Thanks for the great notes, @gousaiyang. Yeah that will be a lot of work and some of it might need to happen sooner or later, but right now I agree with your assessments. Annotations are kind of a pain.. Haha.

Added e32f92c. It's ignoring the nodes.

It's also good to skip evaluating code under if TYPE_CHECKING: if the code does not contain new syntax.

What exactly do you mean here?

@gousaiyang
Copy link
Contributor

It's also good to skip evaluating code under if TYPE_CHECKING: if the code does not contain new syntax.

What exactly do you mean here?

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing import Final

x: 'Final[int]' = 1

By using if TYPE_CHECKING:, this code will run well on 3.6 and pass a type checker on 3.8. Code under if TYPE_CHECKING: will not be executed at runtime, so we can skip analyzing them (except for syntax-level features). TYPE_CHECKING can be imported either from typing or typing_extensions.

@netromdk
Copy link
Owner

Thanks for the feedback. Pushed 99240ed with corrections.

I'm inclined not to add special code for if TYPE_CHECKING: because such conditional logic could also use while, boolean operators and other nested logic, which could lead to false negatives.

@gousaiyang
Copy link
Contributor

Pushed 99240ed with corrections.

I think that looks good.

I'm inclined not to add special code for if TYPE_CHECKING: because such conditional logic could also use while, boolean operators and other nested logic, which could lead to false negatives.

It's OK since users can always use # novermin to manually skip analyzing them.

netromdk added a commit that referenced this issue Apr 18, 2021
Ignore annotation AST nodes if not evaluating annotations
@netromdk
Copy link
Owner

Thanks again, both. Will release version 1.1.1 soon.

yan12125 pushed a commit to EMCLab-Sinica/DynBal that referenced this issue Sep 5, 2022
And use postponed evaluation of annotations [2] to avoid syntax errors
on Python 3.7 and 3.8. See [3] for why it works.

[1] https://peps.python.org/pep-0585/
[2] https://peps.python.org/pep-0563/
[3] netromdk/vermin#66 (comment)
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

No branches or pull requests

3 participants