2024-12-gophercon-india.slide

# Operationalising golangci-lint
: @formatter:off
Leveraging Go Linters for Robust Code Quality
02 Dec 2024
Tags: linters, golangci-lint
Summary: Configuring golangci-lint for better Go code.

Suhas Karanth
Staff Engineer, Netskope
skaranth@netskope.com
https://github.com/sudo-suhas
https://www.linkedin.com/in/skaranth90/

: @formatter:on

## Agenda

- What are linters?
- Why use linters?
- Examples
- How to use `golangci-lint`?

## What are linters?

- Static code analysis tools
- Flag potential bugs and code style issues

Trivia: The term originates from a Unix utility that examined C language source code.

: Static code analysis means that we analyze the code without executing it.

## Why use linters?

- **[Shift-Left Testing][shift-left-testing]**
- **Code style**:
	- Consistency
	- Readability
	- Slightly faster speed of delivery
	- Avoid inconsequential debates (ex: `gofmt` for tabs vs spaces)
- **Guardrails**

[shift-left-testing]: https://www.drdobbs.com/shift-left-testing/184404768

: [Shift-Left Testing]:
: - Feature lifecycle - Product requirement gathering, architectural design, implementation, unit
: tests, integration tests etc.
: - Objective: Detect issues earlier
: - Rationale: Bugs are cheap when caught young
: - Win: Faster feedback => improved quality and speed of delivery.
: - Contextual: Move detection of bugs from unit tests/integration tests to development stage.
: [Consistency]:
: - Scope: code constructs + package/function/type names. Ex: Effective Go guidelines on package
: names.
: - Rationale: Consistency => readability => maintainability.
: [Guardrails]:
: - For both experienced and new developers, avoid tricky bugs
: - More helpful for newbies
: - Not all bugs are easily caught in tests either.

## Examples

: To understand why we would want to use linters, lets look at some examples. I am going to show
: some code snippets and I want you to try and spot the issue. The examples are a bit contrived but
: that should be okay for the purposes of this discussion. I will also show an example of a bug
: that is harder to catch in tests.

## Example #1

.code demo-code/snippets/errorsas.go /START BROKEN/,/END BROKEN/

## Example #1

.image assets/errorsas/errorsas-01.png _ 957

## Example #1

.image assets/errorsas/errorsas-02.png _ 957

## Example #1

.image assets/errorsas/errorsas-03.png _ 957

## Example #1

.image assets/errorsas/errorsas-04.png _ 957

## Example #1

.code demo-code/snippets/errorsas.go /START BROKEN/,/END BROKEN/

## Example #1

Issue can be fixed by passing a pointer to `*MyError`, the type implementing error.

.code demo-code/snippets/errorsas.go /START FIXED/,/END FIXED/

`go vet` can flag the issue in this case.

```
$ go tool vet help errorsas
errorsas: report passing non-pointer or non-error values to errors.As

The errorsas analysis reports calls to errors.As where the type
of the second argument is not a pointer to a type implementing error.
```

For the example from the previous slide, we get the following error:

```
snippets/errorsas.go:21:6: second argument to errors.As must be a non-nil pointer to either a
type that implements error, or to any interface type
```

: By default, `go test` runs a high-confidence subset of `go vet` checks which includes `errorsas`.

## Example #2

## Example #2

.code demo-code/urlqueryv1/main.go /START/,/END/

## Example #2

<p style="margin: -20px">

.image assets/urlquery/urlquery-01.png _ 979

</p>

## Example #2

<p style="margin: -20px">

.image assets/urlquery/urlquery-02.png _ 979

</p>

## Example #2

<p style="margin: -20px">

.image assets/urlquery/urlquery-03.png _ 979

</p>

## Example #2

<p style="margin: -20px">

.image assets/urlquery/urlquery-04.png _ 979

</p>

## Example #2

.play -edit demo-code/urlqueryv1/main.go /START/,/END/

## Example #2

One of the _many_ detections in [staticcheck][staticcheck-sa4027] can flag this:

[staticcheck-sa4027]: https://staticcheck.dev/docs/checks/#SA4027

```
snippets/tbd.go:15:2: SA4027: (*net/url.URL).Query returns a copy, modifying it doesn't change
the URL (staticcheck)
        u.Query().Set("search", params.Search)
        ^
```

Small changes are needed to fix the issue:

.play -edit demo-code/urlqueryv2/main.go /START/,/END/

## Example #3

: Example where it is harder to catch the issue in tests

## Example #3

