Allow creating aliases for builtin types, using `typename`. #133

csilvers · 2021-10-04T06:04:40Z

Summary:

This lets you write code like:

query x {
   # @genqlient(typename: "MyString")
   someStringField
}

and genqlient will do

typename MyString string
type x struct {
   someStringField MyString
}

This was not difficult to implement, though it required introducing a
new identifier type. The main difficulty I had was weird test
failures, that it turns out was due to the tests putting a bunch of
fields on the same line, so that the genqlient directive on the
previous line applied to all of them, accidentally. This became a
problem when typename suddenly started being respected for builtin
types! I fixed it by just spreading out the queries a bit.

Fixes #130

Test plan:

make check

dnerdy · 2021-10-05T02:18:30Z

@csilvers did you try using bind to map builtin scalars to nonstandard types? I believe bind is meant to address this case (whereas typename is meant to override the generated name for a type). See

genqlient/docs/genqlient.yaml

Lines 79 to 83 in 59b6df6

    
           # This is primarily used for custom scalars, or to map builtin scalars to 
        
           # a nonstandard type.  By default, builtin scalars are mapped to the 
        
           # obvious Go types (String and ID to string, Int to int, Float to float64, 
        
           # and Boolean to bool), but this setting will extend or override those 
        
           # mappings.

and

genqlient/docs/genqlient_directive.graphql

Lines 152 to 171 in 59b6df6

    
             # If set, this argument or field will use the given Go type instead of a 
        
             # genqlient-generated type. 
        
             # 
        
             # The value should be the fully-qualified type name to use for the field, 
        
             # for example: 
        
             #  time.Time 
        
             #  map[string]interface{} 
        
             #  []github.com/you/yourpkg/subpkg.MyType 
        
             # Note that the type is the type of the whole field, e.g. if your field in 
        
             # GraphQL has type `[DateTime]`, you'd do 
        
             #  # @genqlient(bind: "[]time.Time") 
        
             # (But you're not required to; if you want to map to some type DateList, 
        
             # you can do that, as long as its UnmarshalJSON method can accept a list 
        
             # of datetimes.) 
        
             # 
        
             # See bindings in genqlient.yaml for more details; this is effectively to a 
        
             # local version of that global setting and should be used with similar care. 
        
             # If set to "-", overrides any such global setting and uses a 
        
             # genqlient-generated type. 
        
             bind: String

.

csilvers · 2021-10-05T02:28:22Z

