Add stubs for more unimplemented reflect methods #2574

kyleconroy · 2022-01-24T00:33:50Z

I'm current working on solving #447 by getting https://github.com/protocolbuffers/protobuf-go to compile with TinyGo. In combination with https://github.com/planetscale/vtprotobuf, I think there's a chance I get the code generated by protoc to compile and run without any changes.

This is my first attempt to "implement" some of the missing methods in the reflect package. This is my first time contributing to TinyGo, so I have no idea if I'm doing things correctly. Please let me know!

I've included some code copied directly from the Go source code. Is this allowed? How do I mark that code?

kyleconroy · 2022-01-24T00:34:42Z

src/reflect/type.go

+// Method represents a single method.
+type Method struct {
+	// Name is the method name.
+	Name string
+
+	// PkgPath is the package path that qualifies a lower case (unexported)
+	// method name. It is empty for upper case (exported) method names.
+	// The combination of PkgPath and Name uniquely identifies a method
+	// in a method set.
+	// See https://golang.org/ref/spec#Uniqueness_of_identifiers
+	PkgPath string
+
+	Type  Type  // method type
+	Func  Value // func with receiver as first argument
+	Index int   // index for Type.Method
+}


Copied from Go source

kyleconroy · 2022-01-24T00:34:54Z

src/reflect/value.go

+	if e.Kind == 0 {
+		return "reflect: call of " + e.Method + " on zero Value"
+	}
+	return "reflect: call of " + e.Method + " on " + e.Kind.String() + " Value"


Copied from Go source

kyleconroy · 2022-01-24T00:35:21Z

src/reflect/value.go

@@ -187,7 +187,7 @@ func (v Value) Bool() bool {
 			return uintptr(v.value) != 0
 		}
 	default:
-		panic(&ValueError{"Bool"})
+			panic(&ValueError{Method: "Bool"})


Not sure how all the spacing got messed up here. Happy to fix this up if this is something you'd like to merge.

kyleconroy · 2022-01-24T00:36:01Z

src/reflect/value.go

