src/reflect: implement rawType.Name()

[Sean-Der]

No description provided.

[Sean-Der]

In good news with this merged I am able to use json :)

package main

import (
    "encoding/json"
    "fmt"
)

type JSONStruct struct {
    Outer    string
    Embedded struct {
        Inner string
    }
}

func main() {
    s := JSONStruct{}
    s.Outer = "Outer"
    s.Embedded.Inner = "Inner"
    out, err := json.Marshal(s)

    fmt.Println(string(out))
    fmt.Println(err)

}

before

panic: unimplemented: (reflect.Type).Name()
error: failed to run compiled binary /tmp/tinygo2878253476/main: signal: aborted

after

{"Outer":"Outer","Embedded":{"Inner":"Inner"}}
<nil>

[Sean-Der]

I am out of time tonight. @dgryski @aykevl Any pointers on how to get the name/how structs are encoded?

[deadprogram]

@Sean-Der can you please rebase this branch against dev? It should then pass the various CI builds.

[Sean-Der]

@deadprogram done!

[niaow]

Can you please add a test of this somewhere?

[dgryski]

We should document that we don't yet support named types, and so this fix is only partial support.

[aykevl]

I'm not sure I agree with this change. I've considered it before but rejected the idea because while it gets the encoding/json package to mostly work, it is somewhat incorrect.

[freegroup]

Any idea when this pullrequest is accepted? Actually quite a lot depends on it e.g. the parsing of x.509 certificate. We need this to write WASM filters for Istio. Without the crypto package, which needs this fix, TinyGo is almost worthless for WASM.

[doniacld]

We would rather panic here, to make it explicit we do not support this type?

Suggested change

	return ""
	panic("unimplemented: (reflect.Type).Name()")

[aykevl]

Specifically, what I'm worried about with this PR is that it is not correct for named types. For unnamed types it seems to be correct (it just returns the underlying type name like int or float32) but for named types it returns the underlying type name, not the named type as it should. This could lead to misbehaving code. Panicking would in my opinion be more appropriate than returning an incorrect value as it makes it very obvious where the problem is coming from.

A partial solution might be to detect whether the type is named and panic if it is, returning the underlying type name if it is not named. But I'm afraid this won't solve the problem for serialization libraries.

[Sean-Der]

@aykevl I am happy to add tests/modify how we do this to get it merged :) JSON I think matters a lot for JSON users (probably not so much for the small devices)

I ended up not using the stdlib json because of how big it made the binary size. I switched to github.com/moznion/go-json-ice

[jcchavezs]

I think support for unnamed types is better than no support at all which is in line with @aykevl. I am up to drive the PR towards such direction, tests included.

[jcchavezs]

I'd rather do this

Suggested change

	return ""
	panic("unimplemented for named types: (reflect.Type).Name()")

[doniacld]

FYI, we had a discussion in this thread on slack with @aykevl. There is one PR refactoring the reflect package that is in the waiting room :) So it will change after the PR is merged... I am unsure about the delays concerning the merge of the PR.

[codefromthecrypt]

Is it possible to proceed with this, even if a longer term, larger PR may happen in the future? As probably some notice, there's an increase of people trying and failing to use TinyGo with wasm, sometimes leaving the Go language entirely as a result. It would seem like "cutting of one's nose to spite their face" to not proceed tactically even if a more strategic change could happen one day.

I suspect to move this forward, we need someone with commit access to wave their hand and say "I'll help" and give whatever feedback is needed to the authors, as it seems the authors are ready.

How about it?

[aykevl]

Is it possible to proceed with this, even if a longer term, larger PR may happen in the future?

I still don't agree with merging incorrect code. This PR, while well intentioned, will result in broken code.

A relatively simple way forward is by returning "" for non-named types and panicking for named types. You can determine the difference between the two using the documentation at the top of src/reflect/types.go. Feel free to ask if something is unclear.

[codefromthecrypt]

@aykevl maybe close out this then? open PRs are distracting especially in reflect which is spread across so many things. I think that people knowing what won't happen is at least direction giving.

[dgryski]

Superseded by #3498