[flake8-future-annotations] Add optional integration with TC rules (FA100)
#18919
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| FA100 | 10 | 5 | 5 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+5 -5 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)
apache/airflow (+5 -5 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL
+ airflow-core/src/airflow/api_fastapi/execution_api/deps.py:58:26: FA100 Add `from __future__ import annotations` to simplify `Optional` - airflow-core/src/airflow/api_fastapi/execution_api/deps.py:58:26: FA100 Add `from __future__ import annotations` to simplify `typing.Optional` + airflow-core/src/airflow/api_fastapi/execution_api/deps.py:59:26: FA100 Add `from __future__ import annotations` to simplify `Optional` - airflow-core/src/airflow/api_fastapi/execution_api/deps.py:59:26: FA100 Add `from __future__ import annotations` to simplify `typing.Optional` + airflow-core/src/airflow/api_fastapi/execution_api/deps.py:69:10: FA100 Add `from __future__ import annotations` to simplify `Optional` - airflow-core/src/airflow/api_fastapi/execution_api/deps.py:69:10: FA100 Add `from __future__ import annotations` to simplify `typing.Optional` + providers/standard/tests/unit/standard/decorators/test_python.py:134:21: FA100 Add `from __future__ import annotations` to simplify `Union` - providers/standard/tests/unit/standard/decorators/test_python.py:134:21: FA100 Add `from __future__ import annotations` to simplify `typing.Union` + providers/standard/tests/unit/standard/decorators/test_python.py:964:23: FA100 Add `from __future__ import annotations` to simplify `Union` - providers/standard/tests/unit/standard/decorators/test_python.py:964:23: FA100 Add `from __future__ import annotations` to simplify `typing.Union`
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| FA100 | 10 | 5 | 5 | 0 | 0 |
ntBre
added
configuration
Can you tell me more why you opted for a configuration option over a new rule? Edit: Maybe something to answer on the issue. I think there are at least 3 possible designs and I find it hard to think through if the approach chosen here is the most ideal without looking at the problem holsiticly: Some rules change their behavior based on a future import. |
| if !reference.in_typing_only_annotation() { | ||
| return IsTypingReference::Maybe; | ||
| } |
There was a problem hiding this comment.
You might want to check whether there is a future import, if there is, the answer here is definitely No and not Maybe. The same goes for if the target Version is 3.14 or above.
I also don't quite recall how Ruff treats PEP 695 type parameter bounds or PEP 696 type parameter defaults. They probably could relatively safely be treated as typing only references, but I'm not sure that's currently how they are treated. A similar, although weaker, argument could be made for type statements. See #16981 for further discussions.
Treating them as Maybe seems correct on the surface, but Maybe here seems to mean that the decisions will (or at least likely will) change based on a future annotation or the target python version, which is not the case for type statements or type parameter bounds/defaults. Maybe a different name would be more appropriate? Do we want a fourth value?
Also #14787 is relevant to some degree here as well. I'm also a little worried about the interaction with the quote_annotations setting, since quoting annotations is antithetical to adding a future import.
There was a problem hiding this comment.
Thanks for taking a look!
You might want to check whether there is a future import, if there is, the answer here is definitely
Noand notMaybe. The same goes for if the target Version is 3.14 or above.
I think if there's already a future import or the version is 3.14+ we will have already returned Yes a few lines above, right? That's what I was thinking while writing this at least. This logic does feel tricky, so I could certainly be wrong here.
I also don't quite recall how Ruff treats PEP 695 type parameter bounds or PEP 696 type parameter defaults. They probably could relatively safely be treated as typing only references, but I'm not sure that's currently how they are treated. A similar, although weaker, argument could be made for
typestatements. See #16981 for further discussions.Treating them as
Maybeseems correct on the surface, butMaybehere seems to mean that the decisions will (or at least likely will) change based on a future annotation or the target python version, which is not the case for type statements or type parameter bounds/defaults. Maybe a different name would be more appropriate? Do we want a fourth value?
Hmm, I think I see what you mean. This might just not be the best place to perform this check. The integration with some other rules looked more repetitive, but if we just re-checked the condition that was here originally (or modified slightly to fit our needs), we could avoid trying to overload the yes/no/maybe meaning.
Also #14787 is relevant to some degree here as well. I'm also a little worried about the interaction with the
quote_annotationssetting, since quoting annotations is antithetical to adding a future import.
Related to my first point, I think we will have already returned Yes if we're in a string annotation, but, again, I could be wrong. It sounds like I should add more test cases anyway to be sure.
There was a problem hiding this comment.
I think if there's already a future import or the version is 3.14+ we will have already returned Yes a few lines above, right? That's what I was thinking while writing this at least. This logic does feel tricky, so I could certainly be wrong here.
You're probably correct for annotations, for some reason I thought in_typing_only_annotation() was less reliable than it actually is. But that's probably only because I personally would like it to apply to type statements and type parameters as well, but currently it does not.
There was a problem hiding this comment.
But either way, this function gets called for a lot more type definitions than annotations, so Maybe is too generous for generic subscripts, type aliases, casts, type parameters bounds/defaults. So you should definitely make sure the reference stems from an annotation (i.e. in_runtime_evaluated_annotation() for Maybe rather than !in_typing_only_annotation(), it's worth noting here that runtime required annotations also can't be re-written with modern typing syntax).
this way we'll inherit the autofix for free
|
(no changes, just rebasing onto the autofix) |
|
I'll put this back into draft while we discuss the design on the issue. |
Summary
This PR covers the next step in #18502 after adding an autofix in #18903: checking for opportunities to emit FA100 that would cause TC001, TC002, and TC003 to activate. The main changes here are:
lint.flake8-future-annotations.aggressiveconfig option to enable this behavior (not sold on the name, so this is basically a placeholder)is_typing_referenceto help distinguish the cases where applying FA100 would change the applicability of the TC rulesfuture_rewritable_type_annotationimplementation to accept anythingRangedand customize the error messageTest Plan
New tests with both FA100 and the TC rules active.
I opted only to emit FA100 in these cases, but we should double check that applying its autofix then also triggers the TC rules after #18903 lands, maybe with a CLI test.
I also did some manual testing for duplicates with the setup below. Everything seems okay when selecting combinations of the FA and TC rules, even when backporting the fix from #18903, but maybe someone else has a better idea for a test here.
Manual testing setup
Output: