Global severity setting has no effect if `enableAllRules` is `true`

[rustysys-dev]

Describe the bug
Due to the code not having flags for parsing the global severity setting from toml, its not possible to globally set the severity.

https://github.com/mgechev/revive/blame/d7e3d5eac7453bfbb4c5d02e612459f91024427f/lint/config.go#L28

Proposed fix is to add the toml tag for severity here.

[rustysys-dev]

apparently setting confidence does nothing either.

[rustysys-dev]

This issue is actually a bit deeper...

with the following config

severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0

# Enable all available rules
enableAllRules = true

[rule.blank-imports]
    Disabled = true
[rule.file-header]
    Disabled = true
[rule.max-public-structs]
    Disabled = true
[rule.line-length-limit]
    Disabled = true
[rule.function-length]
    Disabled = true

[rule.argument-limit]
    Arguments = [5]
[rule.cyclomatic]
    Arguments = [10]
[rule.function-result-limit]
    Arguments = [3]
[rule.cognitive-complexity]
    Arguments = [10]

and the following command.

revive --config revive.toml --formatter stylish ./...

you will never see error severity raised for any of the rules.

1. Main first calls GetConfig which checks for a config file path specified on the command line. If found when found it parses the file and then normalizes the rules that are defined in the file to include the default severity if available. (which it currently is not)
2. Main then calls GetListingRules to decide which tests are run based on the configuration file. However, since this slice of rules is never added to the config, and never normalized for the global severity... the configuration doesn't match expectation.
3. Hypothetically lets say that in my first point severity was successfully set, and therefore all of the rules that are defined in the config should also have had their severity normalized to error properly as well. At that point it is safe to say that these rules will work properly. What about the rules that were not explicitly defined in the config? well... they are not identified as error but as warning.

To fix this issue, it might require appending entries for each missing rule to the config before the call to normalizeConfig

[chavacava]

Hi @rustysys-dev, thanks for filling the issue.

apparently setting confidence does nothing either.

Confidence does something:

$ ./revive -config low-confidence.toml testdata/golint/exported.go | wc -l
$ 8
$ ./revive -config total-confidence.toml testdata/golint/exported.go | wc -l
$ 7
$ ./revive -config more-than-total-confidence.toml testdata/golint/exported.go | wc -l
$ 0

With a config file:

ignoreGeneratedHeader = false
severity = "warning"
confidence = XXXX
errorCode = 0
warningCode = 0

[rule.exported]

with XXXX set to 0.5, 1.0, and 1.5 respectively

I'll check for severity

[chavacava]

Testing severity with the following configuration

ignoreGeneratedHeader = false
severity = "error"
confidence = 0.5
errorCode = 255
warningCode = 111

[rule.exported]

Results in:

$ ./revive -config low-confidence.toml testdata/golint/exported.go
testdata/golint/exported.go:7:6: func name will be used as golint.GolintFoo by other packages, and that stutters; consider calling this Foo
testdata/golint/exported.go:20:6: exported type Foo should have comment or be unexported
testdata/golint/exported.go:53:1: exported function Error should have comment or be unexported
testdata/golint/exported.go:54:1: exported function String should have comment or be unexported
testdata/golint/exported.go:55:1: exported function ServeHTTP should have comment or be unexported
testdata/golint/exported.go:56:1: exported function Read should have comment or be unexported
testdata/golint/exported.go:57:1: exported function Write should have comment or be unexported
testdata/golint/exported.go:58:1: exported function Unwrap should have comment or be unexported
$ echo $?
255

After setting severity to "warning" in the configuration, we get

$ ./revive -config low-confidence.toml testdata/golint/exported.go
testdata/golint/exported.go:7:6: func name will be used as golint.GolintFoo by other packages, and that stutters; consider calling this Foo
testdata/golint/exported.go:20:6: exported type Foo should have comment or be unexported
testdata/golint/exported.go:53:1: exported function Error should have comment or be unexported
testdata/golint/exported.go:54:1: exported function String should have comment or be unexported
testdata/golint/exported.go:55:1: exported function ServeHTTP should have comment or be unexported
testdata/golint/exported.go:56:1: exported function Read should have comment or be unexported
testdata/golint/exported.go:57:1: exported function Write should have comment or be unexported
testdata/golint/exported.go:58:1: exported function Unwrap should have comment or be unexported
$ echo $?
111

That seems a correct behavior of the severity flag of the configuration.

[mgechev]

Keeping open until we hear back from @rustysys-dev

[rustysys-dev]

@mgechev Thank you for keeping the issue open.

I just tried it on my end, and I am still not getting results.

The configuration given above works on my end....

Working config

severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0
ignoreGeneratedHeader = false


# Enable all available rules
# enableAllRules = true

