Rules definitions

[mmorel-35]

The actual rules needs to clearly explicit to be easily understood and used by the users. This is a first draft that needs to be challenged to then enable the separation of concerns in the tool.

error-format

❌
fmt.Errorf("Simple message")
fmt.Sprintf("%s", err)
fmt.Sprintf("%v", err)

✅
errors.New("Simple message")
err.Error()

with:

• error-format.err-error option equivalent to err-error to use err.Error() even if it is only equivalent for non-nil errors

string-format

❌
fmt.Sprintf("%s", strVal)
fmt.Sprintf("format string %s", strVal)
fmt.Sprintf("%s format string", strVal)

✅
strVal
"format string " + strVal
strVal + "format string"

with

• string-format.concat option equivalent to strconcat to enable strings concatenation

bool-format

❌
fmt.Sprintf("%t", boolVal)

✅
strconv.FormatBool(boolBal)

hash-format

❌
fmt.Sprintf("%x", hash)

✅
hex.EncodeToString(hash)

integer-format

❌
fmt.Sprintf("%d", id)
fmt.Sprintf("%v", version)

✅
strconv.Itoa(id)
strconv.FormatUint(uint64(version), 10)

with

• integer-format.cast option equivalent to int-conversion to enable int or uint type cast

[alexandear]

Please remember that we have to maximize compatibility with existing rules:

  perfsprint:
    # Optimizes even if it requires an int or uint type cast.
    # Default: true
    int-conversion: false
    # Optimizes into `err.Error()` even if it is only equivalent for non-nil errors.
    # Default: false
    err-error: true
    # Optimizes `fmt.Errorf`.
    # Default: true
    errorf: false
    # Optimizes `fmt.Sprintf` with only one argument.
    # Default: true
    sprintf1: false
    # Optimizes into strings concatenation.
    # Default: true
    strconcat: false

https://github.com/golangci/golangci-lint/blob/e0e37c43762068149116bcd69b305c1ca05647ae/.golangci.reference.yml#L2190

Maybe it is better to add simply only one string-format rule to cover format cases for all types fmt.Sprintf("%t", boolVal), fmt.Sprintf("%x", hash), fmt.Sprintf("%d", id).

[catenacyber]

hash-format

Let's name it hex-format ?

[ccoVeille]

I was more likely to report them the way @alexandear suggested.

So hex would be reported as string-format

But, then the logic of having a way to configure what linter is enabled, it won't be easy.

Does it mean that it would be reported as string-format.hex

[ccoVeille]

Another distinct question, the related configuration would be something hex-conv ? The same way we have int-conv

I try to think about the configuration at the same time as the way we report them

[catenacyber]

I guess you need some rules like integer-format to have options like int-conversion

One thing to emphasize about current option is that there are some that do behavior changes :

• err-error : breaks behavior for nil errors
• sprintf1 : changes behavior (avoids panic) for string with user inputting %whatever in it

[ccoVeille]

The Christmas break is now over.

Let's regroup and start to think about it again

[catenacyber]

Happy new year @ccoVeille

Do you expect something from me here ?

[ccoVeille]

Maybe yes, from you or from @mmorel-35 or @alexandear

It would be helpful if someone could list the option the linter should have and how to group them

Here is an example

- integer-format
    - int-conversion

- error-format
    - errorf

- string-format that will report
   - string-format.hex to use hex.EncodeToString
   - string-format.bool to use FormatBool
   - string-format.concat to use concatenation

Did I forgot something?

Assuming that if you disable string-format you disable everything, same for others.

Existing conf value will be supported, and mapped to new one.

Matthieu provided some guidance by creating a well documented issues. But everyone feedback made it difficult to follow. It's something like that I would like to get from you @catenacyber

About possible breaking change with the reporting:

we could add a setting that will allow to report the rules. This would allow us to provide rules name (like string-format.bool) in the reported error message, something @mmorel-35 tried to add I think.

• or we simply consider people will have to deal with the breaking changes. It would only affect people who ignored some rules via issues node in golangci-lint.

• or this change won't affect the way to report them, only the configuration

[catenacyber]

So, I see (adding current options that were not here)

- integer-format
    - int-conversion

- error-format
    - errorf
    - err-error (known behavior change adding panic for nil errors)

- string-format
   - sprintf1 (known behavior change removing panic)
   - strconcat

- bool-format

- hex-format

(We format into strings different objects)

[ccoVeille]

Thanks

Is this post OK for you @alexandear @mmorel-35 ?

I'm asking before anyone start to consider working on this

[mmorel-35]

It seems good to me.
IMHO, the next thing to do would to formalize it in a README or anything that is useful to document the rules namings and behavior.
And then the code could be aligned accordingly

[alexandear]

Looks good to me, also.
Thank you for your work, @ccoVeille

[catenacyber]

Fixed by #42 right ?