.code demo-code/rowserrv1/main.go /START FETCH/,/END FETCH/

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-01.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-02.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-03.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-04.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-05.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-06.png _ 979

</p>

## Example #3

.code demo-code/rowserrv1/main.go /START DEMO/,/END DEMO/

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-07.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-08.png _ 979

</p>

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-09.png _ 979

</p>

## Example #3

.play -edit demo-code/rowserrv1/main.go /START DEMO/,/END DEMO/

## Example #3

.code demo-code/rowserrv1/main.go /START FETCH/,/END FETCH/

## Example #3

Linter that can catch this issue - [rowserrcheck][rowserrcheck]:

```
rowserrv1/main.go:82:30: rows.Err must be checked (rowserrcheck)
        rows, err := db.QueryContext(ctx, qry)
                                    ^
```

[rowserrcheck]: https://github.com/jingyugao/rowserrcheck

Issue can be fixed with a `rows.Err()` check:

.play -edit demo-code/rowserrv2/main.go /START/,/END/

## Example #3

<p style="margin: -20px">

.image assets/rowserr/rowserr-10.png _ 979

</p>

## Linting in Go

## Linting in Go

1. Why `golangci-lint`?
2. Why do we need extensive configuration?
3. Configuration builder sheet walkthrough.

## golangci-lint

