feat: add go sdk pragma parsing for default and optional

[jedevc]

Fixes #6162.

This allows defining defaults and optional parameters for Go modules using "dagger" pragmas. These are inspired in style by "go" pragmas.

Thankfully, this is actually a pretty simple patch, so we can bikeshed exact syntax - the earlier refactoring of opts through parsing args into paramSpecs makes this relatively simple to work with (also because of the way baseType and paramType are separated out, it's no extra effort to keep supporting the Optional generic as well, though I do think if this approach is successful, we might want to remove that at some point).

Personally, I think I prefer the more explicit //dagger: default=abc instead of //+default abc, but I also don't realllly have strong opinions on this. I've written the code so we just need to update the regexp and the respective tests for this - no other code is dependent on this.

[jedevc]

Note, this PR only adds support for this behavior for arguments, we'll need a follow-up for struct fields - which should be pretty easy if this approach looks good!

[vito]

Nice!

Personally, I think I prefer the more explicit //dagger: default=abc instead of //+default abc, but I also don't realllly have strong opinions on this. I've written the code so we just need to update the regexp and the respective tests for this - no other code is dependent on this.

My vote is for // +optional and // +default abc since this is gonna be repeated a ton of times in the grand scheme of things. Every keystroke counts given the sheer amount of APIs that people can/will write.

I don't think we need to be as explicit as Go since we can safely assume the comment is written for Dagger if we're discovering it while crawling a module type. Go has to disambiguate from all possible code comments, but we don't afaict. Even K8s, where we originally drew this inspiration from, just uses // +optional - so it seems fine.

[sipsma]

I don't think we need to be as explicit as Go since we can safely assume the comment is written for Dagger if we're discovering it while crawling a module type. Go has to disambiguate from all possible code comments, but we don't afaict. Even K8s, where we originally drew this inspiration from, just uses // +optional - so it seems fine.

That's actually the type of corner case that feels like a slight argument for a dagger: prefix; if we choose something super generic like +optional we could end up accidentally interpreting comments on structs from outside dagger in ways that weren't intended (specifically in combination with #6185)

To be clear, the situations in which this would happen are so obscure that they're right on the border of whether I think we should even care or not. It would have to be that someone is using a struct that just so happens to have a comment line with // +optional but doesn't actually want dagger to interpret it as such. This would get slightly worse as we expand to more than just optional (private, etc.), but even still...

However, if we do use a prefix like //dagger: then the corner cases become so solidly obscure that I wouldn't care anymore.

I guess my vote would be a //dagger: prefix, with support for including multiple settings on a single comment if desired (i.e. //dagger: optional,private,default=abc) to alleviate a little bit of the verbosity in simple situations. Then if the future comes along where we get tired of typing it and the obscure corner cases never manifest we can pretty easily still switch to also supporting the extremely terse syntax.

Also, I'd be find if it was even just //dag: as the prefix, so then it's only 3 extra chars vs +.

[sipsma]

Overall looks great!

Echoing my other comment, I'd probably air on the side of dag: as the prefix (but could still be convinced of +, not incredibly opinionated here). And support for additionally being able to combine the annotations onto a single line if desired would be nice, but can easily be a follow up.

Only last question is what the failure mode is currently for types that don't make sense to have defaults in this approach, i.e. Container.

I don't want to support that at all to be clear, just want to make sure the error message is comprehensible and/or we don't crash :-D

[sipsma]

Are there situations in which this is expected to happen now? just curious

[jedevc]

I don't think it was really possible before either actually - since we only ever ran commentForFuncField on functions in the current module, and they were always parsed. Even in the context of #6185, where functions might be outside of the current package, then the fset is still populated for files in those other packages (though they haven't been fully parsed into a syntax tree).

So really this case wasn't actually possible to trigger, so we don't need to bother about returning an error (and even if there's a bizarre edge case, this just means that we don't return any comments for that field, which seems like the right behavior).

[jedevc]

Only last question is what the failure mode is currently for types that don't make sense to have defaults in this approach, i.e. Container.

Yeah this is actually fine - in this case, we end up generating a weird DefaultValue like:

						dag.TypeDef().WithObject("Directory")).
							WithDescription("Builds a Hugo site\n").
							WithArg("target", dag.TypeDef().WithObject("Directory"), FunctionWithArgOpts{Description: "Directory containing the Hugo site", DefaultValue: JSON("\"foobar\"")}).

But, the server errors out "nicely" in this case:

┃ Error: failed to get loaded module ID: input:1: host.directory.asModule.serve failed to convert module to executable schema: expected object default value
┃ got string         

The code for this is in

dagger/core/schema/module.go

Lines 1219 to 1222 in 24e552a

	mapVal, ok := val.(map[string]any)
	if !ok {
	return nil, fmt.Errorf("expected object default value, got %T", val)
	}

[jedevc]

Ok, so trying to steer this towards a decision - I think I'd rather have +optional instead of dag:optional (have pushed an update for this).

Couple key things from above that sway it for me:

• "this is gonna be repeated a ton of times in the grand scheme of things"
• "we can safely assume the comment is written for Dagger if we're discovering it while crawling a module type"
• "the situations in which this would happen are so obscure that they're right on the border of whether I think we should even care or not"
  • If this really becomes a problem with feat: allow go modules to accept/return foreign types #6185, we could restrict the scope of that PR to only allow sub-modules (which I think we might have to do anyways to deal with feat: allow go modules to accept/return foreign types #6185 (comment)).

I think we should take the + route now, but revisit this after we release and get some feedback - we can always choose to do something different later, I think the more important switch is to using comments at all.

[sipsma]

Ok, so trying to steer this towards a decision - I think I'd rather have +optional instead of dag:optional (have pushed an update for this).

SGTM, like I said the cases where it matters were right on the border of being meaningful or not, so not gonna block on splitting micro-hairs :-)

Taking another quick look at the updated code now