Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: inconsistent behaviors in judging whether or not two types are identical #24721

Closed
dotaheor opened this issue Apr 6, 2018 · 27 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dotaheor
Copy link

dotaheor commented Apr 6, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

package m3

import "time"

type t = time.Time
type T = time.Time

type A = struct{t}
type B = struct{T}
import "m3"
import "time"

type t = time.Time
type T = time.Time

type A = struct{t}
type B = struct{T}

func main() {
   var a, b interface{} = m3.A{}, m3.B{}
   var x, y interface{} = A{}, B{}
   println(a == x) // true
   println(b == y) // true
   
   _ = A(m3.A{}) // cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
   _ = B(m3.B{}) // ok
   
   println(a == b) // true
   println(x == y) // true
   _ = B(A{})       // cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
   _ = m3.A(m3.B{}) // cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
}

What did you expect to see?

No errors, [edit: or all println calls, except the second, print false].

What did you see instead?

./main.go:18:9: cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
./main.go:23:9: cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
./main.go:24:12: cannot convert struct { time.Time } literal (type struct { time.Time }) to type struct { time.Time }
@dotaheor
Copy link
Author

dotaheor commented Apr 6, 2018

btw, should struct{t} and struct{T} be viewed as the same type?

@dotaheor
Copy link
Author

dotaheor commented Apr 6, 2018

The following program print the same output for the two types:

package main

import "fmt"
import "time"
import "reflect"

type t = time.Time
type T = time.Time

type A = struct{t}
type B = struct{T}

func main() {
	t := reflect.TypeOf(A{}) // the Singer type
	fmt.Println(t, "has", t.NumField(), "fields:")
	for i := 0; i < t.NumField(); i++ {
		fmt.Print("   filed#", i, ": ", t.Field(i).Name, "\n")
	}
	
	/*
	struct { time.Time } has 1 fields:
	filed#0: t
	*/
	
	t = reflect.TypeOf(B{}) // the Singer type
	fmt.Println(t, "has", t.NumField(), "fields:")
	for i := 0; i < t.NumField(); i++ {
		fmt.Print("   filed#", i, ": ", t.Field(i).Name, "\n")
	}
	
	/*
	struct { time.Time } has 1 fields:
	filed#0: t
	*/
}

I feel the outputs are wrong, at least the outputs should be filed#0: Time.

But I change the type A and B to non-alias types, then the outputs will be different
(I think the outputs are correct):

main.A has 1 fields:
   filed#0: t
main.B has 1 fields:
   filed#0: T

@bcmills
Copy link
Contributor

bcmills commented Apr 6, 2018

A simpler repro does not require separate packages: https://play.golang.org/p/_ZimDvvVU0u

The compiler is correct to emit an error, because the fields of the two struct types have different names (t and T, respectively).
The fact that the error does not include the correct field names is a bug.

@bcmills bcmills changed the title cmd/compile: false positive error cmd/compile: misleading error message for struct assignment with different embedded field names of the same type Apr 6, 2018
@bcmills bcmills added this to the Go1.11 milestone Apr 6, 2018
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 6, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 6, 2018

The fix is presumably to use the name of the field in the error message instead of the name of the underlying type.

@bcmills
Copy link
Contributor

bcmills commented Apr 6, 2018

(CC @griesemer, maybe?)

@dotaheor
Copy link
Author

dotaheor commented Apr 7, 2018

If the error messages are correct, the problem is bigger:

package main

import (
	"fmt"
	"time"
)

type t = time.Time
type T = time.Time

type A = struct{ t }
type B = struct{ T }

func main() {
	var a A
	var b B
	fmt.Println(interface{}(b) == interface{}(a)) // true
	//fmt.Println(b == a)
}

@bcmills
Copy link
Contributor

bcmills commented Apr 7, 2018

Huh, that's a neat bug you've found!
(https://play.golang.org/p/LSwVZ_aoNmc)

@jimmyfrasche
Copy link
Member

reduced further https://play.golang.org/p/m6Wv1Hk1U8h

@dotaheor dotaheor changed the title cmd/compile: misleading error message for struct assignment with different embedded field names of the same type cmd/compile: inconsistent behaviors in judging whether or not two types are identical Apr 8, 2018
@griesemer
Copy link
Contributor

griesemer commented Apr 9, 2018

  1. Two struct types with differently named (corresponding) embedded fields are different types, even if the embedded field types are identical. So the assignments a = b are expected to be reported as invalid at compile time.

  2. Because of 1), runtime interface comparison of a and b values should return false, but the issue is that (basically) the same printing code that's used in the error message (and which prints both types as struct{ int }) is used to identify those types' symbols, so the type information is incorrect.

@griesemer
Copy link
Contributor

Too late for 1.12.

@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 5, 2018
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 5, 2018
@griesemer griesemer modified the milestones: Go1.13, Go1.14 May 6, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@griesemer griesemer added this to the Go1.15 milestone Dec 17, 2019
@dotaheor
Copy link
Author

@bcmills they are not totally the same. The unequalness in this issue is because the field names are different.

@dmitshur
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.15. That time is now, so friendly ping. If it no longer needs to be done early in cycle, that label can be removed.

@dotaheor
Copy link
Author

One more difference between gc and gccgo:

package main

import "time"

type T = struct {
	time.Time
}

func main() {
	println(T.Add)
}

It compiles ok with gc, but not gccgo.

@dotaheor
Copy link
Author

dotaheor commented Mar 28, 2020

As the line fmt.Printf("%T\n", struct {time.Time}.Add) also fails to compile with gccgo. I created a new issue for gccgo.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Go1.16 May 19, 2020
@dmitshur
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@aclements aclements modified the milestones: Go1.16, Go1.17 Dec 1, 2020
@griesemer
Copy link
Contributor

This didn't make it for Go 1.16. Should try to address for Go 1.17, certainly for 1.18. As already explained we need to fix the printing of structs, both for error messages (to make clear what's wrong), and for generating type descriptors.

Probably not too hard to do but the respective code is in fmt.go which is mess. Getting it right is subtle. Not worth the risk for 1.16. Eventually we will need to get rid of this code.

@griesemer
Copy link
Contributor

For reference: go/types and types2 report for the last line of https://play.golang.org/p/m6Wv1Hk1U8h:

main.go:14:14: invalid operation: cannot compare a == b (mismatched types struct{int} and struct{T /* = int */})

@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.17.
That time is now, so a friendly reminder to look at it again.

@griesemer
Copy link
Contributor

Again, given other priorities, this didn't make it for 1.17. Pushing to 1.18.

@griesemer griesemer modified the milestones: Go1.17, Go1.18 Apr 20, 2021
@cagedmantis
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.18.
That time is now, so this is a friendly ping so the issue is looked at again.

@ianlancetaylor
Copy link
Contributor

@griesemer I assume that we should push this to 1.19?

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Nov 10, 2021
@griesemer
Copy link
Contributor

Moved to 1.19.

@dotaheor
Copy link
Author

It looks this bug has been fixed in https://golang.org/cl/378434, for #50190.

@griesemer
Copy link
Contributor

@mdempsky Is there anything left to do here?

@mdempsky
Copy link
Member

mdempsky commented Jan 25, 2022

@dotaheor Thanks for pointing that out.

@griesemer I don't think so. I looked over the failure cases, and I don't think they add anything significant beyond ones we already added for #50190. Closing.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests