Various improvements to provisional.py

[aeliasso]

• abc.abstractproperty is deprecated. Fix by using property and abstractmethod instead.
• Fix the type of the stable property.
• Fix the property overriding syntax.
• Start the text in the provisional features' docstrings with a blank line to make it render as a paragraph block. Previously the backticks would not render as typewriter text.
• Fix type hints in parse_provisional.
• Reformat the file using Black.

[lwaern-intel]

I have a significant knee-jerk dislike of this. Why do this instead of stable = True, etc. -- what's the benefit?

[aeliasso]

See microsoft/pyright#5564

As I understand it, it's how properties are supposed to be overridden. @abstractproperty is a deprecated way of defining a read-only property which has been replaced with the syntax: @property @abstractmethod. ( https://docs.python.org/3/library/abc.html#abc.abstractproperty )

Since it's a property, it needs to be overridden in subclasses. If assigned using = I think you are instead replacing the read-only property with an attribute which is read-write.

[lwaern-intel]

From that thread:

I guess there's no way for an ABC to indicate that a class variable is required.

Buh. I really want to keep my stable = True syntax; if there is no go-to way to express mandatory class variables, we have to roll our own. I guess we could do that in feature. Something like:

def feature(cls: type[ProvisionalFeature]):
    assert issubclass(cls, ProvisionalFeature)
    for (attr, type) in (('__doc__', str), ('short', str), ('stable', bool)):
        assert hasattr(cls, attr) and isinstance(getattr(cls, attr), type), attr
    singleton = cls()
    features[singleton.tag()] = singleton
    return singleton

@mandolaerik Your opinion?

[mandolaerik]

If assigned using = I think you are instead replacing the read-only property with an attribute which is read-write

No. Consider:

class X:
  foo = 1
class Y:
  @property
  def foo(self): return 1

In X, foo is available on class level, in Y the value is only available from the instance. You can say X.foo = 2 to change the class member, and you can shadow the class member in one specific instance by X().foo = 3. In Y, you can also change the class member, Y.foo = 4. Y().foo is indeed a read-only expression, in the sense that Y().__setattr__ rejects foo. However, the same is true for X if you seal the set of members through e.g. __slots__.

So, if you insist that properties are kept read-only on instance level, then use __slots__ or equivalent (e.g. dataclass). Or, if you want hard guarantees that thiings you intend as immutable stay immutable, then don't use Python.

[aeliasso]

I think I get it now. You are only using these classes at the class level, not as instances. I didn't realize. In that case my change is indeed incorrect. Also in that case, I think the second part of the comment quoted is applicable:

It sounds like you're really looking for a way to specify a structural subtype. A Protocol may serve your purposes better than an ABC.

I have to read up on Protocol before I have an opinion on that. Assuming you want to keep the code as is, however, then I would like to turn the question around: Why do you declare these class members as properties in the abstract base class? To cause an error if a subclass forgets to override them, e.g. because the property is not printable?

Erik, in you example:

>>> class Y:
...   @property
...   def foo(self): return 1
...
>>> type(Y.foo)
<class 'property'>
>>> Y.foo=2
>>> type(Y.foo)
<class 'int'>

Y.foo is now an int and the original property is no more. It has ceased to exist. It is a late property. If this is the expected use of the abstract base class, then why declare a property in the first place?

[mandolaerik]

My point with Y.foo=2 is that python is hopelessly dynamic: even though the property is read-only in one sense, it's still possible to lift that protection. Nothing is truly protected. In particular, you might say:

class X:
  @property
  @abstractmethod
  def foo(self): pass

with the intention that overrides should be read-only as in:

class Y(X):
  @property
  def foo(self): return 1

but Python will happily swallow if someone says

class Z(X):
  foo = 2

Thus, a user of X does not have an immutability guarantee on foo.

[lwaern-intel]

I'm not inherently opposed to this, but neither me nor Erik are using Black. You are free to use it, of course -- but enshrining it into the repo feels a bit much. The only one style guide DMLC is beholden to is Intel's own. I'd be OK with you putting pyproject.toml in the .gitignore though.

[mandolaerik]

I didn't know about Black before. I'm open to try enforcing it for the DMLC repo, but it doesn't belong to this PR.

More specifically: Feel free to blackify all python files in the DMLC repo. The repo is small enough that we probably can sweep through the diff manually and see if it's a net win. If it is, then let's pioneer it by enforcing it with a test.