Skip to content

Specialize type variables of generic functions that are arguments #1471

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

Closed
wants to merge 1 commit into from

Conversation

rwbarton
Copy link
Contributor

@rwbarton rwbarton commented May 3, 2016

This kind of situation appears in pytokenize.py:

tokenprog, pseudoprog, single3prog, double3prog = map(
    re.compile, (Token, PseudoToken, Single3, Double3))

The inferred types of tokenprog, ... are currently Pattern[AnyStr], which is very wrong.
(Not enough of this file is annotated for that to cause other mypy errors yet, though.)

res[ai] = self.accept(args[ai], callee.arg_types[i])
callee_arg_type = callee.arg_types[i]
arg_type = self.accept(args[ai], callee_arg_type)
if (isinstance(arg_type, CallableType) and
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to fix this for more general argument types. For example, what if the formal argument type is List[Callable[...]]? We could look into the types recursively, but not sure if that's the right thing to do (and it might be inefficient). I thought about this for a bit and this is far from clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A list-of-generic-function isn't a subtype of list-of-specific-function anyways, because List is invariant in its argument. But your concern is valid for covariant type constructors.

I'm going to think some more about why exactly this change is needed and what goes wrong without it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The example I used involved a [...] expression in the argument so the list-of-generic-function could be specialized based on context (but isn't specialized correctly), but you are correct and it's a different case due to invariance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're talking about a case like

def f(xs: List[Callable[[T], S]], y: T) -> List[S]: pass
def id(x: U) -> U: pass

f([id], 1)

In principle we should be able to type check this as follows:

  • The first pass of type checking finds that T = int.
  • f provides the context List[Callable[[int], S]] to the expression [id], which is itself treated as a call to a generic function "[_]" that produces a list.
  • [_] provides the context Callable[[int], S] to the expression id. Now when id is type checked, the result is a generic callable type, so the code in this change unifies that type with the context, resulting in Callable[[int], int].
  • Now the first argument of f has type List[Callable[[int], int]] so we can infer that S = int.

In practice the third step doesn't work yet because we don't substitute variables in infer_function_type_arguments_using_context when the replacement would involve a type variable (here S).

@rwbarton
Copy link
Contributor Author

rwbarton commented May 9, 2016

I'm going to abandon this attempt for now since this is turning into a larger issue than expected and I'd rather work on #1261 first.

However one observation I've made is that in situations like #1506 and #1507 it usually works correctly, or almost correctly, to replace the generic function by a lambda (re.compile -> lambda s: re.compile(s)). Perhaps that suggests a better strategy for dealing with these cases.

@rwbarton rwbarton closed this May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants