Skip to content

Go: Only output origin constraints for methods #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anurag6569201 wants to merge 1 commit into
base: qa/agent-github-codeql/pr-11-22399/base
Choose a base branch
from
Open

Go: Only output origin constraints for methods #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 90 additions & 24 deletions go/extractor/extractor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,16 @@ type typeParamParentEntry struct {
isFromReceiver bool
}

// typeParamMutex protects typeParamParent.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var typeParamParentMutex sync.RWMutex

var typeParamParent map[*types.TypeParam]typeParamParentEntry = make(map[*types.TypeParam]typeParamParentEntry)

// typeParamMutex protects typeParamOrigin.
var typeParamOriginMutex sync.RWMutex

var typeParamOrigin map[*types.TypeParam]*types.TypeParam = make(map[*types.TypeParam]*types.TypeParam)

func init() {
// this sets the number of threads that the Go runtime will spawn; this is separate
// from the number of goroutines that the program spawns, which are scheduled into
Expand Down Expand Up @@ -1658,29 +1666,7 @@ func extractType(tw *trap.Writer, tp types.Type) trap.Label {
for i := 0; i < origintp.NumMethods(); i++ {
meth := origintp.Method(i).Origin()
extractMethod(tw, meth)

// Consider a generic struct and a generic method:
//
// type S[P any] struct{}
// func (*S[P]) m[Q any](x Q) {}
//
// If we have a variable 's' of type 'S[int]' and the expression
// 's.m[string]("")', then the type of the selector expression 's.m'
// is ' func(Q)'. The method 'm' here is an instantiation of the
// declaration, which has its own type with type parameter 'Q'.
// As we do not extract method instantiations, 'populateTypeParamParents'
// does not automatically get called for the type parameter 'Q'
// from the instantiation of 'm'. To compensate, we add the type
// parameters here.
//
// As a parent we use the origin method. This suffices, as the name
// and index of the type parameter in the instantiation will be
// identical to those of the uninstantiated method, and as only
// these two properties will be extracted for a type parameter.
if tp.Method(i) != meth {
signature := tp.Method(i).Type().(*types.Signature)
populateTypeParamParents(signature.TypeParams(), meth, false)
}
populateTypeParamParentsAndOrigins(tp.Method(i), meth)

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

underlyingInterface, underlyingIsInterface := underlying.(*types.Interface)
Expand All @@ -1704,7 +1690,8 @@ func extractType(tw *trap.Writer, tp types.Type) trap.Label {
case *types.TypeParam:
kind = dbscheme.TypeParamType.Index()
parentlbl, isReceiverChild := getTypeParamParentLabel(tw, tp)
constraintLabel := extractType(tw, tp.Constraint())
constraint := getTypeParamOrigin(tp).Constraint()
constraintLabel := extractType(tw, constraint)
dbscheme.TypeParamTable.Emit(tw, lbl, tp.Obj().Name(), constraintLabel, parentlbl, tp.Index(), isReceiverChild)
case *types.Union:
kind = dbscheme.TypeSetLiteral.Index()
Expand Down Expand Up @@ -2062,7 +2049,10 @@ func getObjectBeingUsed(tw *trap.Writer, ident *ast.Ident) types.Object {
}

func getTypeParamParentLabel(tw *trap.Writer, tp *types.TypeParam) (trap.Label, bool) {
typeParamParentMutex.RLock()
entry, exists := typeParamParent[tp]
typeParamParentMutex.RUnlock()

if !exists {
log.Fatalf("Parent of type parameter does not exist: %s %s", tp.String(), tp.Constraint().String())
}
Expand All @@ -2074,6 +2064,9 @@ func getTypeParamParentLabel(tw *trap.Writer, tp *types.TypeParam) (trap.Label,
}

func setTypeParamParent(tp *types.TypeParam, parent types.Object, isFromReceiver bool) {
typeParamParentMutex.Lock()
defer typeParamParentMutex.Unlock()

entry, exists := typeParamParent[tp]
newEntry := typeParamParentEntry{parent, isFromReceiver}
if !exists {
Expand Down Expand Up @@ -2121,3 +2114,76 @@ func checkObjectNotSpecialized(obj types.Object) {
}
}
}

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// getTypeParamOrigin returns the origin type parameter for a type parameter
// from an instantiated method.
func getTypeParamOrigin(tp *types.TypeParam) *types.TypeParam {
typeParamOriginMutex.RLock()
origin, exists := typeParamOrigin[tp]
typeParamOriginMutex.RUnlock()

if exists {
return origin
} else {
return tp
}
}

// populateTypeParamParentsAndOrigins records for each type parameter of a method
// the origin parent and type parameter.
//
// Consider a generic struct and a generic method:
//
// type S[P any] struct{}
// func (*S[P]) m[Q ~P](x Q) {}
//
// If we have a variable 's' of type 'S[int]' and the expression 's.m[int](42)',
// then the type of the selector expression 's.m' is 'func[Q ~int](Q)'. The
// method 'm' here is an instantiation of the declaration, which has its own
// type with type parameter 'Q' with constraint 'interface { ~int }'. As we
// do not extract method instantiations, but only their origins, we want to
// match this behavior for the constraints of instantiated type parameter, and
// record their origin. Moreover, not extracting instantiations also means that
// 'populateTypeParamParents' does not automatically get called on their type
// parameters. To compensate, we add the type params here by calling
// `setTypeParamParent`.
//
// As the parent of a type parameter use the origin method. This suffices, as
// the name and index of the type parameter in the instantiation will be
// identical to those of the uninstantiated method, and as only a constraint and
// these two properties will be extracted for a type parameter.
func populateTypeParamParentsAndOrigins(meth *types.Func, originmeth *types.Func) {
if meth == originmeth {
return
}

typeparams := meth.Type().(*types.Signature).TypeParams()
populateTypeParamParents(typeparams, originmeth, false)

origintypeparams := originmeth.Type().(*types.Signature).TypeParams()
populateTypeParamOrigins(meth, typeparams, origintypeparams)
}

func populateTypeParamOrigins(meth *types.Func, typeparams *types.TypeParamList, origintypeparams *types.TypeParamList) {
if typeparams.Len() != origintypeparams.Len() {
log.Fatalf("Method instantiation %s has %d type parameters, origin has %d",
meth, typeparams.Len(), origintypeparams.Len())
}

for j := 0; j < typeparams.Len(); j++ {
setTypeParamOrigin(typeparams.At(j), origintypeparams.At(j))
}
}

func setTypeParamOrigin(typeparam *types.TypeParam, origintypeparam *types.TypeParam) {
typeParamOriginMutex.Lock()
defer typeParamOriginMutex.Unlock()

entry, exists := typeParamOrigin[typeparam]
if !exists {
typeParamOrigin[typeparam] = origintypeparam
} else if entry != origintypeparam {
log.Fatalf("Origin of type parameter '%s %s' being set to a different value: '%s' vs '%s'",
typeparam.String(), typeparam.Constraint().String(), entry.String(), origintypeparam.String())
}
}
5 changes: 5 additions & 0 deletions go/ql/test/library-tests/semmle/go/Function/TypeParamType.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ numberOfTypeParameters
| genericMethods.go:5:33:5:46 | GenericMethod1 | 1 |
| genericMethods.go:7:6:7:28 | StructForGenericMethod2 | 1 |
| genericMethods.go:9:37:9:50 | GenericMethod2 | 2 |
| genericMethods.go:21:6:21:29 | StructWithDependentBound | 1 |
| genericMethods.go:23:38:23:68 | GenericMethodWithDependentBound | 2 |
#select
| codeql-go-tests/function.EdgeConstraint | 0 | | Node | interface { } |
| codeql-go-tests/function.Element | 0 | | S | interface { } |
Expand Down Expand Up @@ -51,6 +53,9 @@ numberOfTypeParameters
| codeql-go-tests/function.StructForGenericMethod2 | 0 | | P2 | interface { } |
| codeql-go-tests/function.StructForGenericMethod2.GenericMethod2 | 0 | | P4 | interface { } |
| codeql-go-tests/function.StructForGenericMethod2.GenericMethod2 | 0 | from receiver | P3 | interface { } |
| codeql-go-tests/function.StructWithDependentBound | 0 | | P5 | interface { } |
| codeql-go-tests/function.StructWithDependentBound.GenericMethodWithDependentBound | 0 | | P7 | interface { ~[]P6 } |
| codeql-go-tests/function.StructWithDependentBound.GenericMethodWithDependentBound | 0 | from receiver | P6 | interface { } |
| codeql-go-tests/function.multipleAnonymousTypeParamsFunc | 0 | | _ | interface { } |
| codeql-go-tests/function.multipleAnonymousTypeParamsFunc | 1 | | _ | interface { string } |
| codeql-go-tests/function.multipleAnonymousTypeParamsFunc | 2 | | _ | interface { } |
Expand Down
9 changes: 9 additions & 0 deletions go/ql/test/library-tests/semmle/go/Function/genericMethods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,12 @@ func generic_methods(s1 StructForGenericMethod1, s2 StructForGenericMethod2[int]
s1.GenericMethod1("hello")
s2.GenericMethod2(42)
}

type StructWithDependentBound[P5 any] struct{}

func (*StructWithDependentBound[P6]) GenericMethodWithDependentBound[P7 ~[]P6](x P7) {}

func genericMethodDependentBounds(t1 StructWithDependentBound[int], t2 StructWithDependentBound[string]) {
t1.GenericMethodWithDependentBound([]int{})
t2.GenericMethodWithDependentBound([]string{})
}
3 changes: 3 additions & 0 deletions go/ql/test/library-tests/semmle/go/Function/getParameter.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
| genericMethods.go:9:37:9:50 | GenericMethod2 | 0 | genericMethods.go:9:60:9:60 | x |
| genericMethods.go:11:6:11:20 | generic_methods | 0 | genericMethods.go:11:22:11:23 | s1 |
| genericMethods.go:11:6:11:20 | generic_methods | 1 | genericMethods.go:11:50:11:51 | s2 |
| genericMethods.go:23:38:23:68 | GenericMethodWithDependentBound | 0 | genericMethods.go:23:80:23:80 | x |
| genericMethods.go:25:6:25:33 | genericMethodDependentBounds | 0 | genericMethods.go:25:35:25:36 | t1 |
| genericMethods.go:25:6:25:33 | genericMethodDependentBounds | 1 | genericMethods.go:25:69:25:70 | t2 |
| main.go:7:6:7:7 | f1 | 0 | main.go:7:9:7:9 | x |
| main.go:9:12:9:13 | f2 | 0 | main.go:9:15:9:15 | x |
| main.go:9:12:9:13 | f2 | 1 | main.go:9:18:9:18 | y |
Expand Down
2 changes: 2 additions & 0 deletions go/ql/test/library-tests/semmle/go/Function/getTypeParameter.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@
| genericMethods.go:5:1:5:63 | function declaration | MethodDecl | 0 | genericMethods.go:5:48:5:53 | type parameter declaration | 0 | genericMethods.go:5:48:5:49 | P1 | genericMethods.go:5:51:5:53 | any | interface { } |
| genericMethods.go:7:6:7:45 | type declaration specifier | TypeSpec | 0 | genericMethods.go:7:30:7:35 | type parameter declaration | 0 | genericMethods.go:7:30:7:31 | P2 | genericMethods.go:7:33:7:35 | any | interface { } |
| genericMethods.go:9:1:9:67 | function declaration | MethodDecl | 0 | genericMethods.go:9:52:9:57 | type parameter declaration | 0 | genericMethods.go:9:52:9:53 | P4 | genericMethods.go:9:55:9:57 | any | interface { } |
| genericMethods.go:21:6:21:46 | type declaration specifier | TypeSpec | 0 | genericMethods.go:21:31:21:36 | type parameter declaration | 0 | genericMethods.go:21:31:21:32 | P5 | genericMethods.go:21:34:21:36 | any | interface { } |
| genericMethods.go:23:1:23:87 | function declaration | MethodDecl | 0 | genericMethods.go:23:70:23:77 | type parameter declaration | 0 | genericMethods.go:23:70:23:71 | P7 | genericMethods.go:23:73:23:77 | type set literal | interface { ~[]P6 } |