Gocritic lint #7884

albertony · 2024-06-01T11:44:44Z

What is the purpose of this change?

Basic gocritic linting via golangci-lint was enabled in #8009.

This continues on that PR with additional features, primarily enabling custom linting rules with ruleguard via gocritic.

It then adds custom linting rules for log statements, with suggestions to use fs. instead of log., which is required for --use-json-log to properly make log output in JSON format. This fixes #6038.

Note:

After ruleguard files are changed, lint cache will not be automatically invalidated. GitHub Actions cache must be manually deleted, unless waiting for the transition to a new year-week number because then it changes the cache key and therefore will the next lint will run without cache. Running locally, command golangci-lint cache clean can be used to test changes to ruleguard files.

//Note that when used from golangci-lint (using the gocritic linter configured
// with the ruleguard check), because rule files are not handled by
// golangci-lint itself, changes will not invalidate the golangci-lint cache,
// and you must manually clean to cache (golangci-lint cache clean) for them to
// be considered, as explained here:
// https://www.quasilyte.dev/blog/post/ruleguard/#using-from-the-golangci-lint

Was the change discussed in an issue or in the forum before?

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

ncw

Thanks for doing this :-)

I think gocritic makes good suggestions by-and-large.

Maybe we should disable the captlocal linter check as that seemed to create the most noise. URL is what they are actually called "Uniform Resource Locator" and "VFS" is "Virtual Filing System" so unless we can special case those I think we should drop that rule.

The other rules looked good and it found a bug and the code was easier to read at the end.

It would be good to have a framework where we can make our own tests - rclone is getting to be a big project now and linting for our rules seems like a good idea (like the log.Fatal -> fs.Fatal).

fs/operations/operations.go

ncw · 2024-06-06T18:42:12Z

cmd/serve/webdav/webdav.go

@@ -243,19 +243,19 @@ func newWebDAV(ctx context.Context, f fs.Fs, opt *Options) (w *WebDAV, err error
 }

 // Gets the VFS in use for this request
