compiler: correctly generate code for local named types by aykevl · Pull Request #3565 · tinygo-org/tinygo

aykevl · 2023-03-16T14:13:21Z

It is possible to create function-local named types:

func foo() any {
    type named int
    return named(0)
}

This patch makes sure they don't alias with named types declared at the package scope.

Bug originally found by Damian Gryski while working on reflect support.

dgryski · 2023-03-16T15:02:31Z

Seems to work. I'll give it a better review in a bit.

Even passes this test:

package main

import "reflect"

func main() {

        for i := 0; i < 4; i++ {
                a := named(i)
                r := reflect.TypeOf(a)
                println(i, r.Kind().String())
        }
}

func named(n int) any {

        if n == 0 {
                type named bool
                return named(true)
        }

        if n == 1 {
                type named byte
                return named(2)
        }

        if n == 3 {
                type named uint16
                return named(3)
        }

        type named int
        return named(4)
}

dgryski · 2023-03-16T15:32:01Z

Oddly the failing XML test doesn't pass with this patch. So looks like there's something else we need to hunt down. I'll try to figure out if it's this PR or something else is still broken.

dgryski · 2023-03-16T17:07:45Z

Here's the extracted xml test that is still getting the wrong type info for the structs:

// Copyright 2011 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
        "encoding/xml"
        "log"
)

func main() {

        testCases := []struct {
                f    func() ([]byte, error)
                want string
                ok   bool
        }{
                {encodeXMLNS1, `<Test xmlns="http://example.com/ns"><Body>hello world</Body></Test>`, true},
                {encodeXMLNS2, `<Test><body xmlns="http://example.com/ns">hello world</body></Test>`, true},
                {encodeXMLNS3, `<Test xmlns="http://example.com/ns"><Body>hello world</Body></Test>`, true},
                {encodeXMLNS4, `<Test xmlns="http://example.com/ns"><Body>hello world</Body></Test>`, false},
        }

        for i, tc := range testCases {
                println(i, tc.want)
                if b, err := tc.f(); err == nil {
                        if got, want := string(b), tc.want; got != want {
                                log.Fatalf("%d: got %s, want %s \n", i, got, want)
                        }
                } else {
                        log.Fatalf("%d: marshal failed with %s", i, err)
                }
        }
}

func encodeXMLNS1() ([]byte, error) {

        type T struct {
                XMLName xml.Name `xml:"Test"`
                Ns      string   `xml:"xmlns,attr"`
                Body    string
        }

        s := &T{Ns: "http://example.com/ns", Body: "hello world"}
        return xml.Marshal(s)
}

func encodeXMLNS2() ([]byte, error) {

        type Test struct {
                Body string `xml:"http://example.com/ns body"`
        }

        s := &Test{Body: "hello world"}
        return xml.Marshal(s)
}

func encodeXMLNS3() ([]byte, error) {

        type Test struct {
                XMLName xml.Name `xml:"http://example.com/ns Test"`
                Body    string
        }

        //s := &Test{XMLName: Name{"http://example.com/ns",""}, Body: "hello world"} is unusable as the "-" is missing
        // as documentation states
        s := &Test{Body: "hello world"}
        return xml.Marshal(s)
}

func encodeXMLNS4() ([]byte, error) {

        type Test struct {
                Ns   string `xml:"xmlns,attr"`
                Body string
        }

        s := &Test{Ns: "http://example.com/ns", Body: "hello world"}
        return xml.Marshal(s)
}

Doubt this runs, but I can maybe try to extract the pieces so it's just reflect calls grabbing type info.

aykevl · 2023-03-16T17:22:03Z

but I can maybe try to extract the pieces so it's just reflect calls grabbing type info.

That would certainly be very useful for debugging.

dgryski · 2023-03-16T17:48:55Z

Better test case:

package main

import (
	"reflect"
)

type xmlt struct{}

var xml xmlt

type xmlName string

func (xmlt) Marshal(a any) (string, error) {
	t := reflect.TypeOf(a)
	return dumpType(t), nil
}

func dumpType(t reflect.Type) string {
	if t.Kind() == reflect.Struct {
		s := "struct {"
		numField := t.NumField()
		for i := 0; i < numField; i++ {
			f := t.Field(i)
			s += " " + f.Name + " " + dumpType(f.Type)
			// every field except the last needs a semicolon
			if i < numField-1 {
				s += ";"
			}
		}
		s += " }"
		return s
	}
	if t.Kind() == reflect.Pointer {
		return "*" + dumpType(t.Elem())
	}
	return t.Kind().String()
}

func main() {

	testCases := []struct {
		f func() (string, error)
	}{
		{encodeXMLNS1},
		{encodeXMLNS2},
		{encodeXMLNS3},
		{encodeXMLNS4},
	}

	for i, tc := range testCases {
		println(i)
		if b, err := tc.f(); err == nil {
			println(string(b))
		}
	}
}

func encodeXMLNS1() (string, error) {

	type T struct {
		XMLName xmlName `xml:"Test"`
		Ns      string  `xml:"xmlns,attr"`
		Body    string
	}

	s := &T{Ns: "http://example.com/ns", Body: "hello world"}
	return xml.Marshal(s)
}

func encodeXMLNS2() (string, error) {

	type Test struct {
		Body string `xml:"http://example.com/ns body"`
	}

	s := &Test{Body: "hello world"}
	return xml.Marshal(s)
}

func encodeXMLNS3() (string, error) {

	type Test struct {
		XMLName xmlName `xml:"http://example.com/ns Test"`
		Body    string
	}

	//s := &Test{XMLName: Name{"http://example.com/ns",""}, Body: "hello world"} is unusable as the "-" is missing
	// as documentation states
	s := &Test{Body: "hello world"}
	return xml.Marshal(s)
}

func encodeXMLNS4() (string, error) {

	type Test struct {
		Ns   string `xml:"xmlns,attr"`
		Body string
	}

	s := &Test{Ns: "http://example.com/ns", Body: "hello world"}
	return xml.Marshal(s)
}

Output:

~/go/src/github.com/dgryski/bug/refxml $ tinygo run  main.go
0
*struct { XMLName string; Ns string; Body string }
1
*struct { Body string }
2
*struct { Body string }
3
*struct { Body string }
~/go/src/github.com/dgryski/bug/refxml $ go run  main.go
0
*struct { XMLName string; Ns string; Body string }
1
*struct { Body string }
2
*struct { XMLName string; Body string }
3
*struct { Ns string; Body string }

aykevl · 2023-03-16T20:28:49Z

Much smaller reproducer:

package main

import "reflect"

func main() {
        itf := test2()
        t := reflect.TypeOf(itf).Elem()
        println("numField:", t.NumField())
}

func test1() interface{} {
        type Test struct {
                B int
        }
        return &Test{}
}

func test2() interface{} {
        type Test struct {
                A int
                B int
        }
        return &Test{}
}

Prints 2 in go run, but 1 in tinygo run.

aykevl · 2023-03-16T20:34:01Z

This makes it very clear that the wrong type is stored in the interface:

package main

import "reflect"

func main() {
        itf := test2()
        kind := reflect.TypeOf(itf).Elem().Kind()
        println("kind:", kind.String())
}

func test1() interface{} {
        type Test int
        return new(Test)
}

func test2() interface{} {
        type Test byte
        return new(Test)
}

~/src/tinygo/tinygo$ go run tmp/issue3566.go
kind: uint8

~/src/tinygo/tinygo$ tinygo run tmp/issue3566.go
kind: int

aykevl · 2023-03-16T20:37:11Z

Pretty sure I know what's wrong now, I'll update this PR with a fix.
In summary, while the PR fixes plain named types, it is in a very similar way buggy for pointers to local named types.

aykevl · 2023-03-16T22:09:06Z

Should be fixed now (with an additional test case).

dgryski

LGTM.

deadprogram · 2023-03-18T11:47:49Z

This PR does not appear to pass CircleCI tests of backward compatibility:
https://app.circleci.com/pipelines/github/tinygo-org/tinygo/9219/workflows/0510a406-46e8-4cb3-b234-868850b82027/jobs/40141?invite=true#step-118-8