@@ -788,10 +788,14 @@ type stringHeader struct {

 type ValueError struct {
 	Method string
+	Kind   Kind


Updated to match the ValueError from Go

kyleconroy · 2022-01-24T00:38:02Z

src/reflect/type.go

+	// panic("unimplemented: (reflect.Type).PkgPath()")
+	return "<unimplemented>"


I need this to return a string for protocol buffers to work, but I'm happy to put the panic back in so the code at least compiles.

dkegel-fastly · 2022-01-24T01:46:29Z

Copying source from upstream is fine, just make sure the file has copyright headers.

I haven't looked at reflect, so I'm not the person to comment, but:
Please include a test for the particular thing you're fixing. (e.g. a test that uses DeepEquals to compare two things that have the type you're improving support for).

jrauschenbusch · 2022-01-27T11:24:23Z

Do you also plan to implement reflect methods required by gob?

https://tinygo.org/docs/reference/lang-support/stdlib/#encodinggob

kyleconroy · 2022-01-31T03:34:41Z

Please include a test for the particular thing you're fixing.

Will do, not sure how unimplemented methods are currently tested.

Do you also plan to implement reflect methods required by gob?

I do not. If you look at my change, I'm not implementing any methods, only adding unimplemented stubs so that the default code generated by protoc compiles. That code will still fail at runtime.

dkegel-fastly · 2022-01-31T16:42:15Z

Oh, I didn't realize you were just adding stubs. Changed title to reflect what you said.

You can add smoke tests that only verify compilation, and I've done that before, but I'm not sure it's worth it in this case.

kyleconroy · 2022-02-01T17:36:30Z

@dkegel-fastly Would it be possible to approve the 3 workflows for this PR? I'd like to know if my change passes all the tests.

dkegel-fastly · 2022-02-01T23:48:27Z

that is kind of a surprising failure...

kyleconroy · 2022-02-02T02:38:51Z

@dkegel-fastly I've rebased on the latest dev, can you approve the tests again?

kyleconroy · 2022-02-06T06:22:18Z

Great, looks like all the tests are passing. Anything else I need to do to get this merged in?

dkegel-fastly · 2022-02-06T22:33:58Z

Check with @dgryski, he may have suggestions. In slack, he wrote "I wonder if stubbing more of reflect would allow text/template to build and be used for simple things (that don't use the bits we faked)".

I just checked tinygo building example_test.go from upstream's text/template against dev and against your branch. Resulting errors (ones you got rid of are marked with a dash):

  SendDir not declared by package reflect
  etyp.FieldByName undefined (type reflect.Type has no field or method FieldByName)
  item.Slice3 undefined (type reflect.Value has no field or method Slice3)
- ptr.MethodByName undefined (type reflect.Value has no field or method MethodByName)
  receiver.Type().FieldByName undefined (type reflect.Type has no field or method FieldByName)
- typ.In undefined (type reflect.Type has no field or method In)
  typ.IsVariadic undefined (type reflect.Type has no field or method IsVariadic)
  typ.NumIn undefined (type reflect.Type has no field or method NumIn)
  typ.NumOut undefined (type reflect.Type has no field or method NumOut)
- typ.Out undefined (type reflect.Type has no field or method Out)
  v.Type().NumOut undefined (type reflect.Type has no field or method NumOut)
  val.Recv undefined (type reflect.Value has no field or method Recv)
  val.Type().ChanDir undefined (type reflect.Type has no field or method ChanDir)

Whew. Well, getting text/template to build its example_test.go means stubbing another ten methods. That's a lot.

Is there some open source package that your patch rescues? That might help.

kyleconroy · 2022-02-08T05:08:15Z

Is there some open source package that your patch rescues? That might help.

Yes! This patch, combined with https://github.com/planetscale/vtprotobuf, allows tinygo programs to use Protocol Buffers*. I'm also happy to add stubs for the rest of those methods if it will mean this gets merged.

There is still one small change you have to make to the generated code, but I'd commit to fixing that once this is merged.

dkegel-fastly · 2022-02-08T15:46:41Z

Closing in favor of #2624.

kyleconroy commented Jan 24, 2022

View reviewed changes

dkegel-fastly changed the title ~~Add more unimplemented reflect methods~~ Add stubs for more unimplemented reflect methods Jan 31, 2022

kyleconroy force-pushed the kyle/compiler-protobuf branch from 7063e0e to ff89b75 Compare February 2, 2022 02:38

kyleconroy added 5 commits February 7, 2022 21:48

Making progress

5d4d974

It.... works?

a9db848

Return "<unimplemented>" from PkgPath

dcb4b7f

Make PkgPath panic

48bce64

gofmt

5619df0

kyleconroy force-pushed the kyle/compiler-protobuf branch from ff89b75 to 5619df0 Compare February 8, 2022 05:48

kyleconroy mentioned this pull request Feb 8, 2022

feat: Add stubs for more of the reflect package #2624

Merged

dkegel-fastly closed this Feb 8, 2022

tachyonicbytes mentioned this pull request Nov 8, 2022

Setuid not declared by package syscall #3274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stubs for more unimplemented reflect methods #2574

Add stubs for more unimplemented reflect methods #2574

kyleconroy commented Jan 24, 2022 •

edited

Loading

kyleconroy Jan 24, 2022

kyleconroy Jan 24, 2022

kyleconroy Jan 24, 2022

kyleconroy Jan 24, 2022

kyleconroy Jan 24, 2022

dkegel-fastly commented Jan 24, 2022

jrauschenbusch commented Jan 27, 2022

kyleconroy commented Jan 31, 2022

dkegel-fastly commented Jan 31, 2022

kyleconroy commented Feb 1, 2022

dkegel-fastly commented Feb 1, 2022

kyleconroy commented Feb 2, 2022

kyleconroy commented Feb 6, 2022

dkegel-fastly commented Feb 6, 2022 •

edited

Loading

kyleconroy commented Feb 8, 2022

dkegel-fastly commented Feb 8, 2022

		// panic("unimplemented: (reflect.Type).PkgPath()")
		return "<unimplemented>"

Add stubs for more unimplemented reflect methods #2574

Add stubs for more unimplemented reflect methods #2574

Conversation

kyleconroy commented Jan 24, 2022 • edited Loading

kyleconroy Jan 24, 2022

Choose a reason for hiding this comment

kyleconroy Jan 24, 2022

Choose a reason for hiding this comment

kyleconroy Jan 24, 2022

Choose a reason for hiding this comment

kyleconroy Jan 24, 2022

Choose a reason for hiding this comment

kyleconroy Jan 24, 2022

Choose a reason for hiding this comment

dkegel-fastly commented Jan 24, 2022

jrauschenbusch commented Jan 27, 2022

kyleconroy commented Jan 31, 2022

dkegel-fastly commented Jan 31, 2022

kyleconroy commented Feb 1, 2022

dkegel-fastly commented Feb 1, 2022

kyleconroy commented Feb 2, 2022

kyleconroy commented Feb 6, 2022

dkegel-fastly commented Feb 6, 2022 • edited Loading

kyleconroy commented Feb 8, 2022

dkegel-fastly commented Feb 8, 2022

kyleconroy commented Jan 24, 2022 •

edited

Loading

dkegel-fastly commented Feb 6, 2022 •

edited

Loading