Prevent redefined-outer-name for if t.TYPE_CHECKING

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #7524
Similar solution to #6787

[coveralls]

Pull Request Test Coverage Report for Build 3118393877

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 95.315%

---

Totals
Change from base Build 3115053445:	0.0%
Covered Lines:	17109
Relevant Lines:	17950

---

💛 - Coveralls

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on flask:
The following messages are no longer emitted:

1. redefined-outer-name:
   Redefining name 'Flask' from outer scope (line 25)
   https://github.com/pallets/flask/blob/0d8c8ba71bc6362e6ea9af08146dc97e1a0a8abc/src/flask/cli.py#L36
2. redefined-outer-name:
   Redefining name 'Flask' from outer scope (line 25)
   https://github.com/pallets/flask/blob/0d8c8ba71bc6362e6ea9af08146dc97e1a0a8abc/src/flask/cli.py#L115
3. redefined-outer-name:
   Redefining name 'Response' from outer scope (line 24)
   https://github.com/pallets/flask/blob/0d8c8ba71bc6362e6ea9af08146dc97e1a0a8abc/src/flask/helpers.py#L267

This comment was generated for commit c4410ea

[DanielNoord]

Not sure if this is in scope, but there is only one reference to TYPING_TYPE_CHECKS_GUARDS remaining. Should we remove it as well and use this function everywhere?

[jacobtylerwalls]

I think that should be a follow-up PR. At first glance I'm not sure there's a drop-in replacement.

[DanielNoord]

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index 65a9e8c8b..59e9f7fba 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -2011,10 +2011,7 @@ class VariablesChecker(BaseChecker):
             if isinstance(defstmt, (nodes.Import, nodes.ImportFrom)):
                 defstmt_parent = defstmt.parent
 
-                if (
-                    isinstance(defstmt_parent, nodes.If)
-                    and defstmt_parent.test.as_string() in TYPING_TYPE_CHECKS_GUARDS
-                ):
+                if in_type_checking_block(defstmt):
                     # Exempt those definitions that are used inside the type checking
                     # guard or that are defined in both type checking guard branches.
                     used_in_branch = defstmt_parent.parent_of(node)

Passed all functional tests for me locally.

[jacobtylerwalls]

I'm concerned there aren't enough tests. This example depends on defstmt_parent being nodes.If. in_type_checking_block is more robust, and doesn't assume direct parentage by a nodes.If. There's more control flow in this example; it needs more thought.

[DanielNoord]

Ah yeah, my bad. What about:

if isinstance(defstmt_parent, nodes.If) and in_type_checking_block(defstmt):

The in_type_checking_block doesn't do any control flow analysis and only makes sure we infer instead of relying on the string representation. As far as I see it is a drop-in replacement for defstmt_parent.test.as_string() in TYPING_TYPE_CHECKS_GUARDS. (But not for isinstance(defstmt_parent, nodes.If) as you correctly spotted!)

[jacobtylerwalls]

I still don't think it's a drop-in replacement because in_type_checking_block() is recursive. We don't know which if ancestor met the condition, but the way this block is written, the assumption is that defstmt_parent is the one that met the condition, and doing a drop-in replacement will violate that condition. Leading me to think it needs more refactoring and a follow-up issue.

[DanielNoord]

Fair enough. Shall we open an issue/todo for this? I'm also fine with leaving as is.

[jacobtylerwalls]

No problem, I'll do it. 👍🏻

[jacobtylerwalls]

#7539

[Pierre-Sassoulas]

LGTM, I'll let you merge when the remaining discussion is resolved.