Go: Update to 1.27#22042
Conversation
jketema
added
the
depends on internal PR
| recvTypeParams := funcObj.Type().(*types.Signature).RecvTypeParams() | ||
| populateTypeParamParents(recvTypeParams, obj, typeParams.Len()) |
There was a problem hiding this comment.
In theory these lines shouldn't be needed here, because the comment above says that methods do not appear as objects in any scope, so we should only be dealing with top-level functions. However, rather than deleting it, I think it would be better to create a separate function for these four lines, as they are duplicated lower down. Maybe populateTypeParamParentsFromFunction(funcObj *types.Func). (Can you tell I like long method names? 😆 )
There was a problem hiding this comment.
Happy to refactor. The only thing I changed here was the addition of the offset. I simply assumed that this code was necessary (because it was already there 😄 ).
| for i := 0; i < origintp.NumMethods(); i++ { | ||
| meth := origintp.Method(i).Origin() | ||
|
|
||
| typeParams := tp.Method(i).Type().(*types.Signature).TypeParams() |
There was a problem hiding this comment.
Are you sure that this line wants to use the instantiated receiver type (tp instead of origintp), and the instantiated method type (tp.Method(i).Type() rather than tp.Method(i).Type().Origin())? It seems odd to assign meth as the parent in the line below when meth is different from tp.Method(i). Do we even need to populate type param parents here, as we will do something very similar in extractMethods.
Perhaps we should put a short comment here, as it will no doubt be confusing to future readers as well.
There was a problem hiding this comment.
As far as I can tell, it needs to be tp, or at least that solves the Parent of type parameter does not exist: Q any error I was seeing. I'm absolutely not sure if meth is correct here. I think I need to better understand what relations are affected by populateTypeParamParents.
| log.Fatalf("Parent of type parameter '%s %s' being set to a different value: '%s' vs '%s'", tp.String(), tp.Constraint().String(), obj, newobj) | ||
| typeParamParent[tp] = typeParamParentEntry{newobj, idx} | ||
| } else if entry.parent != newobj || entry.index != idx { | ||
| 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.
This error message is slightly inaccurate, as it only prints the objects but it could be printed if the objects are the same but the indexes are different. It's not a big thing, but consider printing the indexes as well (or the typeParamParentEntry struct.
| parentlbl, idx := getTypeParamParentLabel(tw, tp) | ||
| constraintLabel := extractType(tw, tp.Constraint()) | ||
| dbscheme.TypeParamTable.Emit(tw, lbl, tp.Obj().Name(), constraintLabel, parentlbl, tp.Index()) | ||
| dbscheme.TypeParamTable.Emit(tw, lbl, tp.Obj().Name(), constraintLabel, parentlbl, idx) |
There was a problem hiding this comment.
We should update the QLDoc for TypeParamType.getIndex() to be clear about what it means, especially as it is now different from what the go compiler libraries use. For a type param in a named type definition it is what it was before: the index in the type params for that named type. For a type param in a method definition it is now the index in the list of type params, where the type params on the method come first and the type params on the receiver come second. (Do we want this order? From left to right might make more sense.)
There was a problem hiding this comment.
I'm happy to switch the order. I just kept things in the order in which the code appears for now.
| func (*StructForGenericMethod1) GenericMethod1[P any](x P) {} | ||
|
|
||
| type StructForGenericMethod2[P any] struct{} | ||
|
|
||
| func (*StructForGenericMethod2[P]) GenericMethod2[Q any](x Q) {} |
There was a problem hiding this comment.
These lines use P as the name for three different type parameters. That makes it hard to understand the test output. I highly recommend giving them different names, even if it's just P1, P2 and P3.
2 participants
No description provided.