add support for `time.Time` #25

michaellzc · 2023-08-08T17:17:01Z

related #11

TODO

support *time.Time

I am not sure what is the best way to render *time.Time:

✅ generic (~~this is not supported given the current target Go version is 1.15 in go.mod~~)

// an exported func from valast
func Ptr[V any](v V) *V {
	return &v
}

// render as
valast.Ptr(time.Date(...))

❌ inline anonymous fn

// render as
func(t time.Time) *time.Time { return &t }(time.Date(...))

thoughts?

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

Test plan

all tests passed

go test                                                                                                                                                                                                                                                                                                       13:00:24
PASS
ok      github.com/hexops/valast        0.960s

Tested this change with workspace in https://github.com/sourcegraph/controller, and it seems to work just fine. It was able to automatically wrap *time.Time to Ptr(time.Date()) and valast pkg was imported properly in _test.go file as well.

emidoots · 2023-08-08T17:28:41Z

Thanks for sending @michaellzc - I'd be happy to merge this.

I can offer a few options:

I can merge this as-is, without support for *time.Time - I'm content with doing that.
You can implement *time.Time using the generic Ptr approach, I would be happy to have that change and require the latest-and-greatest Go version in go.mod.

I'll let you decide which one you want to go for depending on how much time you want to spend here.

michaellzc · 2023-08-08T17:40:26Z

2. You can implement *time.Time using the generic Ptr approach, I would be happy to have that change and require the latest-and-greatest Go version in go.mod.

😛 I need *time.Time for my own use case, so I'll continue on option 2 with a twist:

// an exported func from autogold
func TimePtr(t time.Time) *time.Time {
	return &t
}

// render as
autogold.TimePtr(time.Date(...))

SomethingPtr is quite a common pattern in many Go projects, e.g., k8s.io/utils/pointer, github.com/aws/jsii-runtime-go.

it's not elegant, but we can avoid breaking backward compatibility unless we have to.

emidoots · 2023-08-08T17:50:22Z

@michaellzc I'm not a fan of that TimePtr change - that feels awkward and a bit cluttery of the API space. I have zero qualms about breaking compatibility, so please do the Ptr approach unless there's something that makes that much more difficult

michaellzc · 2023-08-08T17:51:45Z

nice, let's do the Ptr then

valast.go

codecov-commenter · 2023-08-08T20:36:19Z

Codecov Report

Merging #25 (1c77fe9) into main (5de3f44) will increase coverage by 1.02%.
The diff coverage is 97.14%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   74.36%   75.38%   +1.02%     
==========================================
  Files           6        7       +1     
  Lines         745      780      +35     
==========================================
+ Hits          554      588      +34     
- Misses        144      145       +1     
  Partials       47       47