-func (w *WebDAV) getVFS(ctx context.Context) (VFS *vfs.VFS, err error) {
+func (w *WebDAV) getVFS(ctx context.Context) (vf *vfs.VFS, err error) {


I don't like these VFS -> vf subs

ncw · 2024-06-06T18:42:31Z

backend/netstorage/netstorage.go

@@ -858,10 +858,10 @@ func shouldRetry(ctx context.Context, resp *http.Response, err error) (bool, err

 // callBackend calls NetStorage API using either rest.Call or rest.CallXML function,
 // depending on whether the response is required
-func (f *Fs) callBackend(ctx context.Context, URL, method, actionHeader string, noResponse bool, response interface{}, options []fs.OpenOption) (io.ReadCloser, error) {
+func (f *Fs) callBackend(ctx context.Context, endpoint, method, actionHeader string, noResponse bool, response interface{}, options []fs.OpenOption) (io.ReadCloser, error) {


I think URL really is a better name in this package

bin/rules.go

albertony · 2024-06-07T09:46:34Z

I think gocritic makes good suggestions by-and-large.

Yes, I think so too. I'm also considering looking more into its performance checks (https://go-critic.com/overview.html#checkers-from-the-performance-group), disabled by default, but seem interesting (at least educational for me to look into what they report).

Enabling more and more linters with golangci-lint must result in some considerable amount of overlapping checks, but I don't see it as easy to verify it properly (without creating a large suite of code with representative issues to test the linters on), so as long as the time to run them is acceptable I guess its ok..

Maybe we should disable the captlocal linter check as that seemed to create the most noise. URL is what they are actually called "Uniform Resource Locator" and "VFS" is "Virtual Filing System" so unless we can special case those I think we should drop that rule.

I knew I didn't find a good alternative to those two, and because of that I did those changes in a separate commit.. 😌 However, I must say I find it strange that it is almost exclusively the netstorage backend code that must use the URL name in variables. Everywhere else we have coped fine without it. Could we alias the package "net/url" instead, e.g. "gourl" (netstorage already has aliased hash package as "gohash"), and then use lowercase url as variable names?

I picked "vf" as name of VFS variables simply because fs.Fs variables often is "f".

It would be good to have a framework where we can make our own tests - rclone is getting to be a big project now and linting for our rules seems like a good idea (like the log.Fatal -> fs.Fatal).

Yes. I've only scratched the surface of it, but I think it has potential to be very useful. The ruleguard syntax seems very neat yet extremely powerful for making various custom lint checks! I just put my extremely simple ruleguard implementation in a file in the existing bin folder for now, but we could consider a separate top-level folder for such files if we expect to make more use of it?

albertony · 2024-06-07T14:21:46Z

I improved the rule implementation some. If I'm not mistaken, the intention was to replace calls to any of the standard logging functions from log package with our own in fs package? And also I've tweaked the suggestion so that it gives a working suggestion for Print and Prinln variants, not just Printf. In other words:

log.(Print|Fatal|Panic)(f|ln)?` -> `fs.(Printf|Fatalf|Panicf)

I also pushed commits implementing the missing fs functions, to be able to test the lint suggestions, e.g. execute golangci-lint run --fix and see if the result compiles. But not sure if this matches what you had in mind, in the issue that triggered this.

This is still a draft, and there is something with regards to caching that I need to considered (added a todo in the pr description to remember it), but if @ncw have any feedback on the latest things they are more than welcome.

albertony · 2024-08-02T17:58:33Z

Rebased and fixed conflicts

ncw · 2024-08-15T15:08:50Z

Let's try to get the fixes and the enabling of the linter merged otherwise we'll be in rebase hell forever!

Can we

disable the captlocal linter and drop the corresponding commits
rebase (again - sorry!)
Merge up to and including "enable custom linting rules with ruleguard via gocritic"

This might involve making a new PR for 1-3 and then rebasing this one on master it is merged.

I improved the rule implementation some. If I'm not mistaken, the intention was to replace calls to any of the standard logging functions from log package with our own in fs package? And also I've tweaked the suggestion so that it gives a working suggestion for Print and Prinln variants, not just Printf.

Nice :-)

In other words:

log.(Print|Fatal|Panic)(f|ln)?` -> `fs.(Printf|Fatalf|Panicf)

I think log.Print* should become fs.Logf which is the direct equivalent of log.Print* and I don't think we need the fs.Printf function - that is bound to confuse someone sooner or later! Otherwise exactly what I had in mind

log.(Print|Fatal|Panic)(f|ln)?\(` -> `fs.(Logf|Fatalf|Panicf)\(nil, `

I also pushed commits implementing the missing fs functions, to be able to test the lint suggestions, e.g. execute golangci-lint run --fix

I was not aware of that option. Would have saved me a lot of time a couple of days ago when installing golangci-lint 1.60.1!

and see if the result compiles. But not sure if this matches what you had in mind, in the issue that triggered this.

I think that sounds good. The end goal being that if the user is using --use-json-log then all the log output is in JSON format. (That probably doesn't need to include log.Panicf!)

This is still a draft, and there is something with regards to caching that I need to considered (added a todo in the pr description to remember it)

Doesn't golangci-lint do caching for us?

albertony · 2024-08-15T17:07:06Z

This might involve making a new PR for 1-3

Done: #8009

albertony · 2024-08-15T17:49:14Z

Rebased this on branch gocritic-lint-basic which is basis of #8009, for now. When that PR is merged this will be rebased on master leaving only the additional - currently:

fs: add log Printf, Fatalf and Panicf
fs: refactor base log method name for improved consistency
fs: refactor log statements to use common helper
build: enable custom linting rules with ruleguard via gocritic

I also did this change:

I think log.Print* should become fs.Logf

Regarding caching: The issue is that if the ruleguard rules change, golangci-lint will not detect this and may still use its existing cache and therefore not consider the changed rules. When running locally one have to run golangci-lint cache clean to make sure they are considered. From GitHub Actions it is not that straight forward, although those with privileges can manually delete cache entries from the Actions page, however the cache key contains year-week number so once a week the lint run will create a new cache and this will solve itself. Maybe this is good enough?

ncw · 2024-08-15T21:55:16Z

Rebased this on branch gocritic-lint-basic which is basis of #8009, for now. When that PR is merged this will be rebased on master leaving only the additional - currently:

Merged now!

fs: add log Printf, Fatalf and Panicf
fs: refactor base log method name for improved consistency
fs: refactor log statements to use common helper
build: enable custom linting rules with ruleguard via gocritic
I also did this change:

I think log.Print* should become fs.Logf

Regarding caching: The issue is that if the ruleguard rules change, golangci-lint will not detect this and may still use its existing cache and therefore not consider the changed rules. When running locally one have to run golangci-lint cache clean to make sure they are considered. From GitHub Actions it is not that straight forward, although those with privileges can manually delete cache entries from the Actions page, however the cache key contains year-week number so once a week the lint run will create a new cache and this will solve itself. Maybe this is good enough?

Perhaps we need a note to that effect in the ruleguard code so we see it when updating it and can click the clear cache button. Then that is probably OK.

albertony · 2024-08-16T07:41:05Z

Perhaps we need a note to that effect in the ruleguard code so we see it when updating it and can click the clear cache button. Then that is probably OK.

rclone/bin/rules.go

Lines 3 to 8 in a6f456e

 // Note that when used from golangci-lint (using the gocritic linter configured 

 // with the ruleguard check), because rule files are not handled by 

 // golangci-lint itself, changes will not invalidate the golangci-lint cache, 

 // and you must manually clean to cache (golangci-lint cache clean) for them to 

 // be considered, as explained here: 

 // https://www.quasilyte.dev/blog/post/ruleguard/#using-from-the-golangci-lint

Merged now!

Great.
I have rebased.

This PR now have many (more in log than in "Files changed" tab) lint issues, under "unchanged files with check annotations", such as:

ruleguard: suggestion: fs.Fatalf(nil, "Unable to decode %q: %s", fn, err) (gocritic)

I've intentionally left these in for now, but if you think the suggestions now are good I can add a commit fixing these - let's try golangci-lint run --fix :)

ncw

This is looking great!

The ruleguard fixes all look like they will work, but I think some of them will trigger other lint warnings.

Yes give the auto fix a try :-)

I guess you are intending to add the subsequent commits here so we don't merge a lint apocalypse ;-)

albertony · 2024-08-17T15:00:26Z

The ruleguard fixes all look like they will work, but I think some of them will trigger other lint warnings.

Yes give the auto fix a try :-)

Done. The autofix worked really well to apply the suggestions, however some additional manual work was needed to update imports (maybe that can be done with a simple command as well, I just triggered on each file the on-save auto cleanup that VSCode does and it fixed most of it). It is documented here:

rclone/bin/rules.go

Lines 29 to 38 in a6f456e

 // Caveats: 

 // - After applying the suggestions, imports may have to be fixed manually, 

 // removing unused "log", adding "github.com/rclone/rclone/fs" and "fmt", 

 // and if there was a variable named "fs" or "fmt" in the scope the name 

 // clash must be fixed. 

 // - Suggested code is incorrect when within fs package itself, due to the 

 // "fs."" prefix. Could handle it using condition 

 // ".Where(m.File().PkgPath.Matches(`github.com/rclone/rclone/fs`))" 

 // but not sure how to avoid duplicating all checks with and without this 

 // condition so haven't bothered yet.

After fixing the imports to make it compile, there were no additional lint issues - in my local run of golangci-lint for Windows, but there could be for other platforms - I'll run that on GitHub.

I guess you are intending to add the subsequent commits here so we don't merge a lint apocalypse ;-)

Good point!

⏬

Added this as a single commit, noted that it fixes #6038 (I'm assuming that is correct), and pushed to let it run through all checks...

albertony · 2024-08-17T15:02:29Z

fs/log/redirect_stderr_windows.go

@@ -34,6 +35,6 @@ func setStdHandle(stdhandle int32, handle syscall.Handle) error {
 func redirectStderr(f *os.File) {
 err := setStdHandle(syscall.STD_ERROR_HANDLE, syscall.Handle(f.Fd()))
 if err != nil {
- log.Fatalf("Failed to redirect stderr to file: %v", err)
+ fs.Fatalf(nil, "Failed to redirect stderr to file: %v", err)


@ncw Is it correct to do this change here, or is this an exception where it needs to use log. directly still?

Hmm, not sure. Does it cause a stack overflow due to unbounded recursion? If not then the change is good. You might need to simulate a failure to test.

It seems to work fine. Commenting out the if condition, running with --log-file option, the message "Failed to redirect stderr to file" ends up in the file. No stack overflow.

bin/not-in-stable.go

albertony · 2024-08-17T16:35:49Z

Hm, the linting now failed with a lot of issues from govet of the type:

lib\http\template.go:71:18: printf: non-constant format string in call to github.com/rclone/rclone/fs.Fatalf (govet)
fs.Fatalf(nil, fmt.Sprint("Fatal error executing template", err))

I didn't get those locally before, but I may not have been on latest version of golangci-lint because after I ran go install just now and re-ran I get the same issues locally as well. So this is probably related to a change in golangci-lint version v1.60.1.

ncw · 2024-08-17T20:27:35Z

I didn't get those locally before, but I may not have been on latest version of golangci-lint because after I ran go install just now and re-ran I get the same issues locally as well. So this is probably related to a change in golangci-lint version v1.60.1.

Yes I fixed a load of those recently for golangci lint v1.60.1 and that's the error I thought would happen, I guess having fixed a load of them recently!

albertony · 2024-08-18T20:07:38Z

Yes I fixed a load of those recently for golangci lint v1.60.1 and that's the error I thought would happen, I guess having fixed a load of them recently!

I see! What is the best fix then? Is it to add alternative non-format variants of all the functions (Log in addition to Logf, etc)? I have pushed a version with this (not entirely finished, naming things etc..). Then I updated the custom lint rules accordingly, reverted back the lint fix and re-ran the lint. This time the autofix fixed everything without creating new lint issues. These changes are in the last 3 commits, rest is same as before.

Ping @ncw

ncw · 2024-09-06T11:33:36Z

I see! What is the best fix then? Is it to add alternative non-format variants of all the functions (Log in addition to Logf, etc)? I have pushed a version with this (not entirely finished, naming things etc..). Then I updated the custom lint rules accordingly, reverted back the lint fix and re-ran the lint. This time the autofix fixed everything without creating new lint issues. These changes are in the last 3 commits, rest is same as before.

Nice - that seems like the proper fix - well done :-)

Sorry due to my vacation I've lost momentum on this PR :-(

Do you want to try to get it merged for 1.68 - I'm hoping to release this weekend? If so can you rebase, make sure the tests all pass and I'll merge.

…rmat string warnings

…unctions

…#6038 This changes log statements from log to fs package, which is required for --use-json-log to properly make log output in JSON format. The recently added custom linting rule, handled by ruleguard via gocritic via golangci-lint, warns about these and suggests the alternative. Fixing was therefore basically running "golangci-lint run --fix", although some manual fixup of mainly imports are necessary following that.

ncw · 2024-09-06T16:04:10Z

Thank you :-)

albertony · 2024-09-06T17:46:23Z

Thank you! :)

Sorry due to my vacation I've lost momentum on this PR :-(

Well deserved! I was getting a bit impatient due to rather high risk of more conflicts, but seems that was not necessary.

Do you want to try to get it merged for 1.68 - I'm hoping to release this weekend?

Yes, works for me. I rebased it, and I see you have already merged it. Thank you. 🥳

ncw · 2024-09-06T21:19:10Z

In the hour between you rebasing this and me merging it, I merged something else first which caused the build to break when I merged this. Unfortunate but easily fixed!

I wish GitHub had a rebase a pr button, would make life much easier! Or even rebase this pr if there are no conflicts.

I should probably write a script to do that, wouldn't be too hard...

I'm glad we've merged this now 🙂

albertony · 2024-09-10T11:19:50Z

I wish GitHub had a rebase a pr button, would make life much easier!

Is this what you wished for, or did you mean something else:

https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/#always-have-the-option-to-update

ncw · 2024-09-10T11:36:45Z

That is exactly what I meant :-) I've enabled that now - thank you!

albertony self-assigned this Jun 1, 2024

albertony mentioned this pull request Jun 1, 2024

Flag --use-json-log does not working as expected #6038

Closed

ncw reviewed Jun 6, 2024

View reviewed changes

albertony force-pushed the gocritic-lint branch 3 times, most recently from 07c5054 to 2ca0dc5 Compare June 7, 2024 14:13

albertony force-pushed the gocritic-lint branch from 2ca0dc5 to 14a75e3 Compare August 2, 2024 17:58

albertony mentioned this pull request Aug 15, 2024

Gocritic lint basic #8009

Merged

5 tasks

albertony force-pushed the gocritic-lint branch 2 times, most recently from 62e50bc to 1734ad9 Compare August 15, 2024 17:37

albertony marked this pull request as ready for review August 15, 2024 17:49

albertony force-pushed the gocritic-lint branch from 1734ad9 to a6f456e Compare August 16, 2024 07:24

albertony requested a review from ncw August 16, 2024 07:41

ncw approved these changes Aug 17, 2024

View reviewed changes

albertony force-pushed the gocritic-lint branch from a6f456e to b38e50c Compare August 17, 2024 14:58

albertony commented Aug 17, 2024

View reviewed changes

bin/not-in-stable.go Outdated Show resolved Hide resolved

albertony force-pushed the gocritic-lint branch 2 times, most recently from 392878d to 9c483ac Compare August 18, 2024 15:22

albertony force-pushed the gocritic-lint branch from 9c483ac to 61e3eb8 Compare August 18, 2024 15:57

albertony force-pushed the gocritic-lint branch from 61e3eb8 to eb89cf6 Compare August 31, 2024 20:17

albertony added 7 commits September 6, 2024 15:04

build: enable custom linting rules with ruleguard via gocritic

dae9e7d

fs: refactor log statements to use common helper

3d64bdf

fs: refactor base log method name for improved consistency

5a26fa7

fs: add log Printf, Fatalf and Panicf

42145c3

fs: add non-format variants of log functions to avoid non-constant fo…

8d029f9

…rmat string warnings

build: update custom linting rule for log to suggest new non-format f…

a3fb9ac

…unctions

albertony force-pushed the gocritic-lint branch from eb89cf6 to 510d87a Compare September 6, 2024 13:05

ncw merged commit bcdfad3 into rclone:master Sep 6, 2024
10 checks passed

albertony deleted the gocritic-lint branch September 6, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gocritic lint #7884

Gocritic lint #7884

albertony commented Jun 1, 2024 •

edited

Loading

ncw left a comment

ncw Jun 6, 2024

ncw Jun 6, 2024

albertony commented Jun 7, 2024

albertony commented Jun 7, 2024

albertony commented Aug 2, 2024

ncw commented Aug 15, 2024

albertony commented Aug 15, 2024

albertony commented Aug 15, 2024

ncw commented Aug 15, 2024

albertony commented Aug 16, 2024

ncw left a comment

albertony commented Aug 17, 2024

albertony Aug 17, 2024 •

edited

Loading

ncw Aug 17, 2024

albertony Aug 31, 2024

albertony commented Aug 17, 2024

ncw commented Aug 17, 2024

albertony commented Aug 18, 2024 •

edited

Loading

ncw commented Sep 6, 2024

ncw commented Sep 6, 2024

albertony commented Sep 6, 2024

ncw commented Sep 6, 2024

albertony commented Sep 10, 2024

ncw commented Sep 10, 2024

Gocritic lint #7884

Gocritic lint #7884

Conversation

albertony commented Jun 1, 2024 • edited Loading

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

ncw left a comment

Choose a reason for hiding this comment

ncw Jun 6, 2024

Choose a reason for hiding this comment

ncw Jun 6, 2024

Choose a reason for hiding this comment

albertony commented Jun 7, 2024

albertony commented Jun 7, 2024

albertony commented Aug 2, 2024

ncw commented Aug 15, 2024

albertony commented Aug 15, 2024

albertony commented Aug 15, 2024

ncw commented Aug 15, 2024

albertony commented Aug 16, 2024

ncw left a comment

Choose a reason for hiding this comment

albertony commented Aug 17, 2024

albertony Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

ncw Aug 17, 2024

Choose a reason for hiding this comment

albertony Aug 31, 2024

Choose a reason for hiding this comment

albertony commented Aug 17, 2024

ncw commented Aug 17, 2024

albertony commented Aug 18, 2024 • edited Loading

ncw commented Sep 6, 2024

ncw commented Sep 6, 2024

albertony commented Sep 6, 2024

ncw commented Sep 6, 2024

albertony commented Sep 10, 2024

ncw commented Sep 10, 2024

albertony commented Jun 1, 2024 •

edited

Loading

albertony Aug 17, 2024 •

edited

Loading

albertony commented Aug 18, 2024 •

edited

Loading