Page MenuHomePhabricator

RFC: Amendment to the Stable interface policy, June 2020
Closed, ResolvedPublic

Description

Motivation

On several occasions, I struggled to explain the stability interface policy approved in T193613. I noticed at times that a point I made wasn'tt easily linkable on the page. My interpretation wasn't contradictory per se, but it also wasn't obvious that what I said is how it was meant to be intepreted. There are areas that contain gaps or are split between sections. I'd like to be able to quickly find (or link to) what the policy is for a given scenario, and what one's options would be.

I started drafting an overhaul of that page in which I simplify the language, make the structure easier to digest for wide ranger of audiences (core dev, extension dev, contributor, code reviewer, etc.). I also accomodated the new hooks system per T240307 with some pointers.

I considered publishing this as non-nominal change, but alas, I found some inconsistencies and blank spots I was unable to write clearly without being a nominal change – hence this RFC.

Requirements
  1. Resolve ambiguity over what to do with classes that are stable to construct ("stable to call", 'stable to instantiate", "newable").
    • The policy describes "stable to instantiatie" as as being distinct from "stable to call". However later descriptions of "stable to call" also cover constructor methods. It is not obvious what the requirement annotation(s) are or where they should be put.
  2. Explain why stable constructors are discouraged.
    • The policy mentions "dependency injection", but given we do allow stable constructors in some cases I think we should explain in a sentence or two why an author would want want to avoid it.
  3. Resolve contradiction about whether abstract methods of an extendable class are considered stable to override without the need for an annotation.
    • The policy says "stable for overriding: … This annotation should be applied to all abstract methods of classes marked as stable for subclassing". But, it also says "Only methods explicitly marked this way (or declared abstract) are safe to override".
  4. Explain why interfaces are discouraged.
    • The policy mentions "abstract base classes", but does not explicitly state what problems this avoids or solves.
  5. Define what expectations are put on the author (e.g. MW core) for methods that are "stable to override".
  6. Document the status quo that hard deprecation may not happen until WMF production no longer uses the code path in question, or decide otherwise.

Exploration

Notable changes:

  • Section "Stable to call" now includes constructor methods with @stable to call. It now explains the ceveat with a stable constructor in relation to dependency injection. The @newable annotation is explained as optional marking for the class block.
  • Section "Stable to override" now includes "Methods that are declared as abstract".
  • Section "Stable to extend" now explains the caveat with interfaces as extension points in its recommendation for base classes.
  • Section "Stable to override" now defines a contract between core and class method overrides. The signature won't break, the method will continue to exist, and it will continue to get called.
  • Section "Deprecation" now includes "Wikimedia production" in addition to non-deprecated MW configuration and bundled extensions/skins in its list of areas that must be updated before hard-deprecation may occur.
  • Detailed changelog at: https://www.mediawiki.org/wiki/User:Krinkle/Stable_interface_policy/Changes.

Event Timeline

Krinkle created this task.Jun 18 2020, 5:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2020, 5:45 PM
Krinkle moved this task from P1: Define to P3: Explore on the TechCom-RFC board.Jun 18 2020, 5:45 PM
Krinkle updated the task description. (Show Details)Jun 22 2020, 12:03 AM

[…]
I think the answer to this question is that the interface policy requires (among other items):

  1. MediaWiki core code that triggers hard deprecation warnings MUST NOT be reachable from non-deprecated core code using non-deprecated configuration settings
  2. Extensions and skins bundled with the MediaWiki tarball MUST NOT trigger hard deprecation warnings and MUST be updated to use the new code.

The class cannot be hard-deprecated until its uses are removed or refactored to meet these conditions. See also Daniel's comment.

(Probably the policy should instead or also say that software deployed to Wikimedia servers MUST NOT have hard deprecation warnings, since there are extensions/skins deployed to Wikimedia that are not in the tarball.)

That's an excellent point @Izno, I've added it to this amendment. (Thanks to @daniel for pointing me to this comment.)

Krinkle updated the task description. (Show Details)Jun 24 2020, 9:28 PM
daniel added a comment.EditedJun 26 2020, 11:23 AM

Per the TechCom meeting on June 24, this proposal enters the Last Call period. If no concerns remain unaddressed by July 8, the draft will replace the existing policy.

Not included:
Any method or function marked @deprecated, @internal or @unstable.

All classes and interface, except those marked @deprecated, @internal or @unstable.

Exceptions:
Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

Stable to type

I can't avoid thinking of typing (writing) when I read this. Could it be called Stable to type-hint or Stable to as a type?

Note that there is currently hard-deprecation for the removal of stability guaruantees.

Unclear statement (plus typo). Does this mean that Stability guarantees can only be removed following the deprecation policy?

This is because is not possible to use deprecation in an interface.

Slightly confusing. The real reason is that it is not possible to make changes to an interface without breaking stability guarantees.

If you mark an interface as stable to implement, you commit to never changing its method signatures, and never adding new methods – unless the interface as a whole is deprecated first.

Perhaps this could be simplified to: Stable interfaces cannot be changed. You can only add a new interface and deprecate the old one.

The @stable annotations can be followed by a Deprecated since segment to indicate that a particular use of the class or method is currently deprecated.

IDEs won't pick this kind of deprecation, to my knowledge. Why not just require to use @deprecated annotation?

A minor gripe (that is not introduced in this version) is that the policy claims to apply to extensions as well, but for many parts it is no applicable (e.g. it makes no sense to mark deprecations against MediaWiki core version for extensions that use their own versioning). Ideally, we would limit this to MediaWiki core, or make it more general, or clarify which parts apply only to core.

daniel added a comment.EditedJun 26 2020, 2:46 PM
Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

These three are not the same: @deprecated means it used to be stable, but will go away in the future. @internal means it is not designed to be used across module boundaries. @unstable means it is not yet ready to be used across module boundaries, but is intended for such use.

Perhaps this could be simplified to: Stable interfaces cannot be changed. You can only add a new interface and deprecate the old one.

That would be more restrictive. Currently, you can remove a method, after deprecating it. There is just no way to enforce that all implementations would deprecation warnings.

But perhaps we can do this anyway.

IDEs won't pick this kind of deprecation, to my knowledge. Why not just require to use @deprecated annotation?

Because then IDEs will pick up on too many usages, including legit ones. E.g. we may want to take away the ability to directly call a constructor from an extension in the future, but we still need to call that constructor in a factory in core. Or you want to deprecate the ability to override a method, but you still want to allow that method to be called. @deprecated would go too far...

Stable to type

I agree with Niklas that it's unclear what the verb "to type" is intended to refer to here.

The old policy uses "@unstable for implementation" for this purpose. I realize that this isn't very pretty, but it seems more clear than "stable to type". Also, "@unstable for implementation" is usually followed by instructions about what to do instead, similar to how we filly @deprecated with instructions. E.g.

@unstable for implementation, extensions should sublcass FooBarBase.

How would we do that with @stable to type? The best I can come up with is this:

@stable to type
@note Extensions should sublcass FooBarBase.

Not included:
Any method or function marked @deprecated, @internal or @unstable.

All classes and interface, except those marked @deprecated, @internal or @unstable.

Exceptions:
Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

Do you mean the first part of these three sentences being different, or that we have three annotations that all mean "not stable"?

If the former, these seem consistent to me. They were copied from sections about three different things. I think it would be confusing to enumerate all possible PHP entities when the context is limited to class/interfaces for a given stability kind, etc.

If the latter, as Daniel says, these are three distinct expressions explained under "Remove guarantees".

  • @deprecated means it was stable but will cease to be in the future. (supported by Phan and various IDEs)
  • @internal means it is not stable and should only be used within the same component/namespace. Outside of it you are at risk of it changing any time without notice. (supported by Phan)
  • @unstable is less comon and could potentially be folded into @internal. The main difference is that @unstable means the method is designed for public usage so unlike @internal it is not something you should just remove if its unused internally because it's meant to be publicly consumed elsewhere. It just wants to opt-out from any guruantees, and is considered experimental/alpha at this point. This way also Phan won't flag usage it as breach of internal APIs. It signals that you're accepting the responsibility as early adopter to keep up with changes.

In any event, these three already existed in the policy today and aren't nominally part of this RFC, so I don't consider this blocking the current RFC as such, but it's good to think about this. We may want to file a separate ticket to continue that conversation.

"Note that there is currently hard-deprecation for the removal of stability guaruantees."

Unclear statement (plus typo). Does this mean that Stability guarantees can only be removed following the deprecation policy?

Thanks! This was missing the word "no". Fixed, and typo fixed.

"Stable to type"

I can't avoid thinking of typing (writing) when I read this. Could it be called "Stable to type-hint" or "Stable to [use] as a type"?

Typing means to deal with the type of something. e.g. "Typing against Foo" means setting it as the type of a class property, function parameter, or return value. Or checking the type of a value via instanceof, or using Assert to do so etc. Examples in usage:

I've personally only seen "type hint" used in the context of function arguments. E.g. not in context of return types, variable/property types (e.g. for Doxygen or Phan), or in assert/instanceof checks.

I didn't know this before but the php.net docs say that "type hint" is a PHP5-era term specifically for argument type declarations. As of PHP 7 it refers to these more generally as "type declarations" instead, which covers return types as well (doc link).

Would you prefer @stable to type hint, @stable to type-hint or @stable to hint instead? If that's clearer and preferred by others, I'd be fine with that. Note that they're meant to be verbs, and I was aiming for each being a single word, but that might be hard in English :)

@unstable for implementation, extensions should subclass FooBarBase.

How would we do that with @stable to type? The best I can come up with is this:

@stable to type
@note Extensions should sublcass FooBarBase.

I'm not sure I follow.. There was no rule against adding a sentence after a stability annotation. So if @unstable for implementation Some text here was fine before, I suppose @stable to type Some text here is just as well!

/**
 * @stable to type To create your own class, extend FooBarBase instead.
 */

Personally I tend to place this into the main doc block, though. Something as major as an extension point might deserve more than 1 line of text buried after an annotation.

/**
 * Common interface for FooBar handler.
 *
 * This exists to separate caller concerns between FooBar and FooQuux and should be used
 * in type declarations only.  To create your own handler, extend the stable class FooBase instead.
 *
 * @since 1.35
 * @stable to type
 * @ingroup Foo
 */

Anyway, that's well outside the territory of development policies. Certainly either is allowed no matter which way we go.

  • @internal means it is not stable and should only be used within the same component/namespace. Outside of it you are at risk of it changing any time without notice. (supported by Phan)

Something that has come up in some code reviews - can an @internal method in core be used elsewhere in core outside of that "component/namespace"? (same probably applies for extensions).

Not included:
Any method or function marked @deprecated, @internal or @unstable.

All classes and interface, except those marked @deprecated, @internal or @unstable.

Exceptions:
Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

Do you mean the first part of these three sentences being different, or that we have three annotations that all mean "not stable"?

Sorry I was specific, I meant the placement of these sentences, rather than the content themselves. One is in "not included" section, one is in "exceptions" section and one is in "included" section.

Not included:
Any method or function marked @deprecated, @internal or @unstable.

All classes and interface, except those marked @deprecated, @internal or @unstable.

Exceptions:
Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

[…] I meant the placement of these sentences, rather than the content themselves. One is in "not included" section, one is in "exceptions" section and one is in "included" section.

Aye, gotcha. No problem. There's no reason to vary that indeed. I've patched it up in the latest revision. They all use "Not included" now.

daniel added a comment.EditedJun 29 2020, 9:04 AM

I'm not sure I follow.. There was no rule against adding a sentence after a stability annotation. So if @unstable for implementation Some text here was fine before, I suppose @stable to type Some text here is just as well!

Yes, but it's awkward. "@unstable to implement, use base class" is one sentence with a clear intent. "@stable to type. Use base class" is two sentences, and the connection of the two is unclear. If it's stable for my to type against, why should I use the base class instead?

Would you prefer @stable to type hint, @stable to type-hint or @stable to hint instead? If that's clearer and preferred by others, I'd be fine with that. Note that they're meant to be verbs, and I was aiming for each being a single word, but that might be hard in English :)

How about @stable to type against? Or @stable to reference. Maybe even @stable to reference only.

By the way, we have quite a few usages of @private, apparently applied with the intent of @internal. I propose to just replace them in the code base.

daniel added a comment.EditedJun 29 2020, 12:02 PM

I'm just going through all exceptions to mark them as newable, and their constructor stable to call... While it's good to have that for clarity, should we declare all extensions newable per default? Or will adding exceptions to the rules make things confusing?

By the way - many exceptions don't declare a constructor. In that case, the @newable tag stands without @stable for calling...

Something that might be helpful to include is stability of public constants, since those cannot emit deprecation warnings when deprecated. Eg for the removal of the Revision class, it has public constants that. How should those be deprecated?

daniel added a comment.Jul 2 2020, 9:27 PM

Something that might be helpful to include is stability of public constants, since those cannot emit deprecation warnings when deprecated. Eg for the removal of the Revision class, it has public constants that. How should those be deprecated?

Good question. I suppose we just use soft deprecation there, because there is no way to implement hard deprecation...

daniel added a comment.Jul 2 2020, 9:29 PM

I just remembered an important distinction between @unstable and @internal: An interface or class that is marked @internal should not be used in a public interfaces, since code outside the repo should have no knowledge of them. Interfaces or classes marked as @unstable can be used in signatures of public methods, and code outside the repo may have knowledge of the type, but should not rely on members and methods to be stable (yet).

daniel added a comment.EditedJul 3 2020, 2:24 PM

While applying @stable to override annotations, I realized that we have quite a few methods that are technically stable to override, but we don't want extensions to override them. This is typically the case if the method at hand is inherited from a parent class or interface, and implemented in a canonical way for the present class. Such a method is technically stable for overriding, because the abstract definition in the parent class is. But we don't want to encourage extensions to override them. We really want to tell them not to.

My suggestion is to use @final in these cases. That makes it clear that extensions shouldn't override the method, even though it would be safe to do so, at least in terms of the method signature. We don't want to use PHP's native final keyword, since that interferes with phpunit's mocking framework.

I think this might be conflating stability with usage pattern. @stable to override doesn't mean that you're required to change it. Only that you can. Naturally methods that are abstract are required to be implemented as otherwise PHP does not allow instantiation.

What is an example of a stable, abstract, base class with abstract methods that require an implementation that we don't want extensions to implement?

Krinkle claimed this task.Jul 8 2020, 8:23 PM
Krinkle moved this task from Untriaged to Implemented on the TechCom-RFC (TechCom-RFC-Closed) board.
Krinkle closed this task as Resolved.Jul 8 2020, 8:27 PM

We should probably update the part of the deprecation process that mentions hooks.txt, which is obsolete. See https://www.mediawiki.org/wiki/Topic:Vqaxd8a785zc8u88

daniel renamed this task from RFC: Amendment to the Stability interface policy to RFC: Amendment to the Stable interface policy.Aug 28 2020, 8:24 AM
daniel renamed this task from RFC: Amendment to the Stable interface policy to RFC: Amendment to the Stable interface policy, June 2020.Nov 20 2020, 11:32 AM