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

PEP 544: Fix example #2332

Closed
wants to merge 1 commit into from
Closed

PEP 544: Fix example #2332

wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Feb 16, 2022

Fix last example in PEP 544 - Type[] and class objects vs protocols.

Changes

  • C is a class, the annotations for a and b should thus include Type.
    (The section title includes "Type[] and class objects" after all.)
  • The type error is emitted for b: Type[ProtoB] = C and not a.

@JelleZijlstra
Copy link
Member

The example is correct as written. This section is about having the class object itself match the protocol. If you have an instance p of ProtoB, this promises that you can write p.meth(Any, 1), and indeed C.meth(Any, 1) works.

@cdce8p
Copy link
Contributor Author

cdce8p commented Feb 16, 2022

The example is correct as written.

In that case the type checker are broken.

# mypy
error: Incompatible types in assignment (expression has type "Type[C]", variable has type "ProtoA")  [assignment]
error: Incompatible types in assignment (expression has type "Type[C]", variable has type "ProtoB")  [assignment]

# pyright
  error: Expression of type "Type[C]" cannot be assigned to declared type "ProtoA"
    "meth" is an incompatible type
      Type "(self: C, x: int) -> int" cannot be assigned to type "(x: int) -> int"
        Parameter name mismatch: "x" versus "self"
        Parameter 1: type "int" cannot be assigned to type "C"
          "int" is incompatible with "C"
        Keyword parameter "x" is missing in destination (reportGeneralTypeIssues)
  error: Expression of type "Type[C]" cannot be assigned to declared type "ProtoB"
    "meth" is an incompatible type
      Type "(self: C, x: int) -> int" cannot be assigned to type "(obj: Any, x: int) -> int"
        Parameter name mismatch: "obj" versus "self" (reportGeneralTypeIssues)

This section is about having the class object itself match the protocol. If you have an instance p of ProtoB, this promises that you can write p.meth(Any, 1), and indeed C.meth(Any, 1) works.

Was the assignment intended to be to an instance of C, i.e. a: ProtoA = C()? That's the other variant that would work, but even then the OK and error case are switched.

@JelleZijlstra
Copy link
Member

Pyright is just complaining about the mismatch in parameter names. PEP 544 predates PEP 570 and isn't very precise about positional-only arguments. Pyanalyze gives an error for the same reason, but accepts the second assignment if you make all the parameters positional-only. That's a topic that might be worth clarifying in the PEP.

Mypy has a bug here: python/mypy#4536

@cdce8p cdce8p deleted the fix-pep0544 branch February 16, 2022 17:17
@cdce8p
Copy link
Contributor Author

cdce8p commented Feb 16, 2022

Thanks for taking the time to explain it. At first glance it just looked strange. Then after both type checkes failed, the most reasonable thing for me was an error in the example. Especially for such an old PEP which I had assumed to be fully implemented by now.

@Kentzo
Copy link

Kentzo commented Oct 21, 2022

@JelleZijlstra I'm looking at the definition of C and I'm completely missing how C conforms to ProtoB but doesn't conform to ProtoA. Clearly, C.meth doesn't match ProtoB.meth. Current mypy seems to agree as well:

test.py:13: error: Incompatible types in assignment (expression has type "C", variable has type "ProtoB")
test.py:13: note: Following member(s) of "C" have conflicts:
test.py:13: note:     Expected:
test.py:13: note:         def meth(self, obj: Any, x: int) -> int
test.py:13: note:     Got:
test.py:13: note:         def meth(self, x: int) -> int

Are you sure it's not a typo?

@JelleZijlstra
Copy link
Member

C.meth does match ProtoB.meth, because you would call it as C.meth(some_instance, x). Sounds like you found a bug in mypy.

@Kentzo
Copy link

Kentzo commented Oct 22, 2022

I see. Is it too late to slightly augment the example:

a_type: Type[ProtoA] = C  # OK
a_inst: ProtoA = C  # Type check error, signatures don't match: 

b_type: Type[ProtoB] = C  # Type check error, signatures don't match!
b_inst: ProtoB = C  # OK

@JelleZijlstra
Copy link
Member

I think that's fine, though I'll let Ivan have the final say as the PEP author.

@CAM-Gerlach
Copy link
Member

@ilevkivskyi ☝️

@ilevkivskyi
Copy link
Member

PEPs aren't supposed to be "live" documents (nor pedagogical tutorials), once accepted they can be amended only if there is an ambiguity. In this case everything is unambiguous.

About the mypy bug: can you please give a full example that works incorrectly on current mypy master?

@JelleZijlstra
Copy link
Member

PEPs aren't supposed to be "live" documents (nor pedagogical tutorials), once accepted they can be amended only if there is an ambiguity. In this case everything is unambiguous.

While that's true, the current example is counterintuitive enough that it has confused several people who were experienced with typing. I think it's fine to extend the example to make the point more clear.

@CAM-Gerlach
Copy link
Member

For what it's worth as a fellow PEP editor/cheerleader of the "Final PEPs are not living documents" refrain, the way I see it is if a PEP is an interoperability standard, there isn't anywhere that is a living document to point users to (with our new "This PEP is a historical document/See the canonical documentation/specification..." banner), and a specific clarifying example is causing a continuing stream of questions/confusion seeking to clarify it that a modest tweak that doesn't substantively alter the PEP's normative content would alleviate then it would seem justified to me to amend the example. This not only clarifies it for readers, but also reduces the burden on you and the PEP editors of responding to such (which is one of the motivations for the policy in the first place).

Also, FWIW as a typing user, not an expert, I was still not totally clear on this even with Jelle's explanation here until I read the proposed revision of the example.

@ilevkivskyi
Copy link
Member

The PR as it is now, is just wrong, I assume you refer to #2332 (comment). I am not sure adding another (relatively tricky) example will help understand this one, it may just cause more similar questions. Instead adding a note before like (note r.h.s is a class object, not an instance) will be better (maybe also change variable names to class_a and class_b).

@Kentzo
Copy link

Kentzo commented Oct 24, 2022

FWIW, the example I wrote was based on my immediate experience after reading the pep for the first time. I preferred it because it covers all relevant use cases and eliminates guesswork.

Comments for the lines could have been better: instead of a short ok and ni just plainly explain whys and whynots.

@CAM-Gerlach
Copy link
Member

I assume you refer to #2332 (comment).

Yes

Instead adding a note before like (note r.h.s is a class object, not an instance) will be better

Sure, and I would perhaps be even more clear than that and explicitly mention the change in how it is called

@rodrigodesalvobraz
Copy link

rodrigodesalvobraz commented Dec 5, 2023

I thought CAM-Gerlach made a very good argument for the clarification of this extremely confusing example in the PEP, so I am surprised to not see such clarification more than a year later.

Separately from that, I also find the following sentence confusing: "A class object is considered an implementation of a protocol if accessing all members on it results in types compatible with the protocol members." I would interpret "a class object is considered an implementation of a protocol" to mean that that class is a subtype of the protocol (the phrase "a class that implements an interface" is common). But in this example C is acting as an instance, not a subtype, of ProtoB, because it is being assigned to a ProtoB-typed variable. The fact this seems to say that C is a subtype of ProtoB significantly favors the confusion this thread is about.

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Dec 5, 2023

We didn't change this PEP because the PEP author didn't want a change. However, we'll soon have a canonical typing spec elsewhere (python/typing#1517), and there we'll definitely accept clarifications like this.

@rodrigodesalvobraz
Copy link

The last message from the author, at least in this thread, seems open to the idea of modification. Did he later (perhaps outside the thread) decide not to change anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants