ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Avoid flagging `__future__` annotations as required for non-evaluated type annotations

Open charliermarsh opened this issue 1 year ago • 11 comments

Summary

If an annotation won't be evaluated at runtime, we don't need to flag from __future__ import annotations as required. This applies both to quoted annotations and annotations outside of runtime-evaluated positions, like:

def main() -> None:
    a_list: list[str] | None = []
    a_list.append("hello")

Closes https://github.com/astral-sh/ruff/issues/11397.

charliermarsh avatar May 13 '24 17:05 charliermarsh

Hmm, I wouldn't do this for quoted annotations. I think it was definitely intentional in the original flake8 plugin for PEP-585 annotations in quotes to trigger this check, because otherwise UP037 won't automatically remove the quotes for you without the __future__ annotations import. I like the idea of ignoring annotations inside functions, though.

AlexWaygood avatar May 13 '24 17:05 AlexWaygood

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -3 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

zulip/zulip (+0 -3 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

- corporate/lib/stripe.py:3017:29: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
- corporate/lib/stripe.py:3115:48: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
- corporate/lib/stripe.py:915:40: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FA102 3 0 3 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -3 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

zulip/zulip (+0 -3 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- corporate/lib/stripe.py:3017:29: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
- corporate/lib/stripe.py:3115:48: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
- corporate/lib/stripe.py:915:40: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FA102 3 0 3 0 0

github-actions[bot] avatar May 13 '24 17:05 github-actions[bot]

The diagnostic and documentation aren't really correct or consistent with that use-case though, at least as-written. The messaging suggests you need this to avoid runtime errors. Almost feels like it needs an explicit message (e.g., you could remove quotes by adding this), but in that case, isn't it always true that adding from __future__ import annotations would let you remove quotes? Regardless of the syntax you're using?

charliermarsh avatar May 13 '24 17:05 charliermarsh

I'm now wondering if that should be a separate rule or part of FA100? Since the type can be written more succinctly if you import from __future__ import annotations, at the very least by removing the quotes?

charliermarsh avatar May 13 '24 17:05 charliermarsh

The messaging suggests you need this to avoid runtime errors.

Huh, that's not how I've ever interpreted that message. The message is "Missing from __future__ import annotations, but uses PEP 585 collection" -- I always interpreted it as "Missing from __future__ import annotations, but uses PEP 585 collection [which could be written more simply if you added this `future import]".

The original flake8 plugin explicitly mentions pyupgrade in its README as one of the motivations for the plugin: https://github.com/TylerYep/flake8-future-annotations?tab=readme-ov-file#flake8-future-annotations.

And I don't think this rule is nearly adequate to prevent all runtime errors arising from using newer typing syntax on older versions, nor should it try to do that.

isn't it always true that adding from __future__ import annotations would let you remove quotes? Regardless of the syntax you're using?

Not necessarily... eg. removing quotes from this annotation would cause a syntax error:

def foo() -> "Some kind of string":
    pass

AlexWaygood avatar May 13 '24 18:05 AlexWaygood

I'm now wondering if that should be a separate rule or part of FA100? Since the type can be written more succinctly if you import from __future__ import annotations, at the very least by removing the quotes?

IDK, because I've always thought of "enabling other tools to auto-upgrade your annotations" as being the primary motivation of the rule 😄

AlexWaygood avatar May 13 '24 18:05 AlexWaygood

Are you mixing up FA100 and FA102? Or am I mixing them up? FA100 is the rule that tells you "you can write this more succinctly if you import from __future__ import annotations." This PR does not modify FA100.

charliermarsh avatar May 13 '24 18:05 charliermarsh

This rule modifies FA102, which tells you that you're using an unsupported type annotation that will work if you add from __future__ import annotations.

charliermarsh avatar May 13 '24 18:05 charliermarsh

Argh, I should have checked more thoroughly. Yikes, and sorry :(

Yes, I didn't realise there was a distinction between FA100 and FA102. In that case, we should indeed make this change, as long as we still emit FA100 on all these snippets. (But maybe we should also update FA100 so that it's not emitted for annotations inside functions, as well, since the __future__ import is irrelevant in that context?)

It would be good if the rule summary at https://docs.astral.sh/ruff/rules/#flake8-future-annotations-fa made the distinction between the rules here a bit clearer.

AlexWaygood avatar May 13 '24 18:05 AlexWaygood

No need to apologize, I was also unsure. My guess is that we don't emit FA100 on the quoted ones. I'll have to check...

charliermarsh avatar May 13 '24 18:05 charliermarsh

LGTM! Thank you for such swift reply

inoa-jboliveira avatar May 13 '24 18:05 inoa-jboliveira

Confirmed that we still emit FA100 on these.

charliermarsh avatar May 21 '24 18:05 charliermarsh

Confirmed that we still emit FA100 on these.

TYVM!

AlexWaygood avatar May 21 '24 19:05 AlexWaygood

If an annotation won't be evaluated at runtime, we don't need to flag from future import annotations as required.

@charliermarsh This is only accurate in Python versions 3.10+ I believe:

Python 3.8.16 (default, Dec 20 2022, 18:52:29) 
[Clang 14.0.3 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> x: list[int]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'type' object is not subscriptable
>>> from __future__ import annotations
>>> x: list[int]
>>> 

Python 3.10.13 (main, Aug 24 2023, 22:36:46) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> x: list[int]
>>>

TylerYep avatar May 23 '24 21:05 TylerYep

But quoted ones will work regardless of the Python version:

Python 3.8.16 (default, Dec 20 2022, 18:52:29) 
[Clang 14.0.3 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> x: "list[int]"
>>> 

I didn't read the PR thoroughly, but wanted to make sure the lint rule is only changed for the quoted version but still flags non-quoted lowercase types without future annotations?

TylerYep avatar May 23 '24 21:05 TylerYep

@TylerYep - the logic here only applies to annotated assignments within function bodies, which I believe works on all Python versions:

def f():
    x: list[int] = 1

f()

charliermarsh avatar May 23 '24 21:05 charliermarsh

Ah, makes sense, thank you for clarifying!

TylerYep avatar May 23 '24 21:05 TylerYep