amino.WithTypes accepts types being given the same name

[grepsuzette]

amino.WithTypes admits types with the same name

This potential issue was uncovered in #2325.

Description

Quite simply, the following code does not panic:

package amino_test

import (
	"reflect"
	"testing"
	"github.com/stretchr/testify/assert"
	amino "github.com/gnolang/gno/tm2/pkg/amino"
	"github.com/gnolang/gno/tm2/pkg/amino/tests"
)

func TestDupNamesMustPanic(t *testing.T) {
	assert.Panics(t, func() {
		amino.NewPackage(
			reflect.TypeOf(tests.EmptyStruct{}).PkgPath(),
			"amino_test",
			amino.GetCallersDirname(),
		).WithTypes(
	 		tests.EmptyStruct{}, "A",
	 		tests.PrimitivesStruct{}, "B",
	 		tests.ShortArraysStruct{}, "A", // Same name
		)
	}
}

Expected behaviour

Since codec_test.go has the following line a panic should be expected here:

// XXX Test registering duplicate names or concrete types not in a package.

Proposed solution

Existing code for WithTypes prepares a pkg.Types which can just be iterated before returning.

gno/tm2/pkg/amino/pkg/pkg.go

Lines 126 to 187 in 5541e35

	func (pkg *Package) WithTypes(objs ...interface{}) *Package {
	var lastType *Type = nil
	for _, obj := range objs {
	// Initialize variables
	objType := reflect.TypeOf(obj)
	name := ""
	pointerPreferred := false
	// Two special cases.
	// One: a string which follows a type declaration is a name.
	if objType == reflect.TypeOf("string") {
	if lastType == nil {
	panic("Type names (specified via a string argument to WithTypes()) must come *after* the prototype object")
	} else {
	lastType.Name = obj.(string)
	lastType = nil // no more updating is possible.
	continue
	}
	}
	// Two: pkg.Type{} can be specified directly
	if objType.Kind() == reflect.Ptr && objType.Elem() == reflect.TypeOf(Type{}) {
	panic("Use pkg.Type{}, not *pkg.Type{}")
	}
	if objType == reflect.TypeOf(Type{}) {
	objType = obj.(Type).Type
	name = obj.(Type).Name
	if name != capitalize(name) {
	panic(fmt.Sprintf("Type name must be capitalized, but got %v", name))
	}
	pointerPreferred = obj.(Type).PointerPreferred
	}
	// End of special cases.
	// Init deref and ptr types.
	objDerefType := objType
	if objDerefType.Kind() == reflect.Ptr {
	objDerefType = objType.Elem()
	if objDerefType.Kind() == reflect.Ptr {
	panic("unexpected nested pointers")
	}
	pointerPreferred = true
	}
	if objDerefType.PkgPath() != pkg.GoPkgPath {
	panic(fmt.Sprintf("unexpected package for %v, expected %v got %v for obj %v obj type %v", objDerefType, pkg.GoPkgPath, objDerefType.PkgPath(), obj, objType))
	}
	// Check that deref type don't already exist.
	_, ok := pkg.GetType(objDerefType)
	if ok {
	panic(fmt.Errorf("type %v already registered with package", objDerefType))
	}
	if name == "" {
	name = objDerefType.Name()
	}
	lastType = &Type{ // memoize for future modification.
	Type: objDerefType,
	Name: name, // may get overridden later.
	PointerPreferred: pointerPreferred,
	}
	pkg.Types = append(pkg.Types, lastType)
	}
	return pkg
	}

The following actually fixes the test in #2325:

        (...)
		pkg.Types = append(pkg.Types, lastType)
	}

    // APPENDED HERE -------------------------------------------------------
	// Forbid duplicate names
	for i, t := range pkg.Types {
		if t.Name == "" {
			continue
		}
		for j := i + 1; j < len(pkg.Types); j++ {
			if pkg.Types[j].Name == t.Name {
				panic(fmt.Errorf("type name %s is already registered", t.Name))
			}
		}
	} // ------------------------------------------------------------------------^^^
	return pkg
}