[Bug] Unfixable scenario with reportUnusedCatchesOfUncheckedExceptions / [Breaking change request ?] Unchecked exception #116
Description
Introduction
Considering the \LogicException was set as unchecked.
Examples of error returned actually
When using this library, an error is reported for this code.
/**
* @throws LogicException
*/
private function throwLogic(): void // error: Unused @throws LogicException annotation
{
throw new LogicException();
}
An error is also reported without the option ignoreDescriptiveUncheckedExceptions for
/**
* @throws LogicException Description
*/
private function throwLogic(): void // error: Unused @throws LogicException annotation
{
throw new LogicException();
}
No error is reported for
private function throwLogic(): void
{
throw new LogicException();
}
No error is reported without the option reportUnusedCatchesOfUncheckedExceptions for
private function catchLogic(): void
{
try {} catch (\LogicException $e) {}
}
A non-fixable case
So now, we have a pretty weird situation: with reportUnusedCatchesOfUncheckedExceptions: true,
private function catchLogic(): void
{
try {
$this->throwLogic()
} catch (\LogicException $e) {}
}
private function throwLogic(): void
{
throw new LogicException();
}
Return an error (LogicException is never thrown in the corresponding try block). But
private function catchLogic(): void
{
try {
$this->throwLogic()
} catch (\LogicException $e) {}
}
/**
* @throws \LogicException
*/
private function throwLogic(): void
{
throw new LogicException();
}
Return an error too (Unused @throws LogicException annotation).
And there is worst
private function catchLogic(): void
{
try {
$this->callThrowLogic()
} catch (\LogicException $e) {} // Error useless catch
}
private function callThrowLogic(): void
{
$this->throwLogic();
}
/**
* @throws \LogicException
*/
private function throwLogic(): void // Error useless anotation
{
throw new LogicException();
}
Personal reflexion
First, since some linter are asking for an @throws tag for every exception, I feel like
/**
* @throws LogicException
*/
private function throwLogic(): void
{
throw new LogicException();
}
Should not return an error.
The ignoreDescriptiveUncheckedExceptions would not be needed anymore then.
You can annotate or not.
But I think another option could be added to force the annotation for the function creating the exception
RequiredAnnotationForUncheckedExceptions.
/**
* @throws LogicException
*/
private function throwLogic(): void
{
throw new LogicException();
}
private function throwLogic2(): void // Error, the @throws is needed
{
throw new LogicException();
}
private function callThrowLogic(): void // No error, the annotation is only needed for the function throwing the uncheckedException
{
$this->throwLogic();
}
Some linters are trying that every Exception threw is documented ; but there is always false-positive/false-negative cause they only do static analysis. This is a classic false positive.
private function dontThrowLogic(): void // Error missing throw annotation
{
try { throw new LogicException(); } catch (\LogicException $e) {}
}
But what is an unchecked exception then ?
For me, an unchecked exception should be an exception you don't have to catch. You can, but you don't have to.
So, both
private function callThrowLogic(): void
{
$this->throwLogic();
}
and
private function callThrowLogic(): void
{
try {
$this->throwLogic();
} catch (\LogicException $exception) {}
}
would be valid.
How does it fix the issue
With reportUnusedCatchesOfUncheckedExceptions: true, the following code
private function catchLogic(): void
{
try {
$this->throwLogic()
} catch (\LogicException $e) {}
}
private function throwLogic(): void
{
throw new LogicException();
}
can now be fixed as
private function catchLogic(): void
{
try {
$this->throwLogic()
} catch (\LogicException $e) {}
}
/**
* @throws \LogicException
*/
private function throwLogic(): void
{
throw new LogicException();
}
without any error. This seems pretty normal for me: if you want to catch an exception (checked or unchecked) the function you called HAS TO document it.