ethereum-optimism / ethereum-optimism/optimism
linter/bigint: suggested fix emits an unbound qualifier when op-service/bigs is aliased or dot-imported
- 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
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