Update tests to prepare for variable redefinition with new type #6095

JukkaL · 2018-12-20T15:45:22Z

After the planned change, test cases no longer can rely on code
like this generating an error, since the two assignments will
create two logically separate variables with independent types:

x = 0
x = ''  # Currently an error, but likely not in the future

Instead, the idea is to use an if statement to prevent redefinition.
This works since redefinition will only happen unconditionally:

x = 0
if int():
    x = ''  # Will continue to be an error

Even this may stop working in the future, and we may need to
another major test case update then. If we always use the
if int(): idiom, finding things that need changing will be
easy. (Sometimes if int(): didn't work and I used if 1:
instead.)

In new test cases we should generally avoid using assignment
statements such as the above to generate errors. Instead, we
can use reveal_type() and calls to functions with incompatible
argument types, for example.

After the planned change, test cases no longer can rely on code like this generating an error, since the two assignments create two logically separate variables with independent types: ``` x = 0 x = '' # Currently an error, but likely not in the future ``` Instead, we use an if statement to prevent redefinition. This works since redefinition will only happen unconditionally: ``` x = 0 if int(): x = '' # Still an error ``` Even this may stop working in the future, and we may need to another major test case update then. If we always use the `if int():` idiom, finding things that need changing will be easy. In new test cases we should generally avoid using assignment statements such as the above to generate errors. Instead, we can use `reveal_type()` and calls to functions with incompatible argument types, for example.

ilevkivskyi

I have some random comments below, I summarize them all here:

First, I would gather all the patterns used, and add a single explanatory comment at the top of one of the test files, so this information is not lost. I have found:

if int(): ...
if str(): ...
if 1: ...
x (look-up in the same scope)
def dummy(): x (look-up from a nested scope)

It would be helpful to add some motivation for each of those.

Second, I am worried about shadowing final names. This kind of defeats the purpose. We can discuss this separately however, but I would prefer to not modify those tests.

Third, interaction with Any is a bit unexpected, it can create both "false" positives, and "false" negatives (depending on the order), again probably we can discuss this separately.

Finally, there is a use case that is problematic with current strategy (allow redefinition only for non-overlapping life-times):

x = 1.1 
x = 1
if blah():
    x = 1.2

currently this works, but will (IIUC) cause an error with the new logic. I think it would be better to have a mini-design doc to discuss strategy and details of the new logic.

ilevkivskyi · 2018-12-20T16:50:15Z

test-data/unit/check-dynamic-typing.test

+if int():
+    d, a = b, b    # E: Incompatible types in assignment (expression has type "B", variable has type "A")
+if int():
+    d, d = d, d, d # E: Too many values to unpack (2 expected, 3 provided)


Why if int(): is needed here?

ilevkivskyi · 2018-12-20T17:02:13Z

test-data/unit/check-dynamic-typing.test

 d[a()] = a # E: "A" not callable

-a = d[a]
+if int():
+    a = d[a]


I am also not sure why it is needed here. There is no error on this line. Will this create a new variable with type Any that shows the one with type A?

ilevkivskyi · 2018-12-20T17:05:04Z

test-data/unit/check-enum.test

@@ -74,11 +77,11 @@ class N(IntEnum):
    x = 1
    y = 1
 n = 1
-n = N.x  # Subclass of int, so it's okay
+if int():
+    n = N.x  # Subclass of int, so it's okay


This one is also not clear. I assume we want to test that there is no error even with the new semantics.

Yes. If we omitted the if statement, the assignment would not test anything interesting.

ilevkivskyi · 2018-12-20T17:06:08Z

test-data/unit/check-expressions.test

 class A:
    pass

 [case testFloatLiteral]
 a = 0.0
 b = None # type: A
-b = 1.1 # E: Incompatible types in assignment (expression has type "float", variable has type "A")
-a = 1.1
+if str():


What is specific about str() here so it is used instead of int()?

int was not defined in the builtins fixture, so I decided to use str() instead.

ilevkivskyi · 2018-12-20T17:08:40Z

test-data/unit/check-final.test

@@ -339,22 +339,29 @@ class C:
 from typing import Final

 x: Final = 1
+x


I strongly prefer to not shadow final names, since this kind of defeats the purpose.

My implementation does some additional checks in a later pass after renaming to prevent the redefinition of final names. It should preserve the existing semantics of final names. Or is there something else you are worried about?

The x is reference is used to trigger the renaming machinery. Otherwise the second definition would be trivially rejected, as it would not count as a new definition. My implementation only renames a variable if it has been read, since this shouldn't be treated as a redefinition:

x = None # type: List[int] x = [] # Not a redefinition

ilevkivskyi · 2018-12-20T17:27:29Z

test-data/unit/check-isinstance.test

@@ -300,6 +307,7 @@ class B(A):
    b = 1

 x = A()
+def f(): x  # Prevent redefinition


I would create a full list of "hack" patterns used, and place it in one of the test files as a comment.

I'll add a separate file where I describe them, as these aren't specific to any particular test file.

ilevkivskyi · 2018-12-20T17:30:21Z

test-data/unit/check-newtype.test

@@ -310,17 +310,15 @@ main:4: error: Invalid type "__main__.T"
 from typing import NewType

 a = 3
-a = NewType('a', int)
+def f(): a


Allowing redefinition with a different kind of symbol looks dangerous from the point of view of fine grained incremental, but maybe it is OK.

Since the implementation uses renaming, the this shouldn't be dangerous as long as the renaming is implemented correctly. The implementation I have does renaming before semantic analysis so we can't always detect the kind of symbol being defined. Function and class definitions won't be able to redefine variables.

JukkaL · 2018-12-21T13:41:27Z

First, I would gather all the patterns used

A good idea! I'll add some documentation about these.

Second, I am worried about shadowing final names.

I think that final semantics should be effectively the same. I don't remember why I modified the test cases -- it may have been due to changes in the details of error messages. I'll double check this before merging.

Third, interaction with Any is a bit unexpected, it can create both "false" positives, and "false" negatives (depending on the order), again probably we can discuss this separately.

This can be discussed once we have the proposed implementation for the feature. I'd argue that the new behavior is usually an improvement -- they caught many false negatives in Dropbox codebases, and they also fix false positives. There is no explicit interaction with Any, since the implementation performs a renaming transformation without any knowledge of types.

Finally, there is a use case that is problematic with current strategy (allow redefinition only for non-overlapping life-times)

Yes, this could generate new errors. I didn't see many examples of this in the Dropbox S codebase though, so it's probably rare.

ilevkivskyi

Thanks for updates! This looks good now.

JukkaL and others added 4 commits December 20, 2018 13:48

Fix some failing tests

d447058

Use if int(): consistently

b9cce65

Fix some test cases

107f9aa

JukkaL requested a review from ilevkivskyi December 20, 2018 15:45

ilevkivskyi reviewed Dec 20, 2018

View reviewed changes

JukkaL added 2 commits December 21, 2018 14:15

Document some hacks used in test cases

731900f

Add comments to a test case

d2690a3

ilevkivskyi approved these changes Dec 21, 2018

View reviewed changes

JukkaL merged commit a7296c4 into master Dec 21, 2018

JelleZijlstra deleted the redefine-test-updates branch December 21, 2018 18:03

Uh oh!

Update tests to prepare for variable redefinition with new type #6095

Update tests to prepare for variable redefinition with new type #6095

Uh oh!

Conversation

JukkaL commented Dec 20, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JukkaL commented Dec 21, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!