SA6005: replacing ToLower comparison with EqualFold is not semantics-preserving

[typesanitizer]

In https://github.com/dominikh/go-tools/issues/366, there was a check (SA6005) added which recommends using EqualFold over ToLower comparison. However, the transformation of changing strings.ToLower(s) == strings.ToLower(t) to strings.EqualFold(s, t) is not semantics-preserving (counter-example).

While I understand that EqualFold is generally what you'd want, the error message doesn't explain this subtlety:

main.go:6:5: should use strings.EqualFold instead (SA6005)

Without that further context, it's easy to misunderstand that the transformation is indeed semantics-preserving. Moreover, the DigitalOcean article describes the transformation as an optimization -- which are ~almost always semantics-preserving (with the exception of things like FMA) -- which reinforces the misconception. Would it make sense to add further context to the error message, or perhaps just a link to somewhere which covers the subtlety in more detail? One way this could go is:

• Add 1-2 semantics-changing examples in the docs for SA6005.
• Remove the link to the DigitalOcean article.
• Perhaps reword the message to something like prefer using strings.EqualFold (SA6005); note: this is not semantics-preserving, see https://staticcheck.dev/docs/checks#SA6005 for more details. It's a bit of a mouthful, but it seems safer to err on the side of being wordy rather than have someone spend a bunch of time investigate why an innocent seeming "fix" somehow changed the behavior of their code.

[ainar-g]

If this transformation really is unsafe in some cases then perhaps it would be better to move it into the aggressive category.

[typesanitizer]

Sure, that's also an option. I wouldn't describe it as "unsafe" as such; normally comparison via case-folding is the more desirable behavior, but I suspect most people aren't aware that (a) such an operation exists and (b) is distinct from case-insensitive comparison (apparently, myself included for the (b) part until now).

Even with the "aggressive" marker, I think it would be better to at least have a note of caution someplace rather than not (could be just in the docs, not in the error message), because it's easy to misconstrue the operation as semantics-preserving.

[dominikh]

As a first step we'll have to remove SA6005. As you've pointed out, the suggestion is sometimes wrong, which constitutes a false positive. A note in the documentation doesn't suffice, in part because most people don't read the documentation and just trust Staticcheck's advice. A more elaborate message would be inconvenient due to its required length, and it'd still be a false positive that needs to be explicitly ignored, which is something we try to avoid whenever possible.

We can revisit the check for the aggressive category, which lends itself better to longer messages, and puts the onus of making sure that the change is valid on the user, not us.

[Fyb3roptik]

I would like to note also that if I am comparing a string with a dot in it example (Google.com vs google.com) and I use EqualFold, it will not lowercase all characters. Using strings.ToLower actually solved the issue for me where EqualFold did not.

[dominikh]

@Fyb3roptik I don't see how that could be the case: https://go.dev/play/p/fdAwELuvauh

[Fyb3roptik]

Disregard, I had an issue in my code lol