Files Changed	Coverage Δ
ptr.go	`0.00% <0.00%> (ø)`
valast.go	`82.22% <100.00%> (+1.45%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

emidoots · 2023-08-08T20:38:03Z

@michaellzc not sure if you'd be up for it (fine if not) but maybe you could remove valast.Addr in favor of valast.Ptr? It's doing the same thing but results in uglier ASTs as it doesn't use generics:

valast/valast.go

Lines 151 to 167 in 5de3f44

    
           // Addr returns a pointer to the given value. 
        
           // 
        
           // It is the only way to create a reference to certain values within a Go expression, 
        
           // for example since &"hello" is illegal, it can instead be written in a single expression as: 
        
           // 
        
           //	valast.Addr("hello").(*string) 
        
           func Addr(v interface{}) interface{} { 
        
           	vv := reflect.ValueOf(v) 
        
           	// Create a slice with v in it so that we have an addressable value. 
        
           	sliceType := reflect.SliceOf(vv.Type()) 
        
           	slice := reflect.MakeSlice(sliceType, 1, 1) 
        
           	if v != nil { 
        
           		slice.Index(0).Set(vv) 
        
           	} 
        
           	return slice.Index(0).Addr().Interface() 
        
           }

No worries if not, just let me know what you decide & I'll merge either way.

michaellzc · 2023-08-08T21:13:12Z

valast.go

-				AST: &ast.TypeAssertExpr{
-					X: &ast.CallExpr{
-						Fun: &ast.SelectorExpr{
-							X:   ast.NewIdent("valast"),
-							Sel: ast.NewIdent("Addr"),
-						},
-						Args: []ast.Expr{elem.AST},
+				AST: &ast.CallExpr{
+					Fun: &ast.SelectorExpr{
+						X:   ast.NewIdent("valast"),
+						Sel: ast.NewIdent("Ptr"),
 					},
-					Type: ptrType.AST,
+					Args: []ast.Expr{elem.AST},


michaellzc · 2023-08-08T21:13:20Z

valast.go

-				AST: &ast.TypeAssertExpr{
-					X: &ast.CallExpr{
-						Fun: &ast.SelectorExpr{
-							X:   ast.NewIdent("valast"),
-							Sel: ast.NewIdent("Addr"),
-						},
-						Args: []ast.Expr{elem.AST},
+				AST: &ast.CallExpr{
+					Fun: &ast.SelectorExpr{
+						X:   ast.NewIdent("valast"),
+						Sel: ast.NewIdent("Ptr"),
 					},
-					Type: ptrType.AST,
+					Args: []ast.Expr{elem.AST},


michaellzc · 2023-08-08T21:14:00Z

@michaellzc not sure if you'd be up for it (fine if not) but maybe you could remove valast.Addr in favor of valast.Ptr? It's doing the same thing but results in uglier ASTs as it doesn't use generics:

valast/valast.go

Lines 151 to 167 in 5de3f44

// Addr returns a pointer to the given value.

//

// It is the only way to create a reference to certain values within a Go expression,

// for example since &"hello" is illegal, it can instead be written in a single expression as:

//

// valast.Addr("hello").(*string)

func Addr(v interface{}) interface{} {

vv := reflect.ValueOf(v)

// Create a slice with v in it so that we have an addressable value.

sliceType := reflect.SliceOf(vv.Type())

slice := reflect.MakeSlice(sliceType, 1, 1)

if v != nil {

slice.Index(0).Set(vv)

}

return slice.Index(0).Addr().Interface()

}

No worries if not, just let me know what you decide & I'll merge either way.

refactor from Addr to Ptr is done.

Trying to understand what is going in AddrInterface see if we can get rid of it as well

michaellzc · 2023-08-08T21:52:18Z

@michaellzc not sure if you'd be up for it (fine if not) but maybe you could remove valast.Addr in favor of valast.Ptr? It's doing the same thing but results in uglier ASTs as it doesn't use generics:

valast/valast.go

Lines 151 to 167 in 5de3f44

// Addr returns a pointer to the given value.

//

// It is the only way to create a reference to certain values within a Go expression,

// for example since &"hello" is illegal, it can instead be written in a single expression as:

//

// valast.Addr("hello").(*string)

func Addr(v interface{}) interface{} {

vv := reflect.ValueOf(v)

// Create a slice with v in it so that we have an addressable value.

sliceType := reflect.SliceOf(vv.Type())

slice := reflect.MakeSlice(sliceType, 1, 1)

if v != nil {

slice.Index(0).Set(vv)

}

return slice.Index(0).Addr().Interface()

}

No worries if not, just let me know what you decide & I'll merge either way.

refactor from Addr to Ptr is done.

Trying to understand what is going in AddrInterface see if we can get rid of it as well

possible solution for future reference:

// It doesn't work for all cases yet
func InterfacePtr[V any, I any](v *V, pointerToType *I) *I {
	sliceType := reflect.SliceOf(reflect.TypeOf(pointerToType).Elem())
	slice := reflect.MakeSlice(sliceType, 1, 1)
	if v != nil {
		slice.Index(0).Set(reflect.ValueOf(v))
	}
	return slice.Index(0).Addr().Interface().(*I)
}

I may find some existing problems with AddrInterface, so I am going to leave it as it is.

Many tests are not setting options to ExportedOnly, and the rendered code contains a private struct foo from internal/test pkg.
Rendered output of TestPointers/interface is not valid Go

// input
v := test.Bazer(test.NewBaz())
&v

// output
valast.AddrInterface(test.Bazer{&test.Baz{
	Bam:  (1.34 + 0i),
	zeta: &test.foo{bar: "hello"},
}},
	(*test.Bazer)(nil)).(*test.Bazer)

I don't think you can do test.Bazer{&test.Baz{}} with an interface in Go (Bazer is an interface)

michaellzc force-pushed the 08-08-add_support_for_time.Time branch 5 times, most recently from d12b5d9 to 60af2cf Compare August 8, 2023 20:00

michaellzc marked this pull request as ready for review August 8, 2023 20:03

add support for time.Time

e44ce5a

michaellzc force-pushed the 08-08-add_support_for_time.Time branch from 60af2cf to e44ce5a Compare August 8, 2023 20:06

michaellzc changed the title ~~add support for time.Time~~ add support for time.Time Aug 8, 2023

emidoots reviewed Aug 8, 2023

View reviewed changes

valast.go Outdated Show resolved Hide resolved

Update valast.go

1c77fe9

refactor: replace Addr with generic Ptr method

1ecd87b

michaellzc commented Aug 8, 2023

View reviewed changes

michaellzc requested a review from emidoots August 8, 2023 21:55

emidoots approved these changes Aug 8, 2023

View reviewed changes

emidoots merged commit 465506f into hexops:main Aug 8, 2023

michaellzc deleted the 08-08-add_support_for_time.Time branch August 8, 2023 22:02

bobheadxi mentioned this pull request Jul 24, 2024

customtype: add ability to register any custom type #27

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for `time.Time` #25

add support for `time.Time` #25

michaellzc commented Aug 8, 2023 •

edited

Loading

emidoots commented Aug 8, 2023

michaellzc commented Aug 8, 2023 •

edited

Loading

emidoots commented Aug 8, 2023

michaellzc commented Aug 8, 2023

codecov-commenter commented Aug 8, 2023

emidoots commented Aug 8, 2023

michaellzc Aug 8, 2023

michaellzc Aug 8, 2023

michaellzc commented Aug 8, 2023

michaellzc commented Aug 8, 2023 •

edited

Loading

add support for time.Time #25

add support for time.Time #25

Conversation

michaellzc commented Aug 8, 2023 • edited Loading

Test plan

emidoots commented Aug 8, 2023

michaellzc commented Aug 8, 2023 • edited Loading

emidoots commented Aug 8, 2023

michaellzc commented Aug 8, 2023

codecov-commenter commented Aug 8, 2023

Codecov Report

emidoots commented Aug 8, 2023

michaellzc Aug 8, 2023

Choose a reason for hiding this comment

michaellzc Aug 8, 2023

Choose a reason for hiding this comment

michaellzc commented Aug 8, 2023

michaellzc commented Aug 8, 2023 • edited Loading

add support for `time.Time` #25

add support for `time.Time` #25

michaellzc commented Aug 8, 2023 •

edited

Loading

michaellzc commented Aug 8, 2023 •

edited

Loading

michaellzc commented Aug 8, 2023 •

edited

Loading