Go: Track whether a type parameter type was declared as part of a receiver#22178
Go: Track whether a type parameter type was declared as part of a receiver#22178jketema wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Go extractor + schema to track whether a typeparam row originated from receiver type parameters, avoiding keyset collisions expected with Go 1.27’s generic methods (receiver + method type parameters).
Changes:
- Extend the
typeparamrelation keyset with a new booleanis_from_recvcolumn and plumb it through extractor emission and the QL library API (TypeParamType.isFromReceiver()). - Update the Go extractor’s type-parameter parent tracking to record whether a type parameter came from the receiver vs the method/type declaration.
- Add corresponding upgrade/downgrade steps and update the regression test output to include the new column.
Show a summary per file
| File | Description |
|---|---|
| go/ql/test/library-tests/semmle/go/Function/TypeParamType.ql | Test query updated to select isFromReceiver() output. |
| go/ql/test/library-tests/semmle/go/Function/TypeParamType.expected | Expected results updated to include receiver-origin boolean. |
| go/ql/lib/upgrades/b1341734d6870b105e5c9d168ce7dec25d7f72d0/upgrade.properties | Adds upgrade step for typeparam relation. |
| go/ql/lib/upgrades/b1341734d6870b105e5c9d168ce7dec25d7f72d0/typeparam.ql | Upgrade query populates new boolean column (defaulting to false). |
| go/ql/lib/upgrades/b1341734d6870b105e5c9d168ce7dec25d7f72d0/old.dbscheme | Upgrade metadata: old schema snapshot for this upgrade step. |
| go/ql/lib/upgrades/b1341734d6870b105e5c9d168ce7dec25d7f72d0/go.dbscheme | Upgrade metadata: new schema snapshot for this upgrade step. |
| go/ql/lib/semmle/go/Types.qll | QL API updated to expose isFromReceiver() from the underlying relation. |
| go/ql/lib/go.dbscheme.stats | Schema stats updated for the new column. |
| go/ql/lib/go.dbscheme | typeparam relation updated with is_from_recv and extended keyset. |
| go/extractor/trap/trapwriter.go | Adds boolean emission support for TRAP tuples. |
| go/extractor/extractor.go | Tracks receiver-origin for type parameters; includes it in label + emitted typeparam rows. |
| go/extractor/dbscheme/tables.go | Adds boolean column + keyset change to TypeParamTable. |
| go/extractor/dbscheme/dbscheme.go | Adds BooleanColumn helper to define boolean columns. |
| go/downgrades/5ff5325d274ae4f86defa195577bc7c1370b72fa/upgrade.properties | Adds downgrade step for typeparam relation. |
| go/downgrades/5ff5325d274ae4f86defa195577bc7c1370b72fa/typeparam.ql | Downgrade query drops the new boolean column. |
| go/downgrades/5ff5325d274ae4f86defa195577bc7c1370b72fa/old.dbscheme | Downgrade metadata: old schema snapshot for this downgrade step. |
| go/downgrades/5ff5325d274ae4f86defa195577bc7c1370b72fa/go.dbscheme | Downgrade metadata: new schema snapshot for this downgrade step. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Low
| for tparam := range typeparams.TypeParams() { | ||
| setTypeParamParent(tparam, parent, isFromReceiver) | ||
| } |
There was a problem hiding this comment.
The VSCode Go extension begs to differ.
There was a problem hiding this comment.
I believe that this syntax works correctly in newer versions of go. There would be a compilation error otherwise.
| /** Holds if this type parameter type is declared as part of a receiver */ | ||
| boolean isFromReceiver() { typeparam(this, _, _, _, _, result) } |
There was a problem hiding this comment.
Update to predicate instead of boolean, which is what I intended to do.
owen-mc
left a comment
There was a problem hiding this comment.
Looks pretty good. Just a few minor comments.
| recvTypeParams := funcObj.Type().(*types.Signature).RecvTypeParams() | ||
| populateTypeParamParents(recvTypeParams, funcObj, true) | ||
| typeParams := funcObj.Type().(*types.Signature).TypeParams() | ||
| populateTypeParamParents(typeParams, funcObj, false) |
There was a problem hiding this comment.
(non-blocking) I personally slightly prefer:
| recvTypeParams := funcObj.Type().(*types.Signature).RecvTypeParams() | |
| populateTypeParamParents(recvTypeParams, funcObj, true) | |
| typeParams := funcObj.Type().(*types.Signature).TypeParams() | |
| populateTypeParamParents(typeParams, funcObj, false) | |
| signature := funcObj.Type().(*types.Signature) | |
| populateTypeParamParents(signature.RecvTypeParams(), funcObj, true) | |
| populateTypeParamParents(signature.TypeParams(), funcObj, false) |
| for tparam := range typeparams.TypeParams() { | ||
| setTypeParamParent(tparam, parent, isFromReceiver) | ||
| } |
There was a problem hiding this comment.
I believe that this syntax works correctly in newer versions of go. There would be a compilation error otherwise.
| } else if entry.parent != newobj { | ||
| log.Fatalf("Parent of type parameter '%s %s' being set to a different value: '%s' vs '%s'", tp.String(), tp.Constraint().String(), entry.parent, newobj) |
There was a problem hiding this comment.
For completeness, we should also detect the case that entry.isFromReceiver != isFromReceiver and log it somehow. Perhaps the simplest way is to assign newEntry := typeParamParentEntry{newobj, isFromReceiver} before the conditional, then compare entry != newEntry in this conditional and log entry and newEntry instead of entry.parent and newobj.
|
|
||
| func setTypeParamParent(tp *types.TypeParam, newobj types.Object) { | ||
| obj, exists := typeParamParent[tp] | ||
| func setTypeParamParent(tp *types.TypeParam, newobj types.Object, isFromReceiver bool) { |
There was a problem hiding this comment.
(non-blocking) Consider renaming newobj to parent to match the field names of the new struct type.
|
|
||
| from TypeParamType tp, string name, CompositeType bound, TypeParamParentObject parent, int idx | ||
| where typeparam(tp, name, bound, parent, idx) | ||
| select tp, name, bound, parent, idx, false |
There was a problem hiding this comment.
It's probably fine for isReceiverParent to be incorrect sometimes, but I think we can make sure it's always correct for pre-1.27 code. So, for anyone who always uses an up-to-date version of CodeQL it will always be correct.
| from TypeParamType tp, string name, CompositeType bound, TypeParamParentObject parent, int idx | |
| where typeparam(tp, name, bound, parent, idx) | |
| select tp, name, bound, parent, idx, false | |
| // In Go 1.26 and below, a type parameter is from a receiver exactly when its | |
| // parent is a method. | |
| boolean isFromReceiver(TypeParamParentObject parent) { | |
| if methodreceivers(parent, _) then result = true else result = false | |
| } | |
| from TypeParamType tp, string name, CompositeType bound, TypeParamParentObject parent, int idx | |
| select tp, name, bound, parent, idx, isFromReceiver(parent) |
This is needed for Go 1.27, as generic methods will have type parameter types that are both defined in receivers and on the methods themselves. Tracking this as part of the keyset avoids the dataset check errors that would otherwise occur.
All the needed data is there already in Go 1.26, so target
mainand not the upgrade branch.