Go: Only output origin constraints for methods - #11
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22399 Source head: 6a9e2d5
✅ Shipwright · ApproveRecommendation: approve PR #11 · Tier
Findings (6)
Fireworks usage: 12,289 input · 822 output · 13,111 total tokens · $0.0032 · 15s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| signature := tp.Method(i).Type().(*types.Signature) | ||
| populateTypeParamParents(signature.TypeParams(), meth, false) | ||
| } | ||
| populateTypeParamParentsAndOrigins(tp.Method(i), meth) |
There was a problem hiding this comment.
Shipwright · HIGH
The new helper populateTypeParamParentsAndOrigins does two unrelated things: it records parents and records origins.
Impact: The new helper populateTypeParamParentsAndOrigins does two unrelated things: it records parents and records origins. The name is a conjunction of two responsibilities, and the body immediately delegates to two separate functions. A future maintainer reading extractType will not know from the call site which side effects are essential for correctness and which are incidental. Splitting the call into populateTypeParam…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| isFromReceiver bool | ||
| } | ||
|
|
||
| // typeParamMutex protects typeParamParent. |
There was a problem hiding this comment.
Shipwright · HIGH
The new global maps typeParamParent and typeParamOrigin are protected by separate mutexes, but getTypeParamParentLabel and getTypeParamOrigin each acquire only their own lock.
Impact: The new global maps typeParamParent and typeParamOrigin are protected by separate mutexes, but getTypeParamParentLabel and getTypeParamOrigin each acquire only their own lock. If any code path ever reads or writes both maps in a way that requires a consistent cross-map view, the separate locks provide no atomicity. More concretely, setTypeParamParent and setTypeParamOrigin are called from populateTypeParamParentsAnd…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
getTypeParamOrigin falls back to returning the input type parameter when no origin is recorded.
Impact: getTypeParamOrigin falls back to returning the input type parameter when no origin is recorded. This silently masks missing-origin bugs: if populateTypeParamOrigins is not called for some instantiated method, extraction proceeds with the instantiated constraint instead of the origin constraint, producing subtly wrong database rows rather than failing loudly. The existing getTypeParamParentLabel uses log.Fatalf for a…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| isFromReceiver bool | ||
| } | ||
|
|
||
| // typeParamMutex protects typeParamParent. |
There was a problem hiding this comment.
Shipwright · LOW
The comment above typeParamOriginMutex incorrectly says 'typeParamMutex protects typeParamOrigin.' It should reference typeParamOriginMutex.
Impact: The comment above typeParamOriginMutex incorrectly says 'typeParamMutex protects typeParamOrigin.' It should reference typeParamOriginMutex. This is a small but real documentation defect that can mislead someone reasoning about lock ownership.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| isFromReceiver bool | ||
| } | ||
|
|
||
| // typeParamMutex protects typeParamParent. |
There was a problem hiding this comment.
Shipwright · LOW
The comment for typeParamOriginMutex says 'typeParamMutex protects typeParamOrigin.' It should say 'typeParamOriginMutex protects typeParamOrigin.' This is a documentation defect i
Impact: The comment for typeParamOriginMutex says 'typeParamMutex protects typeParamOrigin.' It should say 'typeParamOriginMutex protects typeParamOrigin.' This is a documentation defect introduced by the diff.
Suggested fix: Fix the review finding before release.
1 participant
Addresses github#22042 (comment) by only outputting the origins of constraints on generic method and not instantiated constraints. This matches the behavior in other places where we have both instantiations and origins.
Source merge-base:
072b298c66bb0c085c3cc24319f00ccbfcf0c25aSource head:
6a9e2d514f6c20b0fa0eeb91145b1fc236afacb1