Formalize exception handling and annotations in MediaWiki and enforce it in CI
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	Daimona
	Oct 26 2022, 12:53 PM

Description

The coding conventions say very little about exception handling in MediaWiki. In fact, the only relevant paragraph is:

Exceptions that indicate programming errors should be one of the exceptions that ship with PHP or a more specific subclass, while exceptions that indicate errors that are relevant to the end user should be an ErrorPageError or one of its subclasses.

Effectively, this means that exception handling is done differently in different areas or codebases, depending on who wrote or approved certain code changes. In particular, I would like to focus on two aspects:

What exception classes should be thrown
When and how to use @throws annotations

Over the past years, I've seen many commits introducing code that throws or handles exceptions, and the general consensus amongst developers (which actually goes beyond MediaWiki) is that:

You should never throw the base Exception class; instead, either use another exception available in the SPL, or define a new exception type.[1]
SPL exceptions (like InvalidArgumentException or UnexpectedValueException) are generally not meant to be caught.
@throws annotations should be reserved for checked exceptions, that is, exceptions that callers should either catch or document in their own doc comment.

Additionally, there seems to be some uncertainty as to when MWException should be used (either directly or as a base class) [2], whether it should be caught, etc. All in all, this produced inconsistent code based on varying assumptions, and such code cannot be validated statically (at least for what concerns exception handling).

Therefore, I am proposing that:

We agree on how exception handling should work in MediaWiki, potentially using the rules above (de facto, the status quo) as a starting point.
The new rules should be documented in the coding conventions, in a dedicated section about exception handling.
These rules should be enforced by linters (PHPCS) or static analysis tools (phan).
Possibly, the purpose of MWException should be documented and clarified, both in the class comment and in the coding conventions.

In particular, I think the linters should do the following:

Flag code that throws Exception (this can be done with PHPCS)
Ensure that if a callee has @throws annotations, the caller either catches those exceptions, or documents them in its own doc comment (phan can do this with the warn_about_undocumented_exceptions_thrown_by_invoked_functions config option)
Possibly: flag code that throws a checked exception but does not document it in the doc comment. Phan can do this with the warn_about_undocumented_throw_statements option; the problem is that phan doesn't know which exceptions are (un)checked. The config option exception_classes_with_optional_throws_phpdoc can be used to specify that, but I'm not sure if the current exception hierarchy distinguishes checked vs unchecked exceptions clearly enough for this.

I imagine a lot of code might have to be cleaned up, so the proposal is that we enable the new rules by default, but disable them on repos where they fail. They can then be re-enabled incrementally as noncompliant code is rewritten. Related: T240672: Clean up unchecked exceptions.

[1] A good question here would be if there are other exception classes that should not be thrown directly. Opinions about this seem to vary.
[2] As a matter of fact, the class name does not explain its purpose. The doc comment of that class only says "MediaWiki exception". I would say it's not incredibly helpful, is it?

Details

Subject	Repo	Branch	Lines +/-
[WIP] Bump phan/phan to 5.4.3	mediawiki/tools/phan	master	+1 -1
Bump phan/phan to 5.4.3	mediawiki/tools/phan/SecurityCheckPlugin	master	+6 -3
HTMLFileCache: Remove `@throws` for unchecked exception	mediawiki/core	master	+8 -13

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T321683 Formalize exception handling and annotations in MediaWiki and enforce it in CI
Resolved	Daimona	T338423 Disallow use of `new Exception`
Open	None	T86704 Phase out use of MWException
Open	None	T328220 Stop using MWException in Wikimedia deployed code
Resolved	matmarex	T353444 Remove custom error printing code from MWException, fall back to default MWExceptionRenderer
Duplicate	None	T355798 Four deprecation warnings from Flow
Open	Daimona	T338424 Enable phan rules to warn if a checked exception is not caught or documented
Open	Daimona	T338426 Enable phan rules to warn against checked exception that are thrown but not documented
Open	None	T338427 Create phan plugin to warn if an unchecked exception is documented with `@throws`

Event Timeline

Daimona created this task.Oct 26 2022, 12:53 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 26 2022, 12:53 PM

Mainframe98 awarded a token.Oct 26 2022, 1:43 PM

matmarex awarded a token.Oct 26 2022, 2:00 PM

matmarex subscribed.

dmaza subscribed.Oct 26 2022, 2:00 PM

This seems good. Is this something that should go through TDMP?

In T321683#8346005, @Jdforrester-WMF wrote:

This seems good. Is this something that should go through TDMP?

Good question, I'm not familiar with the process.

Also, here are examples of compliant and noncompliant code:

throw new Exception( 'Somethin bad happened' ); // Noncompliant: should use a more specific exception type. NOTE: make sure "Exception" is not an alias of something else...

function test() {
   throw new SomeCheckedException(); // Noncompliant: exception is not documented in the doc comment
}

/**
 * @throws SomeCheckedException
 */
function test2() {
   throw new SomeCheckedException(); // Compliant
}

function test3() {
   test2(); // Noncompliant: should either catch the exception or document it
}

function test4() {
   try {
      test2(); // Compliant
   } catch ( SomeCheckedException $e ) {
      //...
   }
}

/**
 * @throws SomeCheckedException
 */
function test4() {
   test2(); // Compliant
}

try {
   doSomething();
} catch ( InvalidArgumentException $e ) { // Noncompliant, this exception is not meant to be caught.
}

And finally, here's the output of phan on MediaWiki core with warn_about_undocumented_exceptions_thrown_by_invoked_functions enabled: P36582. As you can see, it catches a lot of things, and this is without warn_about_undocumented_throw_statements. We wouldn't have to fix all of them immediately, of course.

TK-999 subscribed.Oct 26 2022, 3:15 PM

Oh, and something I forgot to say. Another question would be how to document unchecked exception. For instance, if you have a method that throws InvalidArgumentException if given something invalid. Presumably, you'd want callers to know that the exception can be thrown without reading the implementation, even if the exception shouldn't be caught. @throws InvalidArgumentException would be disallowed in that case. So we should perhaps agree on a different way of documenting that.

In fact, it'd be great if we could get a linter (probably phan) to warn if @throws is used with unchecked exception types. I don't think this is currently possible, but I can request this feature upstream.

EBernhardson subscribed.Oct 26 2022, 4:31 PM

PatsagornY subscribed.Oct 26 2022, 4:40 PM

Michael subscribed.Oct 26 2022, 5:32 PM

Novem_Linguae subscribed.Oct 26 2022, 6:34 PM

WMDE-leszek subscribed.Oct 26 2022, 7:43 PM

Krinkle subscribed.Oct 26 2022, 9:51 PM

@Daimona wrote in the task description:

Additionally, there seems to be some uncertainty as to when MWException should be used.

The main value of MWException was to have a skinned and localised "Internal error" page rendered via our exception handler. I don't recall if it was originally introduced for that, but that's effectively what it ended up as. Over the past two decades, the PHP ecosystem (both built-ins and vendor libraries) have moved from warnings or custom callbacks towards proper exceptions. This meant an increasing scope of uncaught exceptions would return in near-blank page errors instead of "pretty" ones.

Since about 5 years now, the MException code has moved out of there and into the general exception handler (now named MWExceptionRenderer), and invoked by both the runtime exception handler (and entry point try-catch). I recall a drive from @ori, myself, and others (ref T86704) to remove usage of MWException. It was never useful in catch conditions (and afaik has no significant use as such), and for throwing or extending, I think we can use other exceptions instead. MWException isn't a useful middleground I think.

Traditionally, the unchecked exceptions are the subclasses of RuntimeException or LogicException (which includes all SPL exceptions). E.g. PHPStorm has that assumption built in.

Another thing that should be mentioned in the documentation is ILocalizedException and INormalizedException.

Joe subscribed.Oct 27 2022, 10:09 AM

In T321683#8347775, @Krinkle wrote:

@Daimona wrote in the task description:

Additionally, there seems to be some uncertainty as to when MWException should be used.

The main value of MWException was to have a skinned and localised "Internal error" page rendered via our exception handler. I don't recall if it was originally introduced for that, but that's effectively what it ended up as. Over the past two decades, the PHP ecosystem (both built-ins and vendor libraries) have moved from warnings or custom callbacks towards proper exceptions. This meant an increasing scope of uncaught exceptions would return in near-blank page errors instead of "pretty" ones.

Since about 5 years now, the MException code has moved out of there and into the general exception handler (now named MWExceptionRenderer), and invoked by both the runtime exception handler (and entry point try-catch). I recall a drive from @ori, myself, and others (ref T86704) to remove usage of MWException. It was never useful in catch conditions (and afaik has no significant use as such), and for throwing or extending, I think we can use other exceptions instead. MWException isn't a useful middleground I think.

Thanks for the useful context; I didn't realize we already had a task about it. In this case, I think we don't have to worry about MWException for this task, and talk about its future in T86704 instead.

In T321683#8347960, @Tgr wrote:

Traditionally, the unchecked exceptions are the subclasses of RuntimeException or LogicException (which includes all SPL exceptions). E.g. PHPStorm has that assumption built in.

I think that's a sensible default in general. I was just wondering if that assumption would hold true in MediaWiki, i.e.: do we have any checked exception that inherits from RuntimeException or LogicException? Also, since the default makes sense, we could simply just flag usages of (subclasses of) Runtime and LogicException with phan.

Another thing that should be mentioned in the documentation is ILocalizedException and INormalizedException.

I know pretty much nothing about them, so yes, it might be a good idea to document their usage :)

All in all, my proposal has become:

