Skip to content
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.

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

Fix issue 7863 #7933

Merged
merged 11 commits into from Nov 15, 2019

Conversation

@beezee
Copy link
Contributor

beezee commented Nov 12, 2019

See #7863 for background context.

A few small notes.

1- Assignment to t on line 529 of checkmember.py was not being referenced til 535. Unless get_proper_type(expand_type_by_instance(typ, itype)) is mutating typ or itype then this created ambiguity for me around whether lines 530-534 were going to be impacted by my overall change. Since it appears not to be the case, I figured it's clearer to introduce t no earlier than it is used, (which in hindsight with the rest of the change could probably be even later in the function.)

2- Changes to check-selftype.test appear to be a result of calling bind_self before get_proper_type(expand_type_by_instance(signature, itype)) - my understanding is that bind_self does not register inference by susbtitution of type var, and so the inference of self types are no longer marked as such.

3- #TODO in my added tests - I'm unable to successfully run when importing ABC (error: module abc has no value ABC,) if anyone has guidance on how I might fix that I'd appreciate it, will gladly update accordingly.

I haven't thoroughly grokked the work in freshen_function_type_vars, I arrived at this solution mostly by stepping through debugger between the diverging code paths in my original issue to isolate when the type became incorrect, and comparing the work being done between the two paths.

I have a few qs that I'd love any insight into.

1- What is the difference between negative and positive generic type identifiers? I had suspected contravariant vs covariant, but then the results of freshen_function_type_vars would seem to contradict (it flips negative to positive in contravariant position.)

2- Why do we track generic type vars solely by the internally assigned numeric identifiers and not by the names assigned by programmer? The original issue came up by conflating A-1 and B-1 during inference, but I can't ever think of a case where a programmer would intend A to have reference equality to B, vs a particular instantiation of Generic[A, B] that happens to choose the same concrete type for both arguments.

I hope this is on the right track. Thanks in advance for review, to @ilevkivskyi for the initial guidance, and for any insight offered on the questions above.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

JelleZijlstra commented Nov 12, 2019

I'm unable to successfully run when importing ABC

This is because the mypy tests run with special, small stubs from the test-data/unit/lib-stubs/ directory, and the abc stub there doesn't include abc.ABC, presumably because no previous test has needed it.

@msullivan msullivan self-requested a review Nov 12, 2019
@beezee

This comment has been minimized.

Copy link
Contributor Author

beezee commented Nov 13, 2019

I thought I could smash this thing to green by adjusting my brittle tests to match the output of travis, and completely missed appveyor, which just highlights the comment I had prepared for once it got there:

At this point it looks like the build passes with the exception of the tests I've added as part of the PR.

The tests currently cannot reliably pass because of the call to freshen_function_type_vars I added in this PR, which is key to the bugfix.

Given that this function is already being called in a separate related code path (analyze_instance_member_access) - is there something I can do to preserve the value of the new tests without the overfit assertions? Is there precedent for data-driven tests that contain generic types in their output?

Best case, is there a way to dynamically get the next increment for assignment of ids to type arguments, and interpolate the correct value into the comments that make up the assertions in my test?

Any suggestions greatly appreciated.


def __call__(self) -> B: pass

# TODO - This should extend ABC but test errors that ABC not in abc on import

This comment has been minimized.

Copy link
@msullivan

msullivan Nov 13, 2019

Collaborator

Comment stale

This comment has been minimized.

Copy link
@beezee

beezee Nov 14, 2019

Author Contributor

Removed thanks.

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Nov 13, 2019

There is not a lot of precedent for type variables like this in tests I think. We could probably hack together something if needed, but how much value would be lost if we just removed that part of the test?

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Nov 13, 2019

For bound type variables, negative indicates that they were bound by a function and positive that they were bound by a class. For "type inference metavariables" that are used during type inference to solve constraints, they are always positive. Type variables track which of these they are but that is not reflected in the string formatting of them...

We don't use the names because names can be reused in bindings for different functions (or even the same function invoked multiple times in an expression with different type parameters)

@@ -555,13 +556,15 @@ def analyze_var(name: str,
dispatched_type = meet.meet_types(mx.original_type, itype)
functype = check_self_arg(functype, dispatched_type, var.is_classmethod,
mx.context, name, mx.msg)
signature = bind_self(functype, mx.self_type, var.is_classmethod)
signature = freshen_function_type_vars(functype)

This comment has been minimized.

Copy link
@msullivan

msullivan Nov 14, 2019

Collaborator

I think we should freshen the function type vars before the check_self_arg, to match how it is done elsewhere

This comment has been minimized.

Copy link
@beezee

beezee Nov 14, 2019

Author Contributor

Done, thanks for the pointer.

@beezee

This comment has been minimized.

Copy link
Contributor Author

beezee commented Nov 14, 2019

I appreciate your q around the tests. I adjusted them to demonstrate the independence of type var bound at class level vs bound at method level, and I think this reasonably covers the focus of the PR against regression, if not less generally.

Re pos/neg type vars - is that association something that freshen_function_type_vars would/should respect, or does that all fall under inference metavariables?

Re using names - it sounds like the identifiers are meant to subsume tracking of lexical scope? I'm sure the impact and effort for a change at that level would be huge, but I can't help wondering if assigning prefixes at certain levels of scope would produce just enough disambiguation without needing to discard the programmers input. To that end, does the internal id management break down if it is used in addition to the names assigned by programmer? I think if the latter approach were taken, we'd still have binding distinction at least at the granularity we have now, and then some. The additional distinction would have prevented the original issue in this PR, namely that A-1 is considered the same as B-1, regardless what concrete types are inhabiting those parameters.

Sorry for the side thread, mostly trying to catch myself up with the underlying design and implementation. I'm having a lot of fun pushing what I can get mypy to prove, and hoping in the process I'll be able to continue contributing back more as I find dark corners.

Thanks for the review, please lmk if there's anything else I can improve here to get merged.

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Nov 15, 2019

All the variables freshen_function_type_vars inserts are inference metavariables.

Using the names in addition to the IDs could have prevented this bug but would have allowed other similar bugs, I think. It would still have been wrong to not replace the variables with inference metavariables.

Copy link
Collaborator

msullivan left a comment

Thanks!

@msullivan msullivan merged commit 9d5d0e9 into python:master Nov 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.