Undocumented usage of reserved dunder names

[kevinsung]

Informations

• Qiskit Experiments version: 0.2.0
• Python version: N/A
• Operating system: N/A

What is the current behavior?

The JSON serialization framework (https://github.com/Qiskit/qiskit-experiments/blob/a0beb41ec0757429031e0e6e7a7855dcc72877cd/qiskit_experiments/framework/json.py) relies on classes defining their own methods called __json_encode__ and __json_decode__. These names are bad choices because they are explicitly documented as subject to breakage in future Python versions. From https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers:

__*__
System-defined names, informally known as “dunder” names. These names are defined by the interpreter and its implementation (including the standard library). Current system names are discussed in the Special method names section and elsewhere. More will likely be defined in future versions of Python. Any use of __*__ names, in any context, that does not follow explicitly documented use, is subject to breakage without warning.

Steps to reproduce the problem

N/A

What is the expected behavior?

Reserved dunder method names should not be used in undocumented ways.

Suggested solutions

The method names __json_encode__ and __json_decode__ and any other such names in the library should be changed. I suggest using a single underscore instead of a double, i.e. _json_encode_.

[mtreinish]

Honestly I think this is a perfectly acceptable use of a dunder method. The python docs are warning that from their perspective the __*__ namespace is considered fair game for future protocol usage and any future python release can add on to it. However, it is this nature of protocol methods that makes it a pretty normal choice for libraries to implement their own protocol methods (ie __array__ in numpy) outside of what python itself defines. In practice the risk here is pretty minimal because if cpython decided to add a conflicting dunder name out of the infinite choice of names (which is honestly pretty unlikely to happen) because cpython is an open project we could discuss it with them prior to its release to push for a different name, and if that was unsuccessful we'd have time to adjust our code since it'd be only in a future python release (as a new feature it wouldn't be backported). So we could guard against the conflict with sys.version until we can complete a migration to a non-conflicting name. That being said this is just a hypothetical scenario and I find it super unlikely that cpython would decide to add a __json_encode__ and __json_decode__ protocol method to stdlib.

[kevinsung]

The choice to use dunder method names affects not only this particular protocol, but any future ones we decide to add to this library, and other Qiskit libraries if we want to be consistent across Qiskit. So while these specific names may be unlikely to break, relying on future name choices to also be unlikely to be broken is somewhat tenuous.

The choice of protocol name affects not only this library, but any user code or other libraries that implement this protocol. If the protocol is broken, their code would also be broken, even if we update the names here.

[chriseclectic]

This isn't really a bug, it's a suggest change to an existing internal API. Since this is an API that is currently in use I don't see this as sufficient justification for introducing breaking changes to it. If it actually becomes a problem in future we can revisit then, but as @mtreinish said this seems quite unlikely, so for now I'm closing this issue.