Phpinspectionsea: False positive for "Statement could be decoupled from foreach"

[zerkms]

False positive for "Statement could be decoupled from foreach":

        foreach ($tos as $to) {
            $xmppMessage->setTo($to);
            $client->send($xmppMessage);
        }

The $client->send($xmppMessage); line marked as "This statement seems to be disconnected from its parent foreach".

[kalessil]

Hi @zerkms, inspection has no a clue what happens inside $xmppMessage->setTo($to).

IMO it should be something like this (depends on how do you build the $xmppMessage object)

        foreach ($tos as $to) {
            $client->send(new XmppMessage($to, $message, ...));
        }

[kalessil]

How is $xmppMessage creation look like?

[zerkms]

inspection has no a clue

that's the point: this inspection assumes too much. It should not report when there is a chance of false positives.

it should be something like this

false-positives driven development, FPDD ;-)

[kalessil]

It does =) I agree, but I need fresh ideas how to teach it new tricks: it only sees that '$client->send($xmppMessage);' is not using $to (directly/indirectly) and there are no local variables inside the loop which send depending on.

/cc @rentalhost: you perhaps you have an idea for the case?

[kalessil]

$xmppMessage->setTo($to) consumes $to, but is it enough to assume the $xmppMessage modification?

[zerkms]

Given in real life the code might be as terrible as

for ($i = 0; $i < count($this->recipients); ++$i) {
    $this->nextRecipient();
    $client->send($this->xmppMessage);
}

it would be near to impossible to implement it reliably.

[rentalhost]

We have two possibilities here:

• Hard way: make inspection understand that $xmppMessage->setTo($to) uses $to (_current behavior_), and that $client->send($xmppMessage) uses $xmppMessage (that have used $to previously);
• Easy way: ignore any code after $to is being used;

I really prefer the "hard way", once that it will be able to inspect a lot of cases, while the easy way will forget a lot of simple cases that are valid for now.

Basically, when $to (the _subject_) is used, it should be checked all neighbor variables and add it as "subject" too.

foreach ($tos as $to) {
    // Subjects: [ $to ]

    $xmppMessage->setTo($to, $anotherNeighbor);
    // $xmppMessage and $anotherNeighbor add to subjects
    // Subjects now: [ $to, $xmppMessage, $anotherNeighbor ] 

    $client->send($xmppMessage);
    // Now it should be valid, once that $xmppMessage is too a subject
    // $client add to subjects
    // Subjects now: [ $to, $xmppMessage, $anotherNeighbor, $client ]

    $outContext->call();
    // Ops! $outContext is not a subject and not use any valid subject.

    $client->submit();
    // Valid: $client is a subject.
}

[kalessil]

@zerkms : I wouldn't be as nice as here for that approach =) Your case is something we can professionally deal with.

@rentalhost : oh, that's a good addition.We can get pretty good quality with your extended approach, thank you!

[zerkms]

Your case is something we can professionally deal with.

with the code from the original post - yes. But I can easily make up an example that is impossible to statically analyse with 100% confidence using runtime variable/member resolution.

I think ideally it should be a conservative behaviour: if it can prove the statement is a loop invariant - it should warn, otherwise it should not.

I personally find the "easy way" good enough: given it's impossible to cover everything reliably anyway, why make codebase more complicated for near to 0 benefit?

[kalessil]

No need, I probably have seen cases similar to what you are mentioning =)

Current behaviour follows the "hard" way, so this kind of natural extension.
Oh this inspection spots code fragments to be refactored really well - FPDD as you mentioned ;)

[kalessil]

BTW @zerkms: FPDD is a nice term, I allowed myself to share it with our community - https://twitter.com/kalessil/status/927647674328846336

[kalessil]

Fixed.