When adding annotation to an interface it also apply to the implemented function in classes implementing this interface? Same for abstract functions?

Than it must be done by phan to known the inherit and interfaces.

The idea presented here sounds nice. However, I am concerned.

The relevant piece really is the @internal tag. Stuff that's declared @internal is forbidden to be used in …, yea, when is it really forbidden?

Marking code with @api can be an indicator that tells everybody "this is intended to be used from the outside". Whatever "outside" means. I feel the same can be achieved by just describing the intended usage of a class in it's top-level documentation.

Adding the suggested tags only makes sense if there are tools that can understand what they mean, and warn if classes are used in contexts they are not meant to be used in. Without such tooling all we get are a few thousand copy-pasted @api and @internal tags all over the place, but nobody knows if they are correctly placed or not, and nobody knows if they are respected or not.

I feel I am missing the crucial element of this idea. Can you help me understand?

Now that we have a Stable interface policy this becomes even more important so that we don't need to waste time on looking up if a method used in a patch is in fact supported for use.

See the policy for details, but an initial version could be just to warn if a method or class tagged @internal is called or constructed. We could file additional tasks for less common and/or more complex use cases (such as overriding a method that isn't abstract or explicitly @stable to override, etc.).

In T209394#6871970, @Krinkle wrote:

See the policy for details, but an initial version could be just to warn if a method or class tagged @internal is called or constructed.

Phan has several issue types for this, but we disabled them in our config, see r546356 and the linked phan issue. Long story short, phan assumes that the scope of an @internal is delimited by the namespace, whereas for our use case, it should be the repository (core or extension or skin), I think. This is something that should be fixed upstream.

In T209394#6871970, @Krinkle wrote:

See the policy for details, but an initial version could be just to warn if a method or class tagged @internal is called or constructed. We could file additional tasks for less common and/or more complex use cases (such as overriding a method that isn't abstract or explicitly @stable to override, etc.).

Some of these might actually turn out to be easier than @internal: for instance, @stable to override is unequivocal, as it doesn't depend on the namespace, repo, etc. Same for @newable, @stable for xxx, perhaps others. The downside is that we have to write some boilerplate for a custom plugin, but that's still minimal/easy.

Also, this should not be implemented with PHPCS.

In T209394#6874363, @Daimona wrote:

Phan has several issue types for this, […] Long story short, phan assumes that the scope of an @internal is delimited by the namespace, whereas for our use case, it should be the repository (core or extension or skin), I think. […]

I think that's historically accurate, but not by design, and not as much as it used to be. For example, there are core components as well, and internal generally does not mean it should be used outside that component. It's just that we historically haven't had namespaces, and so repos are a better approximation if we have nothing else to fallback to.

If we use this rule as-is, I think it would work correctly both between core and extensions, between extensions, and between namespaced features within core. The on area where the would be "wrong" is if the core class and extension class are both global and thus implicitly in the same namespace. The
wrong-ness there would be in the form of it allowing the use, so that's fairly harmless (no noise or overrides needed).

It would make a good starting point, perhaps?

We may have a few cases in core where two components are both namespaced but e.g. component A has @internal For use in thing B, in which case B would likely need to provide a phan comment on its side to recognise that it is using something internal across components and does so intentionally. That's a good thing and discourages such things. This is similar to other cases where we intentionally ignore a best practice but accept the risk. E.g. we find that we're using some internal core methods in a bundled extension at own risk. We'll need to acknowledge those somehow as well, at least temporarily. That seems worthwhile, and even useful.

Change 667926 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan@master] Enable issues for @internal

https://gerrit.wikimedia.org/r/667926

In T209394#6875989, @Krinkle wrote:

In T209394#6874363, @Daimona wrote:

Phan has several issue types for this, […] Long story short, phan assumes that the scope of an @internal is delimited by the namespace, whereas for our use case, it should be the repository (core or extension or skin), I think. […]

I think that's historically accurate, but not by design, and not as much as it used to be. For example, there are core components as well, and internal generally does not mean it should be used outside that component. It's just that we historically haven't had namespaces, and so repos are a better approximation if we have nothing else to fallback to.

If we use this rule as-is, I think it would work correctly both between core and extensions, between extensions, and between namespaced features within core. The on area where the would be "wrong" is if the core class and extension class are both global and thus implicitly in the same namespace. The
wrong-ness there would be in the form of it allowing the use, so that's fairly harmless (no noise or overrides needed).

It would make a good starting point, perhaps?

We may have a few cases in core where two components are both namespaced but e.g. component A has @internal For use in thing B, in which case B would likely need to provide a phan comment on its side to recognise that it is using something internal across components and does so intentionally. That's a good thing and discourages such things. This is similar to other cases where we intentionally ignore a best practice but accept the risk. E.g. we find that we're using some internal core methods in a bundled extension at own risk. We'll need to acknowledge those somehow as well, at least temporarily. That seems worthwhile, and even useful.

I agree with your comment, especially the part about acknowledging special cases, which is why I originally enabled this issue type in our config when it was made available. If namespaces as boundaries are acceptable per the SIP, then we can re-enable the issues. The patch above does that, you can view the result on MW core here (note, there's some noise due to other issues). There are roughly 1300 issues, most of which seem to be caused by HookRunner/HookContainer and User.

In T209394#6876161, @Daimona wrote:

most of which seem to be caused by HookRunner/HookContainer and User.

For User, this seems to be caused by the constructor being marked as @internal; for HookRunner, the whole class is internal. Unless we can improve this in our documentation (e.g. replace @internal with something else), I guess this might be fixed upstream, e.g. by implementing an option to switch this behaviour.

Change 667926 abandoned by Daimona Eaytoy:

[mediawiki/tools/phan@master] Enable issues for `@internal`

Reason:

https://gerrit.wikimedia.org/r/667926

Parsoid's policy has traditionally been that code outside of Wikimedia\Parsoid\Ext is internal to Parsoid and not to be used by outside code (T332457: Define stable interface policy for Parsoid). There are lots of current violations of this policy though, because it is not enforced by tools. It would be great if we could get per-repository @internal, even if we need to change the name. Per-namespace @internal would be awkward, but we'd probably solve that for turning off the check when running phan locally for Parsoid -- this wouldn't detect Parsoid violating the policy of its *child* dependencies, but Parsoid is intended as a leaf library anyway.

Per-namespace @internal would work if we could define a set of "friend" namespaces. That is, when checking Parsoid, all namespaces under Wikimedia\Parsoid\* are considered "friends" and can call @internal methods defined in Wikimedia\Parsoid\*. (That is, Wikimedia\Parsoid\Wt2Html is a friend of Wikimedia\Parsoid\Utils and they can call each others @internal methods.) However, a call from Parsoid to an @internal method of \MediaWiki\Title (something outside the friend universe) would be flagged.

When run in mediawiki-core, a set of MediaWiki\* namespaces would be friends, but a call to an @internal method of Wikimedia\Parsoid would be flagged.

For our use case we'd also want to be able to flag entire classes as @internal, since our policy is that outside code shouldn't call anything in (eg) Wikimedia\Parsoid\Wt2Html\SomeClass.php; we don't want to have to mark each individual method as @internal, just once at the top of the class.

As an alternative to "friend namespaces" this could also be done by specifying file paths which are considered "friends" for the sake of @internal (aka, are allowed to call @internal methods of other classes defined under that same file root). For example, in the Parsoid repo, everything under src, tools and tests would be considered friends; 3rd party code which is not part of the friend set would typically be located under vendor or some ../ path which points to the MediaWiki install. Given that we are trying to standardize on PSR-4, these are probably roughly equivalent, but extensions and skins often use MediaWiki\* namespaces even though they shouldn't be calling MediaWiki internals. You'd have to explicitly enumerate the namespaces until MediaWiki which are friends, which might be more annoying than using a filesystem-based discriminator.

Change #1304641 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/tools/phan@master] Add CrossPackageInternalPlugin and RequireInternalPlugin

https://gerrit.wikimedia.org/r/1304641

Implemented a version of this check; described in more detail at https://www.mediawiki.org/wiki/Continuous_integration/Phan/Plugins

In T209394#12037436, @cscott wrote:

When run in mediawiki-core, a set of MediaWiki\* namespaces would be friends, but a call to an @internal method of Wikimedia\Parsoid would be flagged.

Not all of it – MediaWiki\Extension\* and MediaWiki\Skins\* should definitely not be friends, but there are also some extensions and skins that don’t have the Extension/Skin subnamespace, e.g. Babel chose MediaWiki\Babel\* and Minerva Neue is MediaWiki\Minerva\*.

In T209394#12038075, @Tacsipacsi wrote:

In T209394#12037436, @cscott wrote:

When run in mediawiki-core, a set of MediaWiki\* namespaces would be friends, but a call to an @internal method of Wikimedia\Parsoid would be flagged.

Not all of it – MediaWiki\Extension\* and MediaWiki\Skins\* should definitely not be friends, but there are also some extensions and skins that don’t have the Extension/Skin subnamespace, e.g. Babel chose MediaWiki\Babel\* and Minerva Neue is MediaWiki\Minerva\*.

The way I've implemented it in the patch linked above is based on the nearest composer.json walking up the tree. Extensions and skins have their own composer.json so they are considered separate "modules".

In T209394#12038874, @cscott wrote:

Extensions and skins have their own composer.json

Do they always? Babel and Minerva Neue do, but I don’t remember this being a requirement (extension.json/skin.json is nowadays required, but I don’t remember the same being true for composer.json).

I could easily add a check for extension.json and skin.json. That's more MediaWiki-specific, but this is a MediaWiki-specific plugin at this point (living in mediawiki-phan-config) anyway.

Change #1328330 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/tools/phan@master] Add RequireInternalPlugin

https://gerrit.wikimedia.org/r/1328330