The difference is that bind requires you to define the type yourself, which can be inconvenient. (In this case we'd need a separate package just to define the type name.) typename lets genqlient define it in the genqlient package.

dnerdy

Got it. Okay, this looks reasonable for that case. Could you add an entry to the change log?

genqlient/docs/CHANGELOG.md

Line 25 in 59b6df6

### New features:

dnerdy · 2021-10-05T02:50:25Z

generate/convert.go

@@ -479,6 +479,17 @@ func (g *generator) convertDefinition(
 		return g.addType(goType, goType.GoName, pos)

 	case ast.Scalar:
+		if builtinTypes[def.Name] != "" {
+			// In this case, the user asked for a custom Go type-name
+			// for a built-in type, e.g. `typename MyString string`.


Suggested change

// for a built-in type, e.g. `typename MyString string`.

// for a built-in type, e.g. `type MyString string`.

dnerdy · 2021-10-05T02:57:08Z

generate/convert.go

+			// In this case, the user asked for a custom Go type-name
+			// for a built-in type, e.g. `typename MyString string`.
+			goType := &goTypenameForBuiltinType{
+				GoTypename:    name,


nit: I think GoTypeName would be more consistent with the rest of the code.

dnerdy · 2021-10-05T02:58:22Z

generate/types.go

+	// goTypenameForBuiltinType represents a builtin type that was
+	// given a different name due to a `typename` directive.  We
+	// create a type like `type MyString string` for it.
+	goTypenameForBuiltinType struct {


I might also name this goTypeForBuiltinType.

(I would have called it goNamedType, but any of these work!)

csilvers · 2021-10-05T03:22:44Z

I'm still not sure this is a good idea. It seems less than ideal to have two ways to override basic types, one using bind and one using typename. In general Ben is not a big fan of bind, because it turns off all the type-checking that genqlient normally does, so I am leaning toward this being a better approach. I'll change the documentation to be clearer that typename and bind can both be used to replace built-in types, but typename is preferred.

This lets you write code like: ``` query x { # @genqlient(typename: "MyString") someStringField } ``` and genqlient will do ``` typename MyString string type x struct { someStringField MyString } ``` This was not difficult to implement, though it required introducing a new identifier type. The main difficulty I had was weird test failures, that it turns out was due to the tests putting a bunch of fields on the same line, so that the genqlient directive on the previous line applied to all of them, accidentally. This became a problem when `typename` suddenly started being respected for builtin types! I fixed it by just spreading out the queries a bit. Fixes #130

csilvers · 2021-10-05T04:31:25Z

Documentation added -- if it looks ok can you approve?

StevenACoffman

LGTM now!

benjaminjkraft

Neat idea! I worry a little bit this will make people want to add methods to the type, although it's not really genqlient that's stopping that (but rather the -- probably wise -- Khan convention that generated code is in its own package).

benjaminjkraft · 2021-10-05T16:40:40Z

generate/convert.go

@@ -479,6 +479,17 @@ func (g *generator) convertDefinition(
 		return g.addType(goType, goType.GoName, pos)

 	case ast.Scalar:
+		if builtinTypes[def.Name] != "" {


In principle we should probably support this for types via global or local bind as well, if you have an int and you want to bind it to int32 and also use a named type? The best way to do that is probably to do it in the caller convertType, similar to how we do pointer. (Or you could make it a TODO until anyone actually cares.)

I went to write this TODO and realized I don't understand exactly the use case you're talking about. Can you give a concrete example? bind works with types you define, so you can already do

type MyInt int32 .... # @genqlient(bind: "path/to.MyInt") myfield

can't you?

Ah, I see, bind wins over typename, I was worried typename would win over bind which would be more confusing. So it's still the case that if you have in genqlient.yaml:

bindings: MyType: type: int32

and now you do

# @genqlient(typename: "MyOtherType") myField

the typename is ignored. Really we should combine the two, i.e.:

type MyOtherType int32

Of course it's not really clear why you would want that, so we could also explicitly error. But it's honestly probably just as easy to make it do the obvious thing. (Initially I was worried we'd do type MyOtherType int, which would be wrong.)

Did you mean we should emit type MyOtherType int32? I guess I'm not seeing what "the obvious thing" is.

Ah yes that's what I meant! Edited.

OK, I'll take a stab when I have a moment, or else add a TODO (also when I have a moment :-) )

And, to be clear, that's the obvious thing: define a new alias (from typename) to the bound type (from bind).

benjaminjkraft · 2021-10-05T16:41:43Z

generate/types.go

+	// goTypenameForBuiltinType represents a builtin type that was
+	// given a different name due to a `typename` directive.  We
+	// create a type like `type MyString string` for it.
+	goTypenameForBuiltinType struct {


(I would have called it goNamedType, but any of these work!)

In #133, Craig added support for a new use of typename, where it applies to a scalar and means that genqlient should generate a named type, e.g. `# @genqlient(typename: "MyString")` on a node of type string will generate and use `type MyString string`. But this gets a bit confusing if you mix it with `bind`; should `typename: "MyString", bind: "int32"` generate `type MyString int32`, or should one override the other, or what? Of course in practice you're not likely to write that all in one place, but you could via a global binding, or a `for` directive, and in that case probably it was a mistake. In #138, we looked at making them work together correctly, but it added complexity and got even more confusing. So instead, here, we just ban it; we can always add it back if it proves useful. (Or, you can make the `typename` win over a global binding by locally unbinding it via `bind: "-"`.) This required changes in surprisingly many places; I already knew the directive-validation code was due for a refactor but that will happen some other day. The tests show that it works, in any case. Interestingly, this problem actually could have arisen for a struct binding already, before #133. But all the same reasons it's confusing seem to apply, so I just banned it there too. This is technically a breaking change although I doubt anyone will hit it. Test plan: make check

csilvers self-assigned this Oct 4, 2021

csilvers requested review from benjaminjkraft, dnerdy and StevenACoffman October 4, 2021 06:04

dnerdy reviewed Oct 5, 2021

View reviewed changes

csilvers added 2 commits October 4, 2021 21:27

Update documentation as well.

c2a8f15

csilvers force-pushed the typename-basic-types branch from 66448b5 to c2a8f15 Compare October 5, 2021 04:29

- Rename a var based on review

63875ab

StevenACoffman approved these changes Oct 5, 2021

View reviewed changes

csilvers merged commit a52e556 into main Oct 5, 2021

csilvers deleted the typename-basic-types branch October 5, 2021 15:49

benjaminjkraft reviewed Oct 5, 2021

View reviewed changes

csilvers mentioned this pull request Oct 12, 2021

Allow the use of typename to be combined with bind. #138

Closed

benjaminjkraft mentioned this pull request Feb 10, 2022

Reject the use of both typename and bind #172

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating aliases for builtin types, using `typename`. #133

Allow creating aliases for builtin types, using `typename`. #133

csilvers commented Oct 4, 2021

dnerdy commented Oct 5, 2021 •

edited

Loading

csilvers commented Oct 5, 2021 •

edited

Loading

dnerdy left a comment

dnerdy Oct 5, 2021

dnerdy Oct 5, 2021

dnerdy Oct 5, 2021

benjaminjkraft Oct 5, 2021

csilvers commented Oct 5, 2021

csilvers commented Oct 5, 2021

StevenACoffman left a comment

benjaminjkraft left a comment

benjaminjkraft Oct 5, 2021

csilvers Oct 7, 2021

benjaminjkraft Oct 11, 2021 •

edited

Loading

csilvers Oct 11, 2021

benjaminjkraft Oct 12, 2021

csilvers Oct 12, 2021

benjaminjkraft Oct 12, 2021

benjaminjkraft Oct 5, 2021

	// for a built-in type, e.g. `typename MyString string`.
	// for a built-in type, e.g. `type MyString string`.

Allow creating aliases for builtin types, using typename. #133

Allow creating aliases for builtin types, using typename. #133

Conversation

csilvers commented Oct 4, 2021

Summary:

Test plan:

dnerdy commented Oct 5, 2021 • edited Loading

csilvers commented Oct 5, 2021 • edited Loading

dnerdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csilvers commented Oct 5, 2021

csilvers commented Oct 5, 2021

StevenACoffman left a comment

Choose a reason for hiding this comment

benjaminjkraft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminjkraft Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow creating aliases for builtin types, using `typename`. #133

Allow creating aliases for builtin types, using `typename`. #133

dnerdy commented Oct 5, 2021 •

edited

Loading

csilvers commented Oct 5, 2021 •

edited

Loading

benjaminjkraft Oct 11, 2021 •

edited

Loading