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 e1ad905f4ba6525951fdc26c315c2c3c5e4728d7. 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 https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/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

• [x] I have checked there is no other PR open for the same change.
• [x] I have read the Contribution Guidelines.
• [x] 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).
• [x] I have added tests to cover my changes.
• [x] I have verified that the code complies with the projects coding standards.
• [ ] [Required for new sniffs] I have added XML documentation for the sniff.

[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 9e37c3d4dc649525da7dec3bff0ed892319a417e. 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.