From ebe46db5772dc9989125bbdf4ef7d155994c7352 Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Fri, 28 Oct 2022 21:31:46 +0200 Subject: [PATCH 01/10] try working, except else --- mypy/partially_defined.py | 67 +++++++++- test-data/unit/check-partially-defined.test | 129 ++++++++++++++++++++ 2 files changed, 193 insertions(+), 3 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index 7d87315c23ad..ad966ec26a49 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -24,7 +24,7 @@ ReturnStmt, TupleExpr, WhileStmt, - WithStmt, + WithStmt, TryStmt, ) from mypy.patterns import AsPattern, StarredPattern from mypy.reachability import ALWAYS_TRUE, infer_pattern_value @@ -55,6 +55,9 @@ def __init__( self.must_be_defined = set(must_be_defined) self.skipped = skipped + def copy(self) -> BranchState: + return BranchState(must_be_defined=set(self.must_be_defined), may_be_defined=set(self.may_be_defined), skipped=self.skipped) + class BranchStatement: def __init__(self, initial_state: BranchState) -> None: @@ -66,6 +69,11 @@ def __init__(self, initial_state: BranchState) -> None: ) ] + def copy(self) -> BranchStatement: + result = BranchStatement(self.initial_state) + result.branches = [b.copy() for b in self.branches] + return result + def next_branch(self) -> None: self.branches.append( BranchState( @@ -116,13 +124,21 @@ def done(self) -> BranchState: may_be_defined = all_vars.difference(must_be_defined) return BranchState(may_be_defined=may_be_defined, must_be_defined=must_be_defined) - class DefinedVariableTracker: """DefinedVariableTracker manages the state and scope for the UndefinedVariablesVisitor.""" def __init__(self) -> None: # There's always at least one scope. Within each scope, there's at least one "global" BranchingStatement. self.scopes: list[list[BranchStatement]] = [[BranchStatement(BranchState())]] + # disable_branch_skip is used to disable skipping a branch due to a return/raise/etc. This is useful + # in things like try/except statements. + self.disable_branch_skip = False + + def copy(self) -> DefinedVariableTracker: + result = DefinedVariableTracker() + result.scopes = [s.copy() for s in self.scopes] + result.disable_branch_skip = self.disable_branch_skip + return result def _scope(self) -> list[BranchStatement]: assert len(self.scopes) > 0 @@ -150,7 +166,7 @@ def end_branch_statement(self) -> None: def skip_branch(self) -> None: # Only skip branch if we're outside of "root" branch statement. - if len(self._scope()) > 1: + if len(self._scope()) > 1 and not self.disable_branch_skip: self._scope()[-1].skip_branch() def record_declaration(self, name: str) -> None: @@ -286,6 +302,51 @@ def visit_expression_stmt(self, o: ExpressionStmt) -> None: self.tracker.skip_branch() super().visit_expression_stmt(o) + def visit_try_stmt(self, o: TryStmt) -> None: + self.process_try(o) + # old_tracker = self.tracker + # finally_tracker = self.tracker.copy() + # finally_tracker.disable_branch_skip = True + # self.tracker = finally_tracker + # self.process_try(o, process_jumps=False) + # self.tracker = old_tracker + + def process_try(self, o: TryStmt) -> None: + # todo(stas): this comment is no longer correct: + # This checker treats try statements as the following exclusive branches: + # - try body + # - except bodies + # - else body + # This is technically incorrect (because the try body may be executed as well as except or else bodies. + # For the purposes of this check, it's an accepted source of false positive errors. + # todo(stas): test when try except is inside an if with a raise -- that probably shouldn't skip any branches? + self.tracker.start_branch_statement() + self.tracker.disable_branch_skip = True + o.body.accept(self) + self.tracker.disable_branch_skip = False + for i in range(len(o.types)): + self.tracker.next_branch() + tp = o.types[i] + if tp is not None: + tp.accept(self) + o.handlers[i].accept(self) + for v in o.vars: + if v is not None: + v.accept(self) + if o.else_body is not None: + # We don't want `raise` or `return` inside try/except to prevent an undefined variable from registering. + # self.tracker.next_branch() + # self.tracker.disable_branch_skip = True + o.body.accept(self) + # self.tracker.disable_branch_skip = False + o.else_body.accept(self) + self.tracker.end_branch_statement() + # todo(stas): finally should be executed regardless of returns, raises, etc (i.e. we shouldn't skip branches + # for `finally`) + # todo(stas): test case for raise in except clause + if o.finally_body is not None: + o.finally_body.accept(self) + def visit_while_stmt(self, o: WhileStmt) -> None: o.expr.accept(self) self.tracker.start_branch_statement() diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index d456568c1131..04d1520ea64b 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -312,6 +312,135 @@ def f3() -> None: y = x z = x # E: Name "x" may be undefined +[case testTryBasic] +# flags: --enable-error-code partially-defined +def f1() -> int: + try: + x = 1 + except: + pass + return x # E: Name "x" may be undefined + +def f2() -> int: + try: + pass + except: + x = 1 + return x # E: Name "x" may be undefined + +def f3() -> int: + try: + x = 1 + except: + return 0 + return x + +def f4() -> int: + try: + x = 1 + except: + raise + return x + +def f5() -> None: + try: + pass + except BaseException as e: + pass + # This case is covered by the other check, not by partially defined check. + y = e # E: Trying to read deleted variable "e" + +def f6() -> int: + try: + x = 1 + assert False + except: + pass + # This is a false-positive. Technically, x is always defined but mypy isn't + # smart enough (yet) to detect this. + return x # E: Name "x" may be undefined +[builtins fixtures/exception.pyi] + +[case testTryMultiExcept] +# flags: --enable-error-code partially-defined +def f1() -> int: + try: + x = 1 + except BaseException: + x = 2 + except: + x = 3 + return x + +def f2() -> int: + try: + x = 1 + except BaseException: + pass + except: + x = 3 + return x # E: Name "x" may be undefined +[builtins fixtures/exception.pyi] + +[case testTryFinally] +# flags: --enable-error-code partially-defined +def f1() -> int: + try: + x = 1 + finally: + x = 2 + return x + +def f2() -> int: + try: + pass + except: + pass + finally: + x = 2 + return x + +def f3() -> int: + try: + x = 1 + except: + pass + finally: + y = x # E: Name "x" may be undefined + return x + +[case testTryElse] +# flags: --enable-error-code partially-defined +def f1() -> int: + try: + return 0 + except BaseException: + x = 1 + else: + x = 2 + finally: + y = x # E: Name "x" may be undefined + return y + +def f2() -> int: + try: + pass + except: + x = 1 + else: + x = 2 + return x + +def f3() -> int: + try: + pass + except: + x = 1 + else: + y = 1 + return x # E: Name "x" may be undefined +[builtins fixtures/exception.pyi] + [case testNoReturn] # flags: --enable-error-code partially-defined From e3f708f7cdc5a48f79820fb977ad6ac988650cdd Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Fri, 28 Oct 2022 21:45:51 +0200 Subject: [PATCH 02/10] all tests passing --- mypy/partially_defined.py | 14 ++++++++++---- test-data/unit/check-partially-defined.test | 13 +++++++++++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index ad966ec26a49..f4935b16bdc7 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -321,8 +321,15 @@ def process_try(self, o: TryStmt) -> None: # For the purposes of this check, it's an accepted source of false positive errors. # todo(stas): test when try except is inside an if with a raise -- that probably shouldn't skip any branches? self.tracker.start_branch_statement() + # Try can be treated as a separate branch. self.tracker.disable_branch_skip = True o.body.accept(self) + if o.else_body is not None: + # `else` is executed when both of the following are true: + # - No exception was raised in the `try` body. + # - Function didn't return within the `try` body. + # o.body.accept(self) + o.else_body.accept(self) self.tracker.disable_branch_skip = False for i in range(len(o.types)): self.tracker.next_branch() @@ -334,11 +341,10 @@ def process_try(self, o: TryStmt) -> None: if v is not None: v.accept(self) if o.else_body is not None: - # We don't want `raise` or `return` inside try/except to prevent an undefined variable from registering. - # self.tracker.next_branch() - # self.tracker.disable_branch_skip = True + # `else` is executed when both of the following are true: + # - No exception was raised in the `try` body. + # - Function didn't return within the `try` body. o.body.accept(self) - # self.tracker.disable_branch_skip = False o.else_body.accept(self) self.tracker.end_branch_statement() # todo(stas): finally should be executed regardless of returns, raises, etc (i.e. we shouldn't skip branches diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index 04d1520ea64b..1a7aa4847af6 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -419,7 +419,7 @@ def f1() -> int: else: x = 2 finally: - y = x # E: Name "x" may be undefined + y = x return y def f2() -> int: @@ -437,8 +437,17 @@ def f3() -> int: except: x = 1 else: - y = 1 + pass return x # E: Name "x" may be undefined + +def f4() -> int: + try: + x = 1 + except: + x = 2 + else: + pass + return x [builtins fixtures/exception.pyi] [case testNoReturn] From cc914633104c6662f02c4096a8853ab03d1e17e4 Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Fri, 28 Oct 2022 22:50:17 +0200 Subject: [PATCH 03/10] polish --- mypy/partially_defined.py | 91 ++++++++++----------- test-data/unit/check-partially-defined.test | 37 +++++++-- 2 files changed, 76 insertions(+), 52 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index f4935b16bdc7..381982ed00bb 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -55,9 +55,6 @@ def __init__( self.must_be_defined = set(must_be_defined) self.skipped = skipped - def copy(self) -> BranchState: - return BranchState(must_be_defined=set(self.must_be_defined), may_be_defined=set(self.may_be_defined), skipped=self.skipped) - class BranchStatement: def __init__(self, initial_state: BranchState) -> None: @@ -69,11 +66,6 @@ def __init__(self, initial_state: BranchState) -> None: ) ] - def copy(self) -> BranchStatement: - result = BranchStatement(self.initial_state) - result.branches = [b.copy() for b in self.branches] - return result - def next_branch(self) -> None: self.branches.append( BranchState( @@ -134,12 +126,6 @@ def __init__(self) -> None: # in things like try/except statements. self.disable_branch_skip = False - def copy(self) -> DefinedVariableTracker: - result = DefinedVariableTracker() - result.scopes = [s.copy() for s in self.scopes] - result.disable_branch_skip = self.disable_branch_skip - return result - def _scope(self) -> list[BranchStatement]: assert len(self.scopes) > 0 return self.scopes[-1] @@ -303,53 +289,66 @@ def visit_expression_stmt(self, o: ExpressionStmt) -> None: super().visit_expression_stmt(o) def visit_try_stmt(self, o: TryStmt) -> None: - self.process_try(o) - # old_tracker = self.tracker - # finally_tracker = self.tracker.copy() - # finally_tracker.disable_branch_skip = True - # self.tracker = finally_tracker - # self.process_try(o, process_jumps=False) - # self.tracker = old_tracker - - def process_try(self, o: TryStmt) -> None: - # todo(stas): this comment is no longer correct: - # This checker treats try statements as the following exclusive branches: - # - try body - # - except bodies - # - else body - # This is technically incorrect (because the try body may be executed as well as except or else bodies. - # For the purposes of this check, it's an accepted source of false positive errors. - # todo(stas): test when try except is inside an if with a raise -- that probably shouldn't skip any branches? + # We're processing the try statement twice. Include only the one where we allow jumps in the scope + # for code that exists after the try statement. + self.tracker.enter_scope() + self.process_try_stmt(o, allow_all_jumps=False) + self.tracker.exit_scope() + self.process_try_stmt(o, allow_all_jumps=True) + + def process_try_stmt(self, o: TryStmt, allow_all_jumps: bool) -> None: + """ + Processes try statement decomposing it into the following branches: + - try body + else + - try body + except clauses (every except clause is a separate branch). + - Every except clause is decomposed into something like: + if ...: + `try` body + `except` body + - This is necessary so that all the variables inside `try` body are conditionally defined. + + Note that finding partially defined vars in `finally` requires different handling from + the rest of the code. In particular, we want to disallow skipping branches due to jump + statements in except/else clauses for finally but not for other cases. Imagine a case like: + def f() -> int: + try: + x = 1 + except: + # This jump statement needs to be handled differently depending on whether or + # not we're trying to process `finally` or not. + return 0 + finally: + # `x` may be undefined here. + pass + # `x` is always defined here. + return x + """ self.tracker.start_branch_statement() - # Try can be treated as a separate branch. + # The body branch should never be skipped because it will go `finally` unconditionally + # (or it could go to an except clause). self.tracker.disable_branch_skip = True o.body.accept(self) + if allow_all_jumps: + self.tracker.disable_branch_skip = False if o.else_body is not None: - # `else` is executed when both of the following are true: - # - No exception was raised in the `try` body. - # - Function didn't return within the `try` body. - # o.body.accept(self) o.else_body.accept(self) - self.tracker.disable_branch_skip = False for i in range(len(o.types)): self.tracker.next_branch() tp = o.types[i] if tp is not None: tp.accept(self) + # Create a `fake` branch statement, so that all variables from the `body` are conditionally + # defined. + self.tracker.start_branch_statement() + o.body.accept(self) + self.tracker.next_branch() + self.tracker.end_branch_statement() o.handlers[i].accept(self) for v in o.vars: if v is not None: v.accept(self) - if o.else_body is not None: - # `else` is executed when both of the following are true: - # - No exception was raised in the `try` body. - # - Function didn't return within the `try` body. - o.body.accept(self) - o.else_body.accept(self) + self.tracker.disable_branch_skip = False self.tracker.end_branch_statement() - # todo(stas): finally should be executed regardless of returns, raises, etc (i.e. we shouldn't skip branches - # for `finally`) - # todo(stas): test case for raise in except clause if o.finally_body is not None: o.finally_body.accept(self) diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index 1a7aa4847af6..127a39f33b7e 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -332,17 +332,24 @@ def f3() -> int: try: x = 1 except: - return 0 + y = x # E: Name "x" may be undefined return x def f4() -> int: + try: + x = 1 + except: + return 0 + return x + +def f5() -> int: try: x = 1 except: raise return x -def f5() -> None: +def f6() -> None: try: pass except BaseException as e: @@ -350,14 +357,13 @@ def f5() -> None: # This case is covered by the other check, not by partially defined check. y = e # E: Trying to read deleted variable "e" -def f6() -> int: +def f7() -> int: try: - x = 1 + if int(): + x = 1 assert False except: pass - # This is a false-positive. Technically, x is always defined but mypy isn't - # smart enough (yet) to detect this. return x # E: Name "x" may be undefined [builtins fixtures/exception.pyi] @@ -409,6 +415,16 @@ def f3() -> int: y = x # E: Name "x" may be undefined return x +def f4() -> int: + try: + x = 0 + except BaseException: + raise + finally: + y = x # E: Name "x" may be undefined + return y +[builtins fixtures/exception.pyi] + [case testTryElse] # flags: --enable-error-code partially-defined def f1() -> int: @@ -448,6 +464,15 @@ def f4() -> int: else: pass return x + +def f5() -> int: + try: + pass + except: + x = 1 + else: + return 1 + return x [builtins fixtures/exception.pyi] [case testNoReturn] From dc635f67d034bf4ba8942de59685e1ed2aaa349a Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Fri, 28 Oct 2022 22:52:07 +0200 Subject: [PATCH 04/10] formatting --- mypy/partially_defined.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index 381982ed00bb..e71c105e150b 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -22,9 +22,10 @@ NameExpr, RaiseStmt, ReturnStmt, + TryStmt, TupleExpr, WhileStmt, - WithStmt, TryStmt, + WithStmt, ) from mypy.patterns import AsPattern, StarredPattern from mypy.reachability import ALWAYS_TRUE, infer_pattern_value @@ -116,6 +117,7 @@ def done(self) -> BranchState: may_be_defined = all_vars.difference(must_be_defined) return BranchState(may_be_defined=may_be_defined, must_be_defined=must_be_defined) + class DefinedVariableTracker: """DefinedVariableTracker manages the state and scope for the UndefinedVariablesVisitor.""" From 1354b3cd16185beb9d6cc4153ddbdab912c7a49c Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Mon, 28 Nov 2022 19:09:04 -0600 Subject: [PATCH 05/10] Fix bug with computing may_be_defined set --- mypy/partially_defined.py | 32 ++++++++++++--------- test-data/unit/check-partially-defined.test | 6 ++++ 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index 18ad335aea8f..7cc9741b882e 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -119,23 +119,29 @@ def is_defined_in_a_branch(self, name: str) -> bool: return False def done(self) -> BranchState: - branches = [b for b in self.branches if not b.skipped] - if len(branches) == 0: - return BranchState(skipped=True) - if len(branches) == 1: - return branches[0] - - # must_be_defined is a union of must_be_defined of all branches. - must_be_defined = set(branches[0].must_be_defined) - for b in branches[1:]: - must_be_defined.intersection_update(b.must_be_defined) - # may_be_defined are all variables that are not must be defined. + # First, compute all vars, including skipped branches. We include skipped branches + # because our goal is to capture all variables that semantic analyzer would + # consider defined. all_vars = set() - for b in branches: + for b in self.branches: all_vars.update(b.may_be_defined) all_vars.update(b.must_be_defined) + # For the rest of the things, we only care about branches that weren't skipped. + non_skipped_branches = [b for b in self.branches if not b.skipped] + if len(non_skipped_branches) > 0: + must_be_defined = non_skipped_branches[0].must_be_defined + for b in non_skipped_branches[1:]: + must_be_defined.intersection_update(b.must_be_defined) + else: + must_be_defined = set() + # Everything that wasn't defined in all branches but was defined + # in at least one branch should be in `may_be_defined`! may_be_defined = all_vars.difference(must_be_defined) - return BranchState(may_be_defined=may_be_defined, must_be_defined=must_be_defined) + return BranchState( + must_be_defined=must_be_defined, + may_be_defined=may_be_defined, + skipped=len(non_skipped_branches) == 0, + ) class Scope: diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index 220ba481d20d..287fc15418f6 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -268,6 +268,12 @@ def f5() -> int: return 3 return 1 +def f6() -> int: + if int(): + x = 0 + return x + return x # E: Name "x" may be undefined + [case testDefinedDifferentBranchUseBeforeDef] # flags: --enable-error-code partially-defined --enable-error-code use-before-def From 4c192c5963fcce481609eefe5805d304851eb40a Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Mon, 28 Nov 2022 21:27:21 -0600 Subject: [PATCH 06/10] Only process body twice + better tests --- mypy/partially_defined.py | 98 +++++++++++++-------- test-data/unit/check-partially-defined.test | 28 +++++- 2 files changed, 83 insertions(+), 43 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index 7cc9741b882e..feb43775c932 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -64,6 +64,13 @@ def __init__( self.must_be_defined = set(must_be_defined) self.skipped = skipped + def copy(self) -> BranchState: + return BranchState( + must_be_defined=set(self.must_be_defined), + may_be_defined=set(self.may_be_defined), + skipped=self.skipped, + ) + class BranchStatement: def __init__(self, initial_state: BranchState) -> None: @@ -75,6 +82,11 @@ def __init__(self, initial_state: BranchState) -> None: ) ] + def copy(self) -> BranchStatement: + result = BranchStatement(self.initial_state) + result.branches = [b.copy() for b in self.branches] + return result + def next_branch(self) -> None: self.branches.append( BranchState( @@ -149,6 +161,11 @@ def __init__(self, stmts: list[BranchStatement]) -> None: self.branch_stmts: list[BranchStatement] = stmts self.undefined_refs: dict[str, set[NameExpr]] = {} + def copy(self) -> Scope: + result = Scope([s.copy() for s in self.branch_stmts]) + result.undefined_refs = self.undefined_refs.copy() + return result + def record_undefined_ref(self, o: NameExpr) -> None: if o.name not in self.undefined_refs: self.undefined_refs[o.name] = set() @@ -165,9 +182,15 @@ def __init__(self) -> None: # There's always at least one scope. Within each scope, there's at least one "global" BranchingStatement. self.scopes: list[Scope] = [Scope([BranchStatement(BranchState())])] # disable_branch_skip is used to disable skipping a branch due to a return/raise/etc. This is useful - # in things like try/except statements. + # in things like try/except/finally statements. self.disable_branch_skip = False + def copy(self) -> DefinedVariableTracker: + result = DefinedVariableTracker() + result.scopes = [s.copy() for s in self.scopes] + result.disable_branch_skip = self.disable_branch_skip + return result + def _scope(self) -> Scope: assert len(self.scopes) > 0 return self.scopes[-1] @@ -421,24 +444,7 @@ def visit_expression_stmt(self, o: ExpressionStmt) -> None: super().visit_expression_stmt(o) def visit_try_stmt(self, o: TryStmt) -> None: - # We're processing the try statement twice. Include only the one where we allow jumps in the scope - # for code that exists after the try statement. - self.tracker.enter_scope() - self.process_try_stmt(o, allow_all_jumps=False) - self.tracker.exit_scope() - self.process_try_stmt(o, allow_all_jumps=True) - - def process_try_stmt(self, o: TryStmt, allow_all_jumps: bool) -> None: """ - Processes try statement decomposing it into the following branches: - - try body + else - - try body + except clauses (every except clause is a separate branch). - - Every except clause is decomposed into something like: - if ...: - `try` body - `except` body - - This is necessary so that all the variables inside `try` body are conditionally defined. - Note that finding partially defined vars in `finally` requires different handling from the rest of the code. In particular, we want to disallow skipping branches due to jump statements in except/else clauses for finally but not for other cases. Imagine a case like: @@ -455,31 +461,45 @@ def f() -> int: # `x` is always defined here. return x """ + if o.finally_body is not None: + # In order to find partially defined vars in `finally`, we need to + # process try/except with branch skipping disabled. However, for the rest of the code + # after finally, we need to process try/except with branch skipping enabled. + # Therefore, we need to process try/finally twice. + # Because processing is not idempotent, we should make a copy of the tracker. + old_tracker = self.tracker.copy() + self.tracker.disable_branch_skip = True + self.process_try_stmt(o) + self.tracker = old_tracker + self.process_try_stmt(o) + + def process_try_stmt(self, o: TryStmt) -> None: + """ + Processes try statement decomposing it into the following: + if ...: + body + else_body + elif ...: + except 1 + elif ...: + except 2 + else: + except n + finally + """ self.tracker.start_branch_statement() - # The body branch should never be skipped because it will go `finally` unconditionally - # (or it could go to an except clause). - self.tracker.disable_branch_skip = True o.body.accept(self) - if allow_all_jumps: - self.tracker.disable_branch_skip = False if o.else_body is not None: o.else_body.accept(self) - for i in range(len(o.types)): - self.tracker.next_branch() - tp = o.types[i] - if tp is not None: - tp.accept(self) - # Create a `fake` branch statement, so that all variables from the `body` are conditionally - # defined. - self.tracker.start_branch_statement() - o.body.accept(self) - self.tracker.next_branch() - self.tracker.end_branch_statement() - o.handlers[i].accept(self) - for v in o.vars: - if v is not None: - v.accept(self) - self.tracker.disable_branch_skip = False + if len(o.handlers) > 0: + assert len(o.handlers) == len(o.vars) == len(o.types) + for i in range(len(o.handlers)): + self.tracker.next_branch() + if o.types[i] is not None: + o.types[i].accept(self) + if o.vars[i] is not None: + o.vars[i].accept(self) + o.handlers[i].accept(self) self.tracker.end_branch_statement() if o.finally_body is not None: o.finally_body.accept(self) diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index 287fc15418f6..83df8d55c3e3 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -449,7 +449,7 @@ def f3() -> None: z = x # E: Name "x" may be undefined [case testTryBasic] -# flags: --enable-error-code partially-defined +# flags: --enable-error-code partially-defined --enable-error-code use-before-def def f1() -> int: try: x = 1 @@ -468,8 +468,8 @@ def f3() -> int: try: x = 1 except: - y = x # E: Name "x" may be undefined - return x + y = x # E: Name "x" is used before definition + return x # E: Name "x" may be undefined def f4() -> int: try: @@ -525,7 +525,7 @@ def f2() -> int: [builtins fixtures/exception.pyi] [case testTryFinally] -# flags: --enable-error-code partially-defined +# flags: --enable-error-code partially-defined --enable-error-code use-before-def def f1() -> int: try: x = 1 @@ -559,6 +559,26 @@ def f4() -> int: finally: y = x # E: Name "x" may be undefined return y + +def f5() -> int: + try: + if int(): + x = 1 + else: + return 0 + finally: + pass + return x # No error. + +def f6() -> int: + try: + if int(): + x = 1 + else: + return 0 + finally: + a = x # E: Name "x" may be undefined + return a [builtins fixtures/exception.pyi] [case testTryElse] From 3f8b7207133b702cceb35950abdb0f3c099293ad Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Mon, 28 Nov 2022 21:33:48 -0600 Subject: [PATCH 07/10] Fix is None check --- mypy/partially_defined.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index feb43775c932..cb2514fd7b84 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -495,10 +495,12 @@ def process_try_stmt(self, o: TryStmt) -> None: assert len(o.handlers) == len(o.vars) == len(o.types) for i in range(len(o.handlers)): self.tracker.next_branch() - if o.types[i] is not None: - o.types[i].accept(self) - if o.vars[i] is not None: - o.vars[i].accept(self) + exc_type = o.types[i] + if exc_type is not None: + exc_type.accept(self) + var = o.vars[i] + if var is not None: + var.accept(self) o.handlers[i].accept(self) self.tracker.end_branch_statement() if o.finally_body is not None: From c1a693f61e88a379ae6410f2684cbcdc4ce21ad9 Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Tue, 29 Nov 2022 16:03:29 -0600 Subject: [PATCH 08/10] track definition of exception names --- mypy/partially_defined.py | 14 ++++++++++++++ test-data/unit/check-partially-defined.test | 7 ++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index cc628b960842..b17aba657a9e 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -101,6 +101,11 @@ def record_definition(self, name: str) -> None: self.branches[-1].must_be_defined.add(name) self.branches[-1].may_be_defined.discard(name) + def delete_var(self, name: str) -> None: + assert len(self.branches) > 0 + self.branches[-1].must_be_defined.discard(name) + self.branches[-1].may_be_defined.discard(name) + def record_nested_branch(self, state: BranchState) -> None: assert len(self.branches) > 0 current_branch = self.branches[-1] @@ -228,6 +233,11 @@ def record_definition(self, name: str) -> None: assert len(self.scopes[-1].branch_stmts) > 0 self._scope().branch_stmts[-1].record_definition(name) + def delete_var(self, name: str) -> None: + assert len(self.scopes) > 0 + assert len(self.scopes[-1].branch_stmts) > 0 + self._scope().branch_stmts[-1].delete_var(name) + def record_undefined_ref(self, o: NameExpr) -> None: """Records an undefined reference. These can later be retrieved via `pop_undefined_ref`.""" assert len(self.scopes) > 0 @@ -504,9 +514,13 @@ def process_try_stmt(self, o: TryStmt) -> None: exc_type.accept(self) var = o.vars[i] if var is not None: + self.process_definition(var.name) var.accept(self) o.handlers[i].accept(self) + if var is not None: + self.tracker.delete_var(var.name) self.tracker.end_branch_statement() + if o.finally_body is not None: o.finally_body.accept(self) diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index b6c79709f598..8c7aca7d5f8b 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -522,10 +522,11 @@ def f5() -> int: def f6() -> None: try: pass - except BaseException as e: - pass + except BaseException as exc: + x = exc # No error. + exc = BaseException() # This case is covered by the other check, not by partially defined check. - y = e # E: Trying to read deleted variable "e" + y = exc # E: Trying to read deleted variable "exc" def f7() -> int: try: From bbea3e04291e6c82d7a3a33b11999f405ef20cf5 Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Wed, 30 Nov 2022 10:52:30 -0600 Subject: [PATCH 09/10] Report variables defined in try body inside except as may be undefined --- mypy/partially_defined.py | 7 ++++++- test-data/unit/check-partially-defined.test | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index b17aba657a9e..ca139542ba5c 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -303,6 +303,7 @@ def __init__( self.type_map = type_map self.options = options self.loops: list[Loop] = [] + self.try_depth = 0 self.tracker = DefinedVariableTracker() for name in implicit_module_attrs: self.tracker.record_definition(name) @@ -475,6 +476,7 @@ def f() -> int: # `x` is always defined here. return x """ + self.try_depth += 1 if o.finally_body is not None: # In order to find partially defined vars in `finally`, we need to # process try/except with branch skipping disabled. However, for the rest of the code @@ -486,6 +488,7 @@ def f() -> int: self.process_try_stmt(o) self.tracker = old_tracker self.process_try_stmt(o) + self.try_depth -= 1 def process_try_stmt(self, o: TryStmt) -> None: """ @@ -570,7 +573,9 @@ def visit_name_expr(self, o: NameExpr) -> None: self.tracker.record_definition(o.name) elif self.tracker.is_defined_in_different_branch(o.name): # A variable is defined in one branch but used in a different branch. - if self.loops: + if self.loops or self.try_depth > 0: + # If we're in a loop or in a try, we can't be sure that this variable + # is undefined. Report it as "may be undefined". self.variable_may_be_undefined(o.name, o) else: self.var_used_before_def(o.name, o) diff --git a/test-data/unit/check-partially-defined.test b/test-data/unit/check-partially-defined.test index 8c7aca7d5f8b..e6547e258f95 100644 --- a/test-data/unit/check-partially-defined.test +++ b/test-data/unit/check-partially-defined.test @@ -502,7 +502,7 @@ def f3() -> int: try: x = 1 except: - y = x # E: Name "x" is used before definition + y = x # E: Name "x" may be undefined return x # E: Name "x" may be undefined def f4() -> int: From 3a0bc99ec12f8cde90037bc19fff660a41bb6405 Mon Sep 17 00:00:00 2001 From: Stas Ilinskiy Date: Fri, 16 Dec 2022 16:59:25 -0600 Subject: [PATCH 10/10] Rename error code --- mypy/partially_defined.py | 4 ++-- test-data/unit/check-possibly-undefined.test | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py index 147944932846..9b3e105f64ef 100644 --- a/mypy/partially_defined.py +++ b/mypy/partially_defined.py @@ -472,7 +472,7 @@ def visit_expression_stmt(self, o: ExpressionStmt) -> None: def visit_try_stmt(self, o: TryStmt) -> None: """ - Note that finding partially defined vars in `finally` requires different handling from + Note that finding undefined vars in `finally` requires different handling from the rest of the code. In particular, we want to disallow skipping branches due to jump statements in except/else clauses for finally but not for other cases. Imagine a case like: def f() -> int: @@ -490,7 +490,7 @@ def f() -> int: """ self.try_depth += 1 if o.finally_body is not None: - # In order to find partially defined vars in `finally`, we need to + # In order to find undefined vars in `finally`, we need to # process try/except with branch skipping disabled. However, for the rest of the code # after finally, we need to process try/except with branch skipping enabled. # Therefore, we need to process try/finally twice. diff --git a/test-data/unit/check-possibly-undefined.test b/test-data/unit/check-possibly-undefined.test index 7ca033854e6e..ee7020252de8 100644 --- a/test-data/unit/check-possibly-undefined.test +++ b/test-data/unit/check-possibly-undefined.test @@ -526,7 +526,7 @@ def f3() -> None: z = x # E: Name "x" may be undefined [case testTryBasic] -# flags: --enable-error-code partially-defined --enable-error-code use-before-def +# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def def f1() -> int: try: x = 1 @@ -568,7 +568,7 @@ def f6() -> None: except BaseException as exc: x = exc # No error. exc = BaseException() - # This case is covered by the other check, not by partially defined check. + # This case is covered by the other check, not by possibly undefined check. y = exc # E: Trying to read deleted variable "exc" def f7() -> int: @@ -582,7 +582,7 @@ def f7() -> int: [builtins fixtures/exception.pyi] [case testTryMultiExcept] -# flags: --enable-error-code partially-defined +# flags: --enable-error-code possibly-undefined def f1() -> int: try: x = 1 @@ -603,7 +603,7 @@ def f2() -> int: [builtins fixtures/exception.pyi] [case testTryFinally] -# flags: --enable-error-code partially-defined --enable-error-code use-before-def +# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def def f1() -> int: try: x = 1 @@ -660,7 +660,7 @@ def f6() -> int: [builtins fixtures/exception.pyi] [case testTryElse] -# flags: --enable-error-code partially-defined +# flags: --enable-error-code possibly-undefined def f1() -> int: try: return 0