Guard code-quality recipes against emitting invalid Go - #53
Conversation
Validated the recipes against open-source repositoriesRan every recipe changed or added in this PR against 5 open-source Go repos: cobra, gin, logrus, mux, and websocket. For each recipe that changed a file, the result was applied and compiled with 15 fired, 24 no-op, 0 errors, and every fired recipe compiled on every repo it touched (no invalid Go).
The remaining 24 recipes were no-ops (no matching idioms in these repos). Coverage caveats (pre-existing rewrite-go tooling, not these recipes)
These numbers are therefore a floor, not a ceiling. Where the tooling could process the code, the recipes were clean. |
greg-at-moderne
left a comment
There was a problem hiding this comment.
LGTM.
I have mostly reviewed the tests - i.e. focusing on the behavior of the recipes. I think these are very good additions. I found it hard to analyze each recipe change as they were all kind of different changes.
One tiny remark is the one-line test summaries are probably excessive, e.g.
// Skips a real newline escape, which a raw string would embed as a literal line break.
| * Moderne Proprietary. Only for use by Moderne customers under the terms of a commercial contract. | ||
| */ | ||
|
|
||
| // Package lstutil holds small helpers shared across recipe packages for |
There was a problem hiding this comment.
Should it be moved to https://github.com/openrewrite/rewrite/tree/main/rewrite-go instead?
There was a problem hiding this comment.
Good call, these are generic enough to belong in rewrite-go. I'd suggest doing it as a follow-up rather than in this PR, since moving them upstream needs a rewrite-go PR, a release, and then a go.mod bump here to consume it, which would block this (already-approved) change on the release cycle.
When I pick up the follow-up I'll first check whether rewrite-go already exposes equivalents (e.g. a cursor helper for "is this block a function body") so we don't duplicate, and probably only upstream the generic ones (IsFunctionBodyBlock, BaseIndent, IsInitWrappedIf); IsErrNotNil is recipe-specific enough that it may stay local.
There was a problem hiding this comment.
Here's the follow-up PR: openrewrite/rewrite#8458
Problem
Several recipes rewrote code without checking the surrounding type or arity, so they could produce Go that does not compile. Issue #30 reported six:
PreferStrconvAtoi,PreferOsReadDir,PreferStringsNewReader,PreferRawStringForRegex,ReduceNestingDepth, andHandleErrorReturn. The failures fall into two modes. The first swaps a call or expression for one of a different type or arity, for example replacing a value withstrconv.Atoi(...)where the code needs anint64, or using a two-valueregexpcall in a single-value position. The second synthesizes a statement, areturn, anif err != nil { return err }, or an assignment, without checking the enclosing function signature or variable scope.How it has been addressed
Every affected recipe now guards its rewrite and skips whenever it cannot prove the output compiles. Type-substitution recipes check the required type at the rewrite site, whether that is the function's result position, a typed declaration, or an argument, before firing. Statement-synthesis recipes confirm they sit at a function-body block, that the enclosing function returns the expected single
error, and that the value being handled is genuinely an error. Shared checks are consolidated into helpers (lstutil,type_context,errors_is_common) so the guards live in one place. The audit went past the reported six, since the same root cause was present elsewhere, and the fix now spans more than thirty recipes across the repository. The guards are deliberately conservative: when a recipe cannot confirm safety, it leaves the code untouched rather than risk an invalid rewrite.Potential gaps
PreferStrconvAtoiandPreferOsReadDirare guarded for direct contexts such as a return or a typed declaration, but can still emit invalid code when the result escapes through a short variable declaration, for exampleentries := os.ReadDir(...)followed byentries[i].Size(). The LST collapses all integer widths to one type and drops slice element types, so these contexts cannot be distinguished yet. The residuals are documented in the recipes.PreferStrconvItoais left as a documented limitation forint64anduintarguments for the same integer-width reason, pending the upstream LST fix in openrewrite/rewrite#8406.SimplifySingleCaseSelectandRemoveUnconditionalValueOverwrite. These are out of scope here and are not addressed.