[rule.blank-imports]
    Disabled = true
[rule.file-header]
    Disabled = true
[rule.max-public-structs]
    Disabled = true
[rule.line-length-limit]
    Disabled = true
[rule.function-length]
    Disabled = true

[rule.exported]
[rule.argument-limit]
    Arguments = [5]
[rule.cyclomatic]
    Arguments = [10]
[rule.function-result-limit]
    Arguments = [3]
[rule.cognitive-complexity]
    Arguments = [10]

Working config output

dx-admin-backend on  HEAD (99da7bb) [?] via 🐹 v1.16.3 via 20GiB/31GiB took 15ms 
•100% at 15:30:41 ❯ revive --config revive.toml --formatter stylish ./...
internal/adapter/middleware/auth/auth.go
  (16, 2)  https://revive.run/r#exported  exported const TokenRawKey should have comment (or a comment on this block) or be unexported  
  (21, 1)  https://revive.run/r#exported  exported function AccessController should have comment or be unexported                       

internal/entity/token.go
  (12, 1)  https://revive.run/r#exported  comment on exported type Auth0AccessToken should be of the form "Auth0AccessToken ..." (with optional leading article)  

...
    
  (11, 1)  https://revive.run/r#exported  exported method Usecase.Private should have comment or be unexported  


 ✖ 13 problems (13 errors) (0 warnings)

dx-admin-backend on  HEAD (99da7bb) [?] via 🐹 v1.16.3 via 20GiB/31GiB took 14ms 
•100% at 15:31:18 ❯ echo $?
1

Non-working config

severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0
ignoreGeneratedHeader = false


# Enable all available rules
# enableAllRules = true

[rule.blank-imports]
    Disabled = true
[rule.file-header]
    Disabled = true
[rule.max-public-structs]
    Disabled = true
[rule.line-length-limit]
    Disabled = true
[rule.function-length]
    Disabled = true

[rule.exported]
[rule.argument-limit]
    Arguments = [5]
[rule.cyclomatic]
    Arguments = [10]
[rule.function-result-limit]
    Arguments = [3]
[rule.cognitive-complexity]
    Arguments = [10]

Non-working config output

dx-admin-backend on  HEAD (99da7bb) [?] via 🐹 v1.16.3 via 21GiB/31GiB took 8ms 
•100% at 15:42:05 ❯ revive --config revive.toml --formatter stylish ./...
internal/utils/log/log.go
  (76, 23)  https://revive.run/r#add-constant  string literal "ERROR" appears, at least, 3 times, create a named constant for it  

...
                      
  (93, 1)   https://revive.run/r#exported      comment on exported method Auth0AccessToken.ValidateLoad should be of the form "ValidateLoad ..."                       


 ✖ 28 problems (0 errors) (28 warnings)

dx-admin-backend on  HEAD (99da7bb) [?] via 🐹 v1.16.3 via 20GiB/31GiB took 52ms 
•100% at 15:42:13 ❯ echo $?
0

the only difference being that in the one that doesn't work enableAllRules = true is defined and [rule.exported] is not explicitly defined.

I also tried the following configuration where I define enableAllRules = true and also explicitly define [rule.exported] as below.

severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0
ignoreGeneratedHeader = false


# Enable all available rules
enableAllRules = true

[rule.blank-imports]
    Disabled = true
[rule.file-header]
    Disabled = true
[rule.max-public-structs]
    Disabled = true
[rule.line-length-limit]
    Disabled = true
[rule.function-length]
    Disabled = true

[rule.exported]
[rule.argument-limit]
    Arguments = [5]
[rule.cyclomatic]
    Arguments = [10]
[rule.function-result-limit]
    Arguments = [3]
[rule.cognitive-complexity]
    Arguments = [10]

and strangely enough this worked too...

so the bug might be that if all rules are enabled, but rule.exported is not explicitly defined, then the status code will not be honored.

seems like only explicitly defined rules are able to cause an error result.

Speaking of which I have problem context question. How is global severity defined when it doesn't have a tag to parse it into the struct as shown here?

[chavacava]

Thanks @rustysys-dev I'll check the cases you describe.

Speaking of which I have problem context question. How is global severity defined when it doesn't have a tag to parse it into the struct as shown here?

These annotations are not necessary if struct's field names match (case-insensitively) with the fields in the toml file.
Short example: https://play.golang.org/p/9Mx0QSHO8b6

[rustysys-dev]

These annotations are not necessary if struct's field names match (case-insensitively) with the fields in the toml file.
Short example: https://play.golang.org/p/9Mx0QSHO8b6

Thank you! that makes sense... and now that you say it I do remember hearing about that before... I am just used to seeing everything defined explicitly. Feel a bit stupid now 😂

Also I checked out your PR and tried it for my use case and it looks good!