[WIP] New rule `use-errors-new` (propose replacing `fmt.Errorf` by `errors.New` when possible) #1136

chavacava · 2024-11-18T20:28:31Z

WIP PR implementing a new rule use-errors-new that proposes replacing fmt.Errorf by errors.New when possible.

As indicated in this issue, beyond style considerations, errors.News is ~4x faster that fmt.Errorf

I've tested the rule against public code bases like terraform and gitea and it finds hundreds of "failures" (with no false positives, the rule is very simple)

I'm not so happy with the rule name, if you have something better... I take.
I'll complete the PR (tests+doc) in the next days, meanwhile comments are welcome.

cc: @ccoVeille @alexandear

alexandear · 2024-11-18T20:52:36Z

I'm not so happy with the rule name, if you have something better... I take.

The rule name use-errors-new is good, in my opinion.

PR implementing a new rule use-errors-new that proposes replacing fmt.Errorf by errors.New when possible.

There is a linter called perfsprint with a check named errorf that suggests the same. It can be configured and used through golangci-lint via the perfsprint.errorf configuration.

I have done the replacements to errors.New with the help of perfsprint in these PRs:

ccoVeille · 2024-11-18T21:07:23Z

I was about say there might be another linter that may have the rule.

But I'm fine with having it supported in revive, as it's a specific tool.

But then later, golangci-lint team might report it as a duplicate, maybe they will simply disable it by default.

BTW, do you plan to enable it my default? Or at least to add it to the recommended rules list?

ccoVeille · 2024-11-18T21:08:09Z

rule/use_errors_new.go

+	"github.com/mgechev/revive/lint"
+)
+
+// UseErrorsNewRule lints given else constructs.


I don't get this sentence, maybe you could rephrase

ccoVeille · 2024-11-18T21:09:12Z

rule/use_errors_new.go

As I've said in the PR description, it is work in progress, I'll complete the PR (tests+doc) in the next days.

tests and doc added

ccoVeille · 2024-11-18T21:26:46Z

I checked go vet, go critic, errorlint, gosimple. It's not supported

For me, this rule should be a staticcheck one, but each repository deserves a linter maybe. The main issues with different tools, is the fact you are never sure something will get merged by a maintainer.

Here we are in revive, you decide.

But once in golangci-lint, someone may raise a flag. What do you think @ldez?

ldez · 2024-11-18T21:30:15Z

It already exists inside golangci-lint: perfsprint.

main.go:6:9: fmt.Errorf can be replaced with errors.New (perfsprint)
        return fmt.Errorf("foo")
               ^

#1136 (comment)

ldez · 2024-11-18T21:46:06Z

To be clear, I have no problem with duplicate rules between golangci-lint and revive.

The 2 projects are independent, so it's expected to have duplicates especially with "metalinters".

Same thing with golangci-lint and staticcheck, or staticcheck and revive, or gocritic and revive, etc.

I try to limit them because it's confusing for users to have 2 elements for the same rule/check but it should not be used as a limitation for other tools.

But I have a problem when someone uses a PR open on Golangci-lint, and so the work of someone else, to create a rule inside another tool.
golangci/golangci-lint#5087

ccoVeille · 2024-11-18T22:32:56Z

It already exists inside golangci-lint: perfsprint.

Yes, it does. @alexandear already spotted it, and I was looking at other linters (even the experimental ones in gocritic, go vet and gosimple) if it was already available.

And yes, apparently, perfsprint is currently the only one to report it.

To be clear, I have no problem with duplicate rules between golangci-lint and revive.

The 2 projects are independent, so it's expected to have duplicates especially with "metalinters".

Same thing with golangci-lint and staticcheck, or staticcheck and revive, or gocritic and revive, etc.

I try to limit them because it's confusing for users to have 2 elements for the same rule/check but it should not be used as a limitation for other tools.

Thanks for your feedback @ldez

perfsprint detects way more things than what this rule would do, so I would say it's OK, anyway. It's not as if perfsprint was detecting only what is added in this PR. And then, it could become a deprecation logic at some point.
But I'm going too far 😄 as it's not the case.

