Generic/UnnecessaryStringConcat: improve code coverage

[rodrigoprimo]

Description

This PR improves code coverage for the Generic.Strings.UnnecessaryStringConcat sniff.

It also removes two unreachable conditions. I'm a bit hesitant about the condition removed in e1ad905. As far as I can check, there is no T_STRING_CONCAT token in the JS tokenizer. That being said, I don't have a lot of experience with this part of the code, so I might be missing something.

While working on this PR, I noticed that the sniff doesn't support heredoc and nowdoc and I'm inclined to think that it should as it is possible to concatenate strings specified using those two ways (https://www.php.net/manual/en/language.types.string.php). Should I open an issue for this?

Related issues/external references

Part of #146

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

Hi @rodrigoprimo, thanks for yet another good PR in this series.

Aside from responding to your questions below, I have only two questions:

1. I can see that you have added tests with comments before and after the concat operator. 👍
   The sniff currently does not flag unnecessary concatenation with these comments. I wonder if it should though as the concatenation is still unnecessary and as the sniff does not contain a fixer, it wouldn't add extra complexity to the logic to do so.
   What are your thoughts on this ?
2. Would it be an idea to apply a "bow out early" pattern to the sniff to lower the nesting levels ? (for the condition on line 89/90 and line 92)
   This should also improve the usefullness of the code coverage report as it will show more clearly what paths are covered.

I'm a bit hesitant about the condition removed in e1ad905. As far as I can check, there is no T_STRING_CONCAT token in the JS tokenizer. That being said, I don't have a lot of experience with this part of the code, so I might be missing something.

I think you're right, but am not 100% sure myself (99.8% sure only ;-) ).

Maybe we should play it safe for now ? Knowing that this code will be removed in v4.0 anyway ?

        if ($tokens[$stackPtr]['code'] === T_STRING_CONCAT && $phpcsFile->tokenizerType === 'JS') {
            // JS uses T_PLUS for string concatenation, not T_STRING_CONCAT.
            return;
        } else if ($tokens[$stackPtr]['code'] === T_PLUS && $phpcsFile->tokenizerType === 'PHP') {
            // PHP uses T_STRING_CONCAT for string concatenation, not T_PLUS.
            return;
        }

While working on this PR, I noticed that the sniff doesn't support heredoc and nowdoc and I'm inclined to think that it should as it is possible to concatenate strings specified using those two ways (https://www.php.net/manual/en/language.types.string.php). Should I open an issue for this?

Opening an issue about this sounds good. IMO this would be a low priority feature request though.

I have a feeling that kind of code is rare anyhow and handling it correctly would require a non-trivial fix as heredocs/nowdocs are tokenized quite differently from single/double quoted strings.

[rodrigoprimo]

Thanks for checking this PR, @jrfnl!

The sniff currently does not flag unnecessary concatenation with these comments. I wonder if it should though as the concatenation is still unnecessary and as the sniff does not contain a fixer, it wouldn't add extra complexity to the logic to do so.
What are your thoughts on this ?

I considered this, but I thought that in some rare cases, the concatenation might be intentional precisely to have comments documenting just part of a string. That is why I'm more inclined to think that we should keep the current behavior.

Would it be an idea to apply a "bow out early" pattern to the sniff to lower the nesting levels ? (for the condition on line 89/90 and line 92)
This should also improve the usefullness of the code coverage report as it will show more clearly what paths are covered.

I attempted to implement your suggestion in 9e37c3d. Could you please take a look to check if that is what you had in mind?

I think you're right, but am not 100% sure myself (99.8% sure only ;-) ).
Maybe we should play it safe for now ? Knowing that this code will be removed in v4.0 anyway ?

Makes sense to me. I dropped the original commit and added a new one implementing your suggestion.

Opening an issue about this sounds good. IMO this would be a low priority feature request though.
I have a feeling that kind of code is rare anyhow and handling it correctly would require a non-trivial fix as heredocs/nowdocs are tokenized quite differently from single/double quoted strings.

I share your feeling that that kind of code is rare. Considering this, I'm more inclined not to open an issue. We can revisit this if a user ever comes across this limitation in the sniff and reports it. How does that sound?

[jrfnl]

The sniff currently does not flag unnecessary concatenation with these comments. I wonder if it should though as the concatenation is still unnecessary and as the sniff does not contain a fixer, it wouldn't add extra complexity to the logic to do so.
What are your thoughts on this ?

I considered this, but I thought that in some rare cases, the concatenation might be intentional precisely to have comments documenting just part of a string. That is why I'm more inclined to think that we should keep the current behavior.

Hmm.. still not sure about this (in that case a multi-line concatenation with comment could be used with the allowMultiline property to have the comment and still have clean code) , but this question not a blocker for this PR. Let's leave this for now.

Would it be an idea to apply a "bow out early" pattern to the sniff to lower the nesting levels ? (for the condition on line 89/90 and line 92)

I attempted to implement your suggestion in 9e37c3d. Could you please take a look to check if that is what you had in mind?

Looks fine to me.

Opening an issue about this sounds good. IMO this would be a low priority feature request though.
I have a feeling that kind of code is rare anyhow and handling it correctly would require a non-trivial fix as heredocs/nowdocs are tokenized quite differently from single/double quoted strings.

I share your feeling that that kind of code is rare. Considering this, I'm more inclined not to open an issue. We can revisit this if a user ever comes across this limitation in the sniff and reports it. How does that sound?

That's fair.