Skip to content

Fix internal error on list/dict comprehension with walrus operator in global scope #9062

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 4, 2020
Merged

Fix internal error on list/dict comprehension with walrus operator in global scope #9062

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e66891f
Add tests for dicts using assignment expression
dhood Jun 28, 2020
c0f633f
Duplicate walrus dict/list tests to global scope (passing)
dhood Jun 28, 2020
822813d
Duplicate walrus comprehension tests to global scope (now failing)
dhood Jun 28, 2020
ed9ba4f
Use global symbol table if 1st non-comprehension scope
dhood Jun 28, 2020
a68bd68
Confirm the symbol table retrieved is not None
dhood Jul 5, 2020
8585a4f
Add tests for assignment expression in class scope
dhood Jul 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion mypy/semanal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4542,9 +4542,15 @@ def current_symbol_table(self, escape_comprehensions: bool = False) -> SymbolTab
if self.is_func_scope():
assert self.locals[-1] is not None
if escape_comprehensions:
assert len(self.locals) == len(self.is_comprehension_stack)
# Retrieve the symbol table from the enclosing non-comprehension scope.
for i, is_comprehension in enumerate(reversed(self.is_comprehension_stack)):
if not is_comprehension:
names = self.locals[-1 - i]
if i == len(self.locals) - 1: # The last iteration.
# The caller of the comprehension is in the global space.
names = self.globals
else:
names = cast(SymbolTable, self.locals[-1 - i])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this cast() needed? Could you use a # type: comment on the first assignment instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cast here was because self.locals is a list of Optional[SymbolTable], and so without it, you would get:

mypy\semanal.py:4552: error: Incompatible types in assignment (expression has type "Optional[SymbolTable]", variable has type "SymbolTable")  [assignment]
                                names = self.locals[-1 - i]
                                        ^

I reasoned (at the time) that it is a safe cast as long as we are not retrieving the None that is the first element in the list.

mypy/mypy/semanal.py

Line 226 in ed9ba4f

self.locals = [None]

However, now that you have mentioned to check the class scope, I have realised that there may be other elements in the list that are None:

mypy/mypy/semanal.py

Line 1166 in ed9ba4f

self.locals.append(None) # Add class scope

Perhaps once I deal with the class case properly this cast will not be necessary. Either way, for now, my original assumption that only the first entry is None seems invalid! So I will find an alternative approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since comprehensions in a class scope e.g.

class Example:
    [(j := i) for i in range(5)]

can also result in the retrieved self.locals[-1 -i] symbol table being None, I added extra logic for that case, which removed the need for the cast, in a68bd68.

Because this specific case is called out in https://www.python.org/dev/peps/pep-0572/#scope-of-the-target as being invalid syntax (I know that you know this, I am just being verbose), the specific logic that's added is actually just an assertion that we don't end up in that case.

break
else:
assert False, "Should have at least one non-comprehension scope"
Expand Down
34 changes: 34 additions & 0 deletions test-data/unit/check-python38.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,27 @@ if a := 2:
while b := "x":
reveal_type(b) # N: Revealed type is 'builtins.str'

l = [y2 := 1, y2 + 2, y2 + 3]
reveal_type(y2) # N: Revealed type is 'builtins.int'
reveal_type(l) # N: Revealed type is 'builtins.list[builtins.int*]'

filtered_data = [y3 for x in l if (y3 := a) is not None]
reveal_type(filtered_data) # N: Revealed type is 'builtins.list[builtins.int*]'
reveal_type(y3) # N: Revealed type is 'builtins.int'

d = {'a': (a2 := 1), 'b': a2 + 1, 'c': a2 + 2}
reveal_type(d) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(a2) # N: Revealed type is 'builtins.int'

d2 = {(prefix := 'key_') + 'a': (start_val := 1), prefix + 'b': start_val + 1, prefix + 'c': start_val + 2}
reveal_type(d2) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(prefix) # N: Revealed type is 'builtins.str'
reveal_type(start_val) # N: Revealed type is 'builtins.int'

filtered_dict = {k: new_v for k, v in [('a', 1), ('b', 2), ('c', 3)] if (new_v := v + 1) == 2}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If reviewers are wondering why I didn't just use d.items() here instead of the list of tuples, it's because it's causing error: "Dict[str, int]" has no attribute "items".
I couldn't figure out why that was happening, I tried different flags and casting and whatnot but something in the test environment must be different... in a regular python script filtered_dict = {k: new_v for k, v in d.items() if (new_v := v + 1) == 2} can have its type revealed without such an error. Open to suggestions if you have ideas what's causing that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost certainly because the test fixture you're using happens not to define dict.items. What you're doing here is fine.

reveal_type(filtered_dict) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(new_v) # N: Revealed type is 'builtins.int'

def f(x: int = (c := 4)) -> int:
if a := 2:
reveal_type(a) # N: Revealed type is 'builtins.int'
Expand All @@ -218,6 +239,19 @@ def f(x: int = (c := 4)) -> int:
reveal_type(filtered_data) # N: Revealed type is 'builtins.list[builtins.int*]'
reveal_type(y3) # N: Revealed type is 'builtins.int'

d = {'a': (a2 := 1), 'b': a2 + 1, 'c': a2 + 2}
reveal_type(d) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(a2) # N: Revealed type is 'builtins.int'

d2 = {(prefix := 'key_') + 'a': (start_val := 1), prefix + 'b': start_val + 1, prefix + 'c': start_val + 2}
reveal_type(d2) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(prefix) # N: Revealed type is 'builtins.str'
reveal_type(start_val) # N: Revealed type is 'builtins.int'

filtered_dict = {k: new_v for k, v in [('a', 1), ('b', 2), ('c', 3)] if (new_v := v + 1) == 2}
reveal_type(filtered_dict) # N: Revealed type is 'builtins.dict[builtins.str*, builtins.int*]'
reveal_type(new_v) # N: Revealed type is 'builtins.int'

# https://www.python.org/dev/peps/pep-0572/#exceptional-cases
(y4 := 3)
reveal_type(y4) # N: Revealed type is 'builtins.int'
Expand Down