ethereum-optimism / ethereum-optimism/optimism

linter/bigint: suggested fix emits an unbound qualifier when op-service/bigs is aliased or dot-imported

Open
#22,702 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
6.5k
Forks
4k
Avg merge
2d 38m
Merged PRs (30d)
164

Description

### Summary

The `bigint` custom linter's suggested fix decides two things separately that
have to agree:

- **whether** to add the `op-service/bigs` import — `fileHasImport()` matches on the import **path**
- **what identifier** to qualify the rewritten call with — `buildStrictCall()` hardcodes `"bigs."`

When a file already imports `op-service/bigs` under any name other than `bigs`,
the import edit is suppressed (path matches) *and* the emitted call references
an identifier that isn't bound. `golangci-lint --fix` then rewrites the file
into code that doesn't compile.

Observed on `develop` at `2258ea57`.

### Reproduction

Add a testdata package containing a file that imports the package under an
alias, then run the analyzer with suggested fixes applied:

```go
package a

import (
"math/big"

mybigs "github.com/ethereum-optimism/optimism/op-service/bigs"
)

var _ = mybigs.Uint64Strict

func aliased() {
x := new(big.Int)
_ = x.Uint64()
}
```

`analysistest.RunWithSuggestedFixes` produces:

```go
import (
"math/big"

mybigs "github.com/ethereum-optimism/optimism/op-service/bigs"
)
...
_ = bigs.Uint64Strict(x) // undefined: bigs
```

The dot-import form (`. "…/bigs"`) fails the same way — the fix emits a
`bigs.` qualifier where the call should be unqualified. A blank import (`_`)
binds no identifier at all, so it also needs a usable import added.

### Why this isn't currently caught

`linter/analyzers/bigint/bigint_test.go` uses `analysistest.Run`, which checks
diagnostics only and never applies the suggested fixes. The whole fix path —
`buildStrictCall`, `fileHasImport`, `importEdit`, and the one-shot
`addImportEdit` logic — has no test coverage today.

### Impact

Latent, not currently triggered. Across `develop` at `2258ea57` I count 188
plain imports of `op-service/bigs`, zero named aliases, zero dot imports, and
one blank import (`op-service/testutils/depguard/depguard_test.go`) which
contains no `.Uint64()` calls. So no file in the tree today would be broken by
`--fix`; the defect would surface the first time someone aliases that import in
a file containing a flagged call.

### Suggested direction

Resolve the package's actual local binding in the file and use it for both
decisions, rather than deciding the import and the qualifier independently:
alias → use the alias; dot import → unqualified call; blank import → treat as
absent so a usable import is added.

I have a patch along those lines plus `RunWithSuggestedFixes` golden tests
covering the plain, aliased and dot-import forms, if that would be useful — happy
to open a PR.

Contributor guide

Open the contributing guide

Research direction

Start in linter/analyzers/bigint/bigint_test.go and inspect buildStrictCall, fileHasImport, importEdit, and addImportEdit. Add testdata cases for plain, aliased, dot, and blank imports, then run the analyzer with analysistest.RunWithSuggestedFixes. Done means every suggested fix uses a bound qualifier or adds a usable import and the rewritten package compiles.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.