[java:s3516] FP, if input value is returned

SonarQube Server / Community Build
, ,
1

Qube: Community 9.9

The Java rule s3516 produces FP, if an input value is returned on every code path.

public static <V> V check(V v)
{
    if (earlyExitCondition(v))
        return v;
    
    if (v == null)
        throw new IllegalArgumentException("v must not be null");
    
    if (!someCondition(v))
        throw new IllegalArgumentException("v does not match the condition.");
    
    return v;
}

This is a simple method, that validates an input value and always returns that input value, if no exception is thrown.

2

Hi,

I don’t understand how this is an FP. Each return statement is followed by v. There is no return that returns anything else.

 
Ann

3

Well, let me try to put this rule into a new light to explain that.

The rule is supposed to highlight methods, that accidentally return the same value for each path. It should of course exclude methods, that are expected to always return the same result.

So how can we distinguish these two groups of methods? Well, many methods “produce” an original result or at least select it from a list of options. These methods are to be distinguished from methods, that just validate input values (and return them) or return the input value or “this” for chaining (fluent API).

And I think, the criterion to distinguish these methods is the following:

If the return value is “this” or one of the method parameters, it is a method of the second group, that doesn’t produce original result values. In this case the rule should not hit.

Would be a pity to spread lots of NOSONARs or (less painful) to deactivate this nice rule.

5

Hi,

Thanks for sharing your logic. I’ve flagged this for the language experts.

 
Ann

7

Hello @mfroehlich ,

Unfortunately, I can not reproduce the issue on my side with a SQ 9.9 and its associated java analyzer. I feel that you might have simplified your code a bit too much. Our symbolic execution engine is exploring also methods that are called from within the body of a method. This means that depending on the behaviors of methods earlyExitCondition() and someCondition(). Can you share a complete code snippet that would not contain any unknown method and reproduce consistently the FP?

Otherwise, your issue looks similar to me to this ticket: SONARJAVA-3473.

I would however wait for your full reproducer before judging if it’s the same scenario.

Cheers,
Michael

8

Ok, this here is a copy&paste of the exact method, that is notified about by this rule.

    public static int inRangeOr0( int number, String argName, short[] array, String extMessage )
    {
        Require.nonNull( array, "array" );
        
        if ( number == 0 )
            return number;
        
        if ( ( number < 0 ) )
            throw new IllegalArgumentException( argName + " out of range: " + number + " [" + 0 + ", " + array.length + ")." + ( extMessage == null || extMessage.isEmpty() ? "" : ( " " + extMessage ) ) );
        
        if ( ( number >= array.length ) )
            throw new IllegalArgumentException( argName + " out of range: " + number + " [" + 0 + ", " + array.length + ")." + ( extMessage == null || extMessage.isEmpty() ? "" : ( " " + extMessage ) ) );
        
        if ( argName == null )
            throw new IllegalArgumentException( "'argName' must not be null." );
        
        return number;
    }

Thanks for taking a look.

1 Like
9

Thanks for the reproducer @mfroehlich,

As I was guessing, this is indeed the same problem as reported in SONARJAVA-3473. If you replace the runtime exception (IllegalStateException) with any checked exception (for instance IOException), then the FP disappears. The problem is on the Symbolic Execution side, not handling correctly runtime exceptions for this rule.

We will see what we can do about it!

Cheers,
Michael

11

Sounds great!

Of course using a checked Exception is exactly not an option here.

Thanks for investigating this.

14

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.