Go-tools: simple: S1017 for TrimSuffix and friends doesn't work with literals and manual trimming

[ainar-g]

Currently, S1017 complains—correctly—about this code:

const suffix = ".json"

if strings.HasSuffix(s, suffix) {
        s = strings.TrimSuffix(s, suffix)
}

However, if the suffix is spelled out as a literal (as it unfortunately often is in some codebases), S1017 can't find it:

if strings.HasSuffix(s, ".json") {
        s = strings.TrimSuffix(s, ".json")
}

Same with manual trimming:

if strings.HasSuffix(s, ".json") {
        s = s[:len(s)-len(".json")]
}

[Here] is an example code which contains some other variants, which I would love to get flagged.

(Inspired by code found by @EugeneOne1.)

[dominikh]

Will fix

[dominikh]

I'm unsure about flagging

s = "file.json"
if strings.HasSuffix(s, ".json") {
    s = s[:len(s)-5]
}

It's _unlikely_ that this would ever result in a false positive, but what if ".json" and 5 are unrelated, and the user always wishes to cut off the last 5 characters, even if the length of the suffix changes at some point?

That is, what if the user may do this in the future:

s = "file.json"
if strings.HasSuffix(s, ".notjson") {
    s = s[:len(s)-5]
}

If he followed our suggestion of

s = strings.TrimSuffix(s, ".json")

then changing the suffix to look for later would require changing back to the original form.

[ainar-g]

I also think that the scenario is unlikely. Moreover, if I were to read someone else's code and saw something like the ".notjson" example, I would be about 95 % sure that that is an error, and someone simply forgot to change the constant after changing the suffix. (Which perhaps could become a check of its own, although not necessarily in staticcheck.) On the other hand, if the code was closer to this:

// someSuffixLen is well documented and explained here.
const someSuffixLen = 5

// …

if strings.HasSuffix(s, ".notjson") {
        s = s[:len(s)-someSuffixLen]
}

That is, if a named, documented constant were used, the code would be clearer. So, even if this unlikely scenario were to happen, the check would still signal to the owners of the code that the code is confusing.

[dominikh]

the check would still signal to the owners of the code that the code is confusing

The real danger is users applying suggested fixes without thinking about them. Staticcheck tells the user to use TrimSuffix instead, so they do, even if it's not the right choice in the long run.

[ainar-g]

I guess that the case where 5 is explicitly spelled out could be skipped by staticcheck (although I still think that a tool with weaker false-positive guarantees should warn about it), but what about the one with len(".json")? _Technically_ I could still want to only trim five bytes, and I could also choose to spell 5 as len(".json") for some reason.

[dominikh]

Thinking about it, all of the arguments against flagging 5 also apply to the entirety of S1017… Also, apparently, we already flag it for HasPrefix, and it is only HasSuffix that is the exception. From the existing, passing tests:

if strings.HasPrefix(id1, "test") { // want `should replace.*with.*strings\.TrimPrefix`
    id1 = id1[len("test"):]
}

if strings.HasPrefix(id1, "test") { // want `should replace.*with.*strings\.TrimPrefix`
    id1 = id1[4:]
}

if strings.HasPrefix(id1, s1) { // want `should replace.*with.*strings\.TrimPrefix`
    id1 = id1[14:]
}

if strings.HasPrefix(id1, s1) { // want `should replace.*with.*strings\.TrimPrefix`
    id1 = id1[n:]
}

We even flag the last instance, where n is an integer constant that happens to match the length of s1… I suppose that makes the entirety of this argument moot, and we should just make the check for HasSuffix behave identically to HasPrefix. At least nobody has ever complained about false positives in S1017 before…

[dominikh]

b4848f4..06ae6ed make various changes to S1017. Analysis of HasSuffix now behaves identically to HasPrefix, allowing string literals and integer literals in the same instances.

We no longer permit using named constants in slicing, and we only allow integer literals in slicing when the affix being checked for is a string literal. In all other cases, the use of len is required, and the argument to len has to match the affix argument. This should address all of your wishes, and avoid some false positives where constants only happen to have the right value, without actually being related.

All these changes combined found one new instance in the standard library:

// canonicalHost strips port from host if present and returns the canonicalized
// host name.
func canonicalHost(host string) (string, error) {
    var err error
    host = strings.ToLower(host)
    if hasPort(host) {
        host, _, err = net.SplitHostPort(host)
        if err != nil {
            return "", err
        }
    }
    if strings.HasSuffix(host, ".") {
        // Strip trailing dot from fully qualified domain names.
        host = host[:len(host)-1]
    }
    return toASCII(host)
}