Add the following to a dedicated section of the PHP coding conventions:
- Never throw the base Exception class; instead, either use another exception available in the SPL, or define a new exception type.
- (Subclasses of) RuntimeException and LogicException represent unchecked exceptions, so they should not be caught, nor documented with @throws
- @throws annotations should be reserved for checked exceptions, that is, exceptions that callers should either catch or document in their own doc comment with @throws.
- TBD: documentation of unchecked exceptions, see below
These rules would be enforced by linters as follows:
- Flag code that throws Exception; possibly with PHPCS, but watch out for alias and namespaces. Otherwise, use phan.
- Flag @throws annotations used with unchecked exceptions (subclasses of RuntimeException and LogicException); should be done with phan, but this feature doesn't seem to exist. I can ask upstream and perhaps write a PR.
- Ensure that if a callee has @throws annotations, the caller either catches those exceptions, or documents them in its own doc comment (phan's warn_about_undocumented_exceptions_thrown_by_invoked_functions)
- Flag code that throws a checked exception but does not document it in the doc comment (phan's exception_classes_with_optional_throws_phpdoc)
These rules would be enabled by default for phan/phpcs, but would initially be disabled on repos with violations. As those violations are fixed, the rules would be incrementally enabled.

For me, the big question is how to document unchecked exceptions (T321683#8346466). @throws is not an option. I don't think the manual has to prescribe a specific syntax, but maybe it could suggest one. For instance, something like @note This throws XYZException if .... I'd really like to hear more thoughts about this, though.

Daimona mentioned this in T86704: Phase out use of MWException.Oct 27 2022, 11:13 AM

hashar awarded a token.Oct 27 2022, 3:27 PM

In T321683#8346466, @Daimona wrote:

[…] Another question would be how to document unchecked exception. […] @throws InvalidArgumentException would be disallowed in that case. So we should perhaps agree on a different way of documenting that.

In T321683#8348955, @Daimona wrote:

[…]
For me, the big question is how to document unchecked exceptions […]. For instance, something like @note This throws XYZException if ....

I thought that, implied in disallowing documententation for @throws about "must not catch" exceptions, that we don't want to document them. What's the use case for documenting this — in a different way? Who is the intended audience?

Over the past year, myself and overs have removed countless of such annotations without replacement. I generally found them needlessly repeating an implementation detail that can be gleaned from the code right below it already. Noting that of course, we're talking specifically about exceptions that aren't considered part of the stable API, and aren't meant to be caught or cared for by callers. I think in a handful of cases something useful was written after the @throws line, in which case I copied it to the method description or the relevant parameter description. E.g. if it was describing the required type or format of a parameter.

In T321683#8351552, @Krinkle wrote:

In T321683#8346466, @Daimona wrote:

[…] Another question would be how to document unchecked exception. […] @throws InvalidArgumentException would be disallowed in that case. So we should perhaps agree on a different way of documenting that.

In T321683#8348955, @Daimona wrote:

[…]
For me, the big question is how to document unchecked exceptions […]. For instance, something like @note This throws XYZException if ....

I thought that, implied in disallowing documententation for @throws about "must not catch" exceptions, that we don't want to document them. What's the use case for documenting this — in a different way? Who is the intended audience?

It would not be about documenting the exception type, but the fact that some preconditions are truly checked and not just assumed to be true. Especially in old code, it's not hard to find documented preconditions like "argument X should Y" that are not actually checked in the code. Old-style code may then ignore the preconditions because "it'll work anyway". By clearly stating that the preconditions are enforced (by throwing an exception), callers would know that they really need to validate the arguments. That said:

Over the past year, myself and overs have removed countless of such annotations without replacement. I generally found them needlessly repeating an implementation detail that can be gleaned from the code right below it already. Noting that of course, we're talking specifically about exceptions that aren't considered part of the stable API, and aren't meant to be caught or cared for by callers.

I do agree that it's an implementation detail, and that in a perfect world, there'd be no need to document it.

I think in a handful of cases something useful was written after the @throws line, in which case I copied it to the method description or the relevant parameter description. E.g. if it was describing the required type or format of a parameter.

Oh and yes, I'm talking specifically about cases where the @throws annotation contains something useful. In fact, I think the only reason why you'd want to document an unchecked exception is to clearly state that «an exception will be thrown if X». The type of the exception should not be relevant, only the fact that the exception is thrown is relevant.

I think it'd be great if the documentation said something about this, along the lines of:

When needed, unchecked exceptions should be documented in the method or param description, and not with @throws.

Clarifying what "needed" means, and perhaps providing an example.

dcausse subscribed.Oct 31 2022, 7:49 AM

colewhite subscribed.Nov 3 2022, 3:57 PM

danshick-wmde subscribed.Dec 13 2022, 11:08 AM

I've just added a section to the coding conventions page. Improvements are welcome! I will look into enforcing these rules with phpcs/phan.

One thing that came out while reviewing r850537 is that phan does not inherit @throws annotations in any way, so I've filed https://github.com/phan/phan/issues/4757 to see if it's possible to add this feature upstream. Otherwise we would have to add @throws annotations in most special pages and API modules.

Mainframe98 subscribed.Jan 22 2023, 9:35 PM

Change 926828 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan@master] Add plugin to disallow use of `new Exception`

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

gerritbot added a project: Patch-For-Review.Jun 4 2023, 9:28 PM

On second thought, I believe implementing a rule against new Exception is a task for phan, as we need some understanding of the context of the new statement (e.g., to distinguish the base Exception and a namespaced class that is also called Exception).