php:S3699 reports a false positive when an expression has a return type `never`

Clean Code Report False-positive / False-negative...
,
1

Language: PHP
Rule: php:S3699
Sonar version: Community Edition Version 10.0 (build 68432)
quality profile: sonar way last updated at January 19, 2024

Reproducible example:

function throwMyCustomException(): never {
  throw new MyCustomException();
}

function placeWithFalsePositive(?object nullableObjForExample): object {
    return nullableObjForExample ?? throwMyCustomException();
}

Even though throwMyCustomException will never return(indicated by its return type as well as its body), php:S3699 will be reported saying that we should not use the value of throwMyCustomException() since it does not return one. Except that this is a perfectly valid guard-expression.

In fact, the inlined equivalent

function placeWithFalsePositive(?object nullableObjForExample): object {
    return nullableObjForExample ?? throw new MyCustomException();
}

will not be reported as an issue.

2

Hey there.

What version of which products are you using (no version required for SonarCloud/SonarQube Cloud)?

3

my apologies, I’ve updated the main post with the version.

4

Thanks @Barbiero. I can’t get this FP to raise on SonarQube v10.7. v10.0 is an EOL version. Is upgrading an option?

5

My mistake. Upon reviewing these issues, I found out that many of these “never” functions are marked as returning void, ie.

private static function 
throwForbiddenException(): void {
	throw new ForbiddenException("some_message");
}

This is because most of the code was migrated from php < 8.1, before the introduction of the never keyword, and so these methods were marked as void. It would be nice if sonar could analyze these types of bodies and determine that they actually return “never”, but I can also see how that would be out of scope.

In other words, I don’t think that there is a false-positive anymore.

For the record, the behavior is the same between sonarqube 10.0 and 10.7 - I realized my mistake when I tried to reproduce it minimally on 10.7.

1 Like
6

hi @Colin I just found a specific case that causes the false positive, on sonar community 10.7, php 8.1.

This snippet:

<?php
declare(strict_types=1);

function handleInvalidValue($value): never {
        throw new LogicException("$value is invalid.");
}

function mapValues($value): int {
        return match ($value) {
                "1"     => 1,
                "2"     => 2,
                default => handleInvalidValue($value),
        };
}

is the minimum unit that I could create reproducing this false positive.

What is happening is,

  1. it is a match expression
  2. the default branch is mapped to a function that never returns
  3. Sonarqube thinks that this means we are trying to use the returned value of handleInvalidValue, even though it is very common to cause an exception on invalid values

The major problem is that inlining handleInvalidValue with a throw expression causes the issue to no longer be reported:

<?php
declare(strict_types=1);

function mapValues($value): int {
	return match ($value) {
		"1"     => 1,
		"2"     => 2,
		default => throw new LogicException("$value is invalid."),
	};
}

With so I guess the correct wording would be that php:S3699 reports a false positive when a match expression has a branch returning never

9

Hello @Barbiero,

Thanks for raising awareness on this topic and sorry for the late answer.

I tried to reproduce your issue on Sonarqube Community in version 10.7.0.96327 without success. We have specific conditions in the rule that prevent this FP from being raised in this version.

However, I tested with the Sonarqube community in version 10.0.0.68432, which is the version you mentioned in your first post. With this one, I do get this FP.

Is it possible that you made a mistake and performed your last test on Sonarqube 10.0 instead of 10.7?
Otherwise, can you provide the log of this scan?

Thank you very much.
Best,
Rudy

2 Likes