forgejo/forgejo

Fork 659

chore(lint): enable nilnil #11235

Merged

mfenniak merged 1 commit from famfo/forgejo:nilnil-lint into forgejo

2026-02-11 19:08:29 +01:00

famfo commented

2026-02-11 01:26:34 +01:00

Member

The general consensus I have gotten from past contributions is that return nil, nil is an
antipattern in Forgejo, especially because:

val, err := Func()
if err != nil {
    return
}
val.Func()

can easily lead to a nil pointer dereference.

This PR merely enables the lint and ignores all currently broken files. In addition to this PR,
a tracking issue to keep track of re-enabling nilnil linting on the ignored files could be
useful.

The general consensus I have gotten from past contributions is that `return nil, nil` is an antipattern in Forgejo, especially because: ```go val, err := Func() if err != nil { return } val.Func() ``` can easily lead to a nil pointer dereference. This PR merely enables the lint and ignores all currently broken files. In addition to this PR, a tracking issue to keep track of re-enabling nilnil linting on the ignored files could be useful.

famfo added the

test

needed

label

2026-02-11 01:26:34 +01:00

famfo added 1 commit

2026-02-11 01:26:34 +01:00

feat(lint): enable nilnil

testing / semgrep/ci (pull_request) Successful in 20s

Details

testing / frontend-checks (pull_request) Successful in 1m34s

Details

testing / backend-checks (pull_request) Successful in 3m21s

Details

testing / test-unit (pull_request) Successful in 7m17s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m1s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m1s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 1m57s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 1m51s

Details

testing / test-mysql (pull_request) Successful in 23m58s

Details

testing / test-pgsql (pull_request) Successful in 31m57s

Details

testing / test-sqlite (pull_request) Successful in 26m31s

Details

testing / security-check (pull_request) Successful in 43s

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

issue-labels / release-notes (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 1s

Details

issue-labels / backporting (pull_request_target) Has been skipped

Details

milestone / set (pull_request_target) Successful in 5s

Details

testing / test-e2e (pull_request) Successful in 23m9s

Details

70bf0cadde

viceice approved these changes

2026-02-11 16:32:34 +01:00

viceice added

and removed

labels

2026-02-11 16:32:53 +01:00

viceice changed title from ~~feat(lint): enable nilnil~~ to chore(lint): enable nilnil

2026-02-11 16:33:06 +01:00

viceice commented

2026-02-11 16:34:39 +01:00

Owner

changed the pr title, as this isn't a feature which is worth a release note 😉

famfo commented

2026-02-11 17:28:49 +01:00

Author

Member

Ah, that's how chores in conventional commits are meant to work :D

mfenniak commented

2026-02-11 17:58:36 +01:00

Member

Would it be plausible to exclude the current situations with //nolint:nilnil at the site of the return, rather than excluding the whole file? I worry that a whitelist of files will (a) never be cleaned up, (b) subtle because it's not in the affected file, and (c) allow new return nil, nil to be added to those files.

Would it be plausible to exclude the current situations with `//nolint:nilnil` at the site of the return, rather than excluding the whole file? I worry that a whitelist of files will (a) never be cleaned up, (b) subtle because it's not in the affected file, and (c) allow new `return nil, nil` to be added to those files.

famfo commented

2026-02-11 18:40:22 +01:00

Author

Member

Hm, I have the same worry about //nolint:nilnil, it being less discoverable and never being cleared up because of that, especially in a lot of files that aren't touched as often. I agree with the new return nil, nils being added though.

Maybe having an issue with all the files to clean this up could be actionable enough?

Hm, I have the same worry about `//nolint:nilnil`, it being less discoverable and never being cleared up because of that, especially in a lot of files that aren't touched as often. I agree with the new `return nil, nil`s being added though. Maybe having an issue with all the files to clean this up could be actionable enough?

mfenniak commented

2026-02-11 19:07:57 +01:00

Member

Creating a follow-up issue would be fine, maybe a "good first issue" for anyone wanting to find a starting point contribution.