Prohibit Callable[[...], X] #320
Conversation
|
Strange that was ever accepted. Was this aways there? I don't recall intending to support this. |
|
@gvanrossum It look like it was introduced in #308 when I split |
| if not isinstance(args, list): | ||
| raise TypeError("Callable[args, result]: args must be a list." | ||
| " Got %.100r." % (args,)) | ||
| if not isinstance(args, list) or ... in args or () in args: |
There was a problem hiding this comment.
Hm... It really shouldn't be necessary to check this here. I think the reason may be that you're trying to make the hash key for e.g. Callable[[int, str], float] be the tuple (int, str, float) rather than just nesting the original structure. The exceptions in __getitem_inner__ seem due to this.
Or maybe you're trying to make __parameters__ a single-level tuple? But there are two exceptions for this, one for Callable[..., t] and one for Callable[[], t] (IIUC). I don't think this is really worth it...
There was a problem hiding this comment.
@gvanrossum Yes, indeed I wanted to flatten __parameters__ to just call super().__getitem__. But you are right, it is not necessary to do this here, but only right before I actually call super().
| @@ -1221,9 +1221,9 @@ def __getitem__(self, parameters): | |||
| elif args == []: | |||
There was a problem hiding this comment.
I don't think you need this special case, and if you remove it and the matching special case in __getitem_inner__ the tests still passes. (It's different here than for Tuple -- while Tuple[] is invalid due to Python's syntax, Callable[[], t] is fine, and there's no need to treat an empty list special.
PS. I can't add a comment there, but your code below has [...,] and [(),] -- those trailing commas are not needed.
There was a problem hiding this comment.
@gvanrossum You are right, only ellipsis needs special case (in two places you mention, and also in __repr__).
|
@gvanrossum Thanks for review! I pushed new commits taking into account your comments. Now this is much simpler (and probably right) way to fix this. |
| else: | ||
| if not isinstance(args, list): | ||
| raise TypeError("Callable[args, result]: args must be a list." | ||
| " Got %.100r." % (args,)) | ||
| parameters = tuple(args) + (result,) | ||
| parameters = tuple(args), result |
There was a problem hiding this comment.
I'd put (redundant) parentheses around the RHS just to emphasize that we're really creating a two-tuple here.
| msg = "Callable[args, result]: result must be a type." | ||
| result = _type_check(result, msg) | ||
| if args == [...,]: | ||
| if args == ...: |
There was a problem hiding this comment.
I'd spell this as Ellipsis -- seeing args == ... is just too weird. :-) Plus it's a singleton, so you should use is.
|
I implemented your latest comments in a new commit.
This is a good idea, I will make a PR later this week. |
@gvanrossum
Now
Callable[[...], X]is accepted and treated as equivalent toCallable[..., X](alsoCallable[[()], X]is treated as equivalent toCallable[[], X]). PEP 484 does not specify the former forms, so that this PR makes them prohibited.I do not have any preference on this, so please feel free to either merge or close this.