So now, if someone want to have revive use-errors-new and perfsprint it can still disable use-errors-new or perfsprint.errorf`

It might become a problem only if use-errors-new is added at some point to the revive default linter.
But here again, I'm anticipating too much

Thanks again Ludovic.

But I have a problem when someone uses a PR open on Golangci-lint, and so the work of someone else, to create a rule inside another tool. golangci/golangci-lint#5087

Yes, this one was pretty crazy

chavacava · 2024-11-19T07:51:57Z

Thank you all for the feedback.

I didn't know about perfsprint. Anyway the fact that there is already a linter that does similar analysis than a rule was never a criterion to decide adding or not the rule to revive.

BTW, do you plan to enable it by default?

I'm not planning to enable it by default.

It might become a problem only if use-errors-new is added at some point to the revive default linter.

Users with enableAllRules = true in the conf will have use-errors-new by default

The rule name use-errors-new is good, in my opinion.

OK, I'll stick to it if I don't find something better.

denisvmedia · 2024-11-20T12:57:28Z

rule/use_errors_new.go

+		Category:   "errors",
+		Node:       n,
+		Confidence: 1,
+		Failure:    "replace fmt.Errorf by errors.New ",


nit:

Suggested change

Failure: "replace fmt.Errorf by errors.New ",

Failure: "replace fmt.Errorf by errors.New",

ccoVeille · 2024-11-20T20:26:09Z

config/config.go

+	&rule.UseErrorsNewRule{},
+	&rule.RedundantBuildTagRule{},
+	&rule.UseErrorsNewRule{},


Likely to be with conflicts, this might explain why #1142 was opened

ccoVeille · 2024-11-20T20:33:44Z

rule/use_errors_new.go

+	}
+
+	if len(funcCall.Args) > 1 {
+		return w // the use of fmt.Errorf is legit


Well, not exactly

We have crazy people out there

fmt.Errorf("%s, "whatever")
https://github.com/search?q=language%3Ago++%22fmt.Errorf%28%5C%22%25s%5C%22%2C+%5C%22%22&type=code

fmt.Errorf("%w", err)
https://github.com/search?q=language%3Ago++%22fmt.Errorf%28%5C%22%25w%5C%22%2C+%22&type=code

I created an issue on perfprint linter, as it's in their scope

errorf improvements catenacyber/perfsprint#36

I think you can merge like this, with current code.

My point can be addressed later

Discussion moved on the PR

#1142 (comment)

ccoVeille · 2024-11-20T20:42:57Z

Moved to #1142

initial working version

77b1bf6

chavacava added the rule proposal Issue proposing a new rule label Nov 18, 2024

chavacava self-assigned this Nov 18, 2024

ccoVeille reviewed Nov 18, 2024

View reviewed changes

denisvmedia reviewed Nov 20, 2024

View reviewed changes

alexandear and others added 8 commits November 20, 2024 20:51

refactor: simplify with strings.Prefix, strings.CutPrefix (#1137)

8d72c32

refactor: move functions out from utils.go (#1139)

6f6316c

test: add default case when no arguments provided to a rule (#1140)

0c08023

feat: add redundant-build-tag rule (#1135)

76b64c0

initial working version

60af015

final version

182296f

Merge branch 'master' into feature/use-errors-new

8692e83

Delete rule/redundant_build_tag.go

44fca17

chavacava closed this Nov 20, 2024

chavacava deleted the feature/use-errors-new branch November 20, 2024 20:27

ccoVeille reviewed Nov 20, 2024

View reviewed changes

ccoVeille mentioned this pull request Nov 20, 2024

errorf improvements catenacyber/perfsprint#36

Open

chavacava mentioned this pull request Nov 20, 2024

adds rule use-errors-new #1142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] New rule `use-errors-new` (propose replacing `fmt.Errorf` by `errors.New` when possible) #1136

[WIP] New rule `use-errors-new` (propose replacing `fmt.Errorf` by `errors.New` when possible) #1136

chavacava commented Nov 18, 2024

alexandear commented Nov 18, 2024

ccoVeille commented Nov 18, 2024

ccoVeille Nov 18, 2024 •

edited

Loading

chavacava Nov 20, 2024

ccoVeille Nov 18, 2024

chavacava Nov 19, 2024

chavacava Nov 20, 2024

ccoVeille commented Nov 18, 2024

ldez commented Nov 18, 2024 •

edited

Loading

ldez commented Nov 18, 2024

ccoVeille commented Nov 18, 2024

chavacava commented Nov 19, 2024

denisvmedia Nov 20, 2024

chavacava Nov 20, 2024

ccoVeille Nov 20, 2024

ccoVeille Nov 20, 2024

ccoVeille Nov 20, 2024 •

edited

Loading

ccoVeille Nov 20, 2024

ccoVeille commented Nov 20, 2024 •

edited

Loading

	Failure: "replace fmt.Errorf by errors.New ",
	Failure: "replace fmt.Errorf by errors.New",

[WIP] New rule use-errors-new (propose replacing fmt.Errorf by errors.New when possible) #1136

[WIP] New rule use-errors-new (propose replacing fmt.Errorf by errors.New when possible) #1136

Conversation

chavacava commented Nov 18, 2024

alexandear commented Nov 18, 2024

ccoVeille commented Nov 18, 2024

ccoVeille Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille commented Nov 18, 2024

ldez commented Nov 18, 2024 • edited Loading

ldez commented Nov 18, 2024

ccoVeille commented Nov 18, 2024

chavacava commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille commented Nov 20, 2024 • edited Loading

[WIP] New rule `use-errors-new` (propose replacing `fmt.Errorf` by `errors.New` when possible) #1136

[WIP] New rule `use-errors-new` (propose replacing `fmt.Errorf` by `errors.New` when possible) #1136

ccoVeille Nov 18, 2024 •

edited

Loading

ldez commented Nov 18, 2024 •

edited

Loading

ccoVeille Nov 20, 2024 •

edited

Loading

ccoVeille commented Nov 20, 2024 •

edited

Loading