[https://golangci-lint.run/][golangci-lint-home]

[golangci-lint-home]: https://golangci-lint.run/

<p style="margin-top:  -15px;">

> _It runs linters in parallel, uses caching, supports YAML configuration,
> integrates with all major IDEs, and includes over a hundred linters._

</p>

<table style="margin-top:  -10px;">
<tr>
<td><b>Popular</b></td>
<td>

.image assets/golangci-lint-popularity.png _ 520

</td>
</tr>
<tr>
<td><b>Actively maintained</b></td>
<td>

.image assets/golangci-lint-pulse-insights.png 380 _

</td>
</tr>

</table>

: What is golangci-lint? It is a linter aggregator that provides a performant, consistent interface
: for running 100+ linters.

## Adopting golangci-lint

Just run `golangci-lint` in your project and you are done.

```
$ golangci-lint run
```

_Right?_

: Hopefully, I have convinced you about using golangci-lint. So just run golangci-lint in your
: project and you are done, right?

## Need for configuration

Unfortunately, just running `golangci-lint` is not enough.

**Default configuration**

`golangci-lint` enables a very small set of linters by default:

- [errcheck][errcheck]
- [gosimple][gosimple]
- [govet][govet]
- [ineffassign][ineffassign]
- [staticcheck][staticcheck]
- [unused][unused]

[errcheck]: https://github.com/kisielk/errcheck

[gosimple]: https://github.com/dominikh/go-tools/tree/master/simple

[govet]: https://pkg.go.dev/cmd/vet

[ineffassign]: https://github.com/gordonklaus/ineffassign

[staticcheck]: https://staticcheck.io/

[unused]: https://github.com/dominikh/go-tools/tree/master/unused

## Useful linters disabled by default

There are many more extremely useful linters that are disabled by default. Some examples:

- [errorlint][errorlint]: for error wrapping scheme introduced in Go 1.13.
- [gocognit][gocognit]: enforce limit on cognitive complexity of functions.
- [gofumpt][gofumpt]: a stricter gofmt.
- [gosec][gosec]: inspect source for **security issues**.
- [noctx][noctx]: find HTTP requests without `context.Context`.
- [rowserrcheck][rowserrcheck], [sqlclosecheck][sqlclosecheck]: ensure correct usage of
  `database/sql` package.
- [gocritic][gocritic]: 100+ diagnostic checks
- [revive][revive]: faster, configurable golint

[errorlint]: https://github.com/polyfloyd/go-errorlint

[gocognit]: https://github.com/uudashr/gocognit

[gofumpt]: https://github.com/mvdan/gofumpt

[gosec]: https://github.com/securego/gosec

[noctx]: https://github.com/sonatard/noctx

[rowserrcheck]: https://github.com/jingyugao/rowserrcheck

[sqlclosecheck]: https://github.com/ryanrolds/sqlclosecheck

[gocritic]: https://github.com/go-critic/go-critic

[revive]: https://github.com/mgechev/revive

: These linters can help uncover bugs and ensure consistent code style.
: gosec deserves a special mention since it is able to catch security issues like SQL and command
: injection.

## So just enable everything?

## So just enable everything?

No, for a few reasons:

- **Noise:** Not all linters match _your_ style.
- **Ineffective:** Some linters are useful only in a specific context.
- **Duplication:** Some checks are duplicated between different linters.
- **Premature optimisation:** Use in constrained execution environments.

[decorder]: https://gitlab.com/bosi/decorder

[zerologlint]: https://github.com/ykadowak/zerologlint

[prealloc]: https://github.com/alexkohler/prealloc

: - [Noise]: Opinions might differ. False positives. exhaustruct
: - [Ineffective]: zerologlint for zerolog
: - [Duplication]: govet and revive both have a structtag check. 
: - [Premature optimisation]: prealloc

: I know of 40+ checks that are duplicated across different linters. Disabling duplicate checks and
: linters has a performance benefit as well.
: I hope you can see why we need to configure golangci-lint to enable the linters that we want. But
: there is 1 additional aspect I want to touch on w.r.t configuration.

## Individual linter configuration

For many of the linters, individual configuration is needed to get the desired behaviour. 

For example, for `gofumpt`, we might want to have the following configuration:

```
  # Choose whether to use the extra rules.
  # Default: false
  extra-rules = true
```

: This configuration is also necessary for linters that include multiple rules such as gocritic 
: and revive because we would want a subset of the available rules.

## Configuration builder

[golangci-lint-linters-v1.61.0 sheet][golangci-lint-linters-v1-61-0-sheet]

This sheet helps to manage the configuration for `golangci-lint`.

[golangci-lint-linters-v1-61-0-sheet]: https://docs.google.com/spreadsheets/d/1-pg0JNHOoN8bRer5bRuZFYYPpspYXdwe5wIKjglcvsE/edit?usp=sharing

: Typically, you don't need to frequently update the configuration for golangci-lint. And it is
: quite common to copy and modify configuration from 1 repo to another. But it is still useful to
: have an excel sheet if you are starting from scratch for being able to discuss and gather feedback
: on the proposed configuration. And when I had finished building the sheet, I used some extra time
: I had to generate the config from the sheet itself. So this might be a bit of overengineering.
: --
: The configuration was last updated for version v1.61.0 of golangci-lint.
: The configuration is not perfect for everyone.
: --
: The configuration is updated in a few seconds whenever any change is made in the sheet by the Apps
: Script attached to the project (see triggers.gs).

## Problem: Integrating golangci-lint into a project

**New project** 

- No problem

**Pre-existing project** 

- Large number of issues could be reported
- _Fixing all the issues in a single effort can be **inhibitively expensive**._

: When we are creating a new project, it is straightforward to integrate golangci-lint into it.
: However, if we are integrating `golangci-lint` into a pre-existing project where either the linter
: was not integrated or the configuration was minimal, we need a strategy to iteratively fix the
: large number of issues that would be reported by the linter with the new/updated configuration.

## Solution: Integrating golangci-lint into an existing project

`golangci-lint` provides a mechanism for gradual adoption by reporting issues only for new and
modified lines in the commit. .

```
$ golangci-lint run --help
Run the linters

Usage:
  golangci-lint run [flags]
Flags:
 ...
      --new-from-rev REV               Show only new issues created after git revision REV
```

See [golangci-lint FAQ][golangci-lint-faq].

[golangci-lint-faq]: https://golangci-lint.run/welcome/faq/#how-to-integrate-golangci-lint-into-large-project-with-thousands-of-issues

: The official Github action provided by authors of `golangci-lint` also supports a configuration
: option `only-new-issues` which can show only the issues that were introduced by a pull request 
: etc.

## Summary

- Linters detect and flag predefined 'patterns'.
- Using linters helps to left shift detections and improves quality and speed of delivery. It helps
  bring consistency and improves readability.
- We saw examples that show different scenarios in which linters can catch the issue.
- A bit of extra work to configure `golangci-lint` goes a long way to get the best out of it.
- The configuration builder sheet can help you get started.
- For existing projects, leverage the `--new-from-rev` option provided by `golangci-lint` to fix
  issues iteratively.

## Special Thank You

The work done by authors of various linters and `golangci-lint` is immensely valuable and I am very
grateful to them. Thank you!

## Questions?

Talk material:

.image assets/bitly-qr-code.png 350 _
<p style="text-align: center;">
  <a href="https://bit.ly/gophercon-in-2024-linters-v1">https://bit.ly/gophercon-in-2024-linters-v1</a> 
</p>

[bitly-link]: https://bit.ly/go-mtp-blr-linters-v1