deadprogram · 2023-03-20T15:27:22Z

 	switch t := t.(type) {
 	case *types.Named:
-		return "named:" + t.String()
+		if t.Obj().Parent() != t.Obj().Pkg().Scope() {


It seems like in Go1.18/LLVM 14 that t.Obj().Pkg() is sometimes nil here, causing

panic: runtime error: invalid memory address or nil pointer dereference

See https://app.circleci.com/pipelines/github/tinygo-org/tinygo/9219/workflows/c192f68a-0c3e-4eb8-a10f-0c1fdb9aa64e/jobs/40162?invite=true#step-118-8

Probably not LLVM, but it could be Go 1.18. I'd have to take a closer look.

aykevl · 2023-03-21T17:07:20Z

Indeed, this was an issue with Go 1.18. It should be fixed now.

deadprogram · 2023-03-21T17:44:46Z

Looks like a number of other CI fails now, however 😺

dgryski · 2023-03-21T18:42:07Z

Need to rerun go test -update.

deadprogram · 2023-03-21T19:01:26Z

@aykevl can you do that please?

It is possible to create function-local named types: func foo() any { type named int return named(0) } This patch makes sure they don't alias with named types declared at the package scope. Bug originally found by Damian Gryski while working on reflect support.

aykevl · 2023-03-21T19:26:55Z

Need to rerun go test -update.

No, this was actually a regression after I fixed Go 1.18 support. I've fixed the fix, so that go test ./compiler now works.

dgryski

LGTM

dgryski · 2023-03-21T20:30:51Z

Only CI fail was the circuitplay-express.

deadprogram · 2023-03-21T21:13:10Z

Only CI fail was the circuitplay-express.

Restarted it and passed.

Now merging, thanks @aykevl and @dgryski

aykevl requested a review from dgryski March 16, 2023 14:13

aykevl marked this pull request as draft March 16, 2023 20:36

aykevl force-pushed the local-named-type branch from 42d8adc to 781bf4a Compare March 16, 2023 22:06

aykevl marked this pull request as ready for review March 16, 2023 22:07

dgryski approved these changes Mar 17, 2023

View reviewed changes

deadprogram reviewed Mar 20, 2023

View reviewed changes

aykevl force-pushed the local-named-type branch from 781bf4a to 740e9c5 Compare March 21, 2023 17:06

aykevl force-pushed the local-named-type branch from 740e9c5 to b113396 Compare March 21, 2023 19:25

dgryski approved these changes Mar 21, 2023

View reviewed changes

deadprogram merged commit 523c6c0 into dev Mar 21, 2023

deadprogram deleted the local-named-type branch March 21, 2023 21:22

aykevl mentioned this pull request Apr 24, 2026

compiler: disambiguate function-local named types #5336

Open

Uh oh!

Conversation

aykevl commented Mar 16, 2023

Uh oh!

dgryski commented Mar 16, 2023

Uh oh!

dgryski commented Mar 16, 2023

Uh oh!

dgryski commented Mar 16, 2023

Uh oh!

aykevl commented Mar 16, 2023

Uh oh!

dgryski commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl commented Mar 16, 2023

Uh oh!

aykevl commented Mar 16, 2023

Uh oh!

aykevl commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl commented Mar 16, 2023

Uh oh!

dgryski left a comment

Choose a reason for hiding this comment

Uh oh!

deadprogram commented Mar 18, 2023

Uh oh!

deadprogram Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

aykevl Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

aykevl commented Mar 21, 2023

Uh oh!

deadprogram commented Mar 21, 2023

Uh oh!

dgryski commented Mar 21, 2023

Uh oh!

deadprogram commented Mar 21, 2023

Uh oh!

aykevl commented Mar 21, 2023

Uh oh!

dgryski left a comment

Choose a reason for hiding this comment

Uh oh!

dgryski commented Mar 21, 2023

Uh oh!

deadprogram commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dgryski commented Mar 16, 2023 •

edited

Loading

aykevl commented Mar 16, 2023 •

edited

Loading