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: clarify prose/examples regarding struct identity when embedded types match equally named non-embedded fields #69472

Closed
leabit opened this issue Sep 14, 2024 · 32 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation Issues describing a change to documentation.
Milestone

Comments

@leabit
Copy link

leabit commented Sep 14, 2024

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/leabit/.cache/go-build'
GOENV='/home/leabit/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/leabit/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/leabit/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/leabit/go/pkg/mod/golang.org/[email protected]'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/leabit/go/pkg/mod/golang.org/[email protected]/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/leabit/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/leabit/goprojects/exeexe/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1849554432=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Golang specification for type identity states the following for the struct types:

Two struct types are identical if they have the same sequence of fields, and if corresponding fields have the same names, and identical types, and identical tags. Non-exported field names from different packages are always different.

Thus, it has 4 requirements:

  1. same sequence of fields
  2. same corresponding names
  3. identical types of corresponding fields
  4. identical tags of corresponding fields

However, there are cases related to embedding aliases of unnamed types where, despite meeting all four previous requirements, the compiler still fails to compile.

I will explain the problem through few examples. The following notes holds for all examples:

  • three packages are used: pac1, pac2 and main
  • declared alias types in pac and pac2 are unnamed types struct{} (thus, they are identical)
  • struct tags aren't used, so they are ignored

(1) This example successfully compiles (as expected). M1 and M2 have the same sequence of fields. Names, even thought not explicitly defined, are indirectly given by embedding and thus they are same (MyType1). Types of the fields (field MyType1) are also same (struct{}).

pac1:

package pac1

type MyType1 = struct{}

pac2:

package pac2

type MyType1 = struct{}

main:

package main

import (
	"exeexe/pac1"
	"exeexe/pac2"
)

type M1 struct {
	pac1.MyType1
}

type M2 struct {
	pac2.MyType1
}

func main() {
	_ = M2(M1{})
}

(2) In this example MyType1 = struct{} is replaced with MyType2 = struct{} in pac2. Also, pac2.MyType2 is now embedded in M2 (instead of pac2.MyType1) in main package. As expected, it doesn't compile. Even thought the types of the fields are the same, the names are not (MyType1 != MyType2), thus requirement 2 doesn't hold.

pac1:

package pac1

type MyType1 = struct{}

pac2:

package pac2

type MyType2 = struct{}

main:

package main

import (
	"exeexe/pac1"
	"exeexe/pac2"
)

type M1 struct {
	pac1.MyType1
}

type M2 struct {
	pac2.MyType2
}

func main() {
	_ = M2(M1{})
}

(3) This example shows the actual problem. The only difference compared to the previous example is that embedded field (pac2.MyType2) is now replaced by non-embedded (ordinary) field with the name MyType1 (type is the same). Since specification for type identity doesn't make a distinct between embedded and ordinary field in the struct this should be valid and compile (all requirements are met).

pac1:

package pac1

type MyType1 = struct{}

pac2:

package pac2

type MyType2 = struct{}

main:

package main

import (
	"exeexe/pac1"
	"exeexe/pac2"
)

type M1 struct {
	pac1.MyType1
}

type M2 struct {
	MyType1 pac2.MyType2
}

func main() {
	_ = M2(M1{})
}

(4) This example just shows that even when MyType1 pac2.MyType2 is replaced with MyType1 pac1.MyType1 in M2 (so just the name is added), it still doesn't compile.

pac1:

package pac1

type MyType1 = struct{}

pac2:

package pac2

type MyType2 = struct{}

main:

package main

import (
	"exeexe/pac1"
)

type M1 struct {
	pac1.MyType1
}

type M2 struct {
	MyType1 pac1.MyType1
}

func main() {
	_ = M2(M1{})
}

What did you see happen?

Inconsistency between specification and implementation

What did you expect to see?

Since I am following specification, I expect examples (3) and (4) to compile, or to modify the specification to address the difference.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

A clever finding.

BTW, it looks your code doesn't prove type identity, but conversion rules. Though I believe that your point is described well and it applies to conversions too.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

The code is used to prove type identity:

package main

type MyType1 = struct{}

type M1 = struct {
	MyType1
}

type M2 = struct {
	MyType1 MyType1
}

func main() {
	indentify := any(M1{}) == any(M2{})
	println(indentify) // false
}

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@zigo101 Right, I just used conversion as a quick way to check type identity equality. According to specification if two types are identical, values of these types can be assigned to each other. Further, if they are assignable, conversion is possible.

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@griesemer @ianlancetaylor

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

As I understand the spec, in case of (3) and (4), M1 and M2 does not have the same sequence of fields, because MyType1 of M1 is an embedded field, while MyType1 of M2 is not. These fields have the same name but are different fields.

@leabit
Copy link
Author

leabit commented Sep 14, 2024

BTW, if I use a @zigo101 way of checking type identity, then even the first example fails (it prints false). However, the conversion is successful (as already stated).

@cuonglm Specification doesn't make a distinction between embedded and ordinary fields. From spec:

Promoted fields act like ordinary fields of a struct except that they cannot be used as field names in composite literals of the struct.

Even if you are right, then the first example with indentify := any(M1{}) == any(M2{}) must print true.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

The problem shown in the first example should be implementation bug in my understanding.

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

Even if you are right, then the first example with indentify := any(M1{}) == any(M2{}) must print true.

I don't get it.

You can't do M1{} == M2{} because they are distincted types, thus any(M1{}) == any(M2{}) should print false IMHO.

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

Specification doesn't make a distinction between embedded and ordinary fields. From spec:

Don't that your quote show the difference? "act like" does not mean they are the same fields, causing M1 and M2 don't have the same sequence of fields

@zigo101
Copy link

zigo101 commented Sep 14, 2024

Aha, @cuonglm is right here. M1 and M2 and not alias declarations. So they are distinct types. No problem is shown in example 1.

[edit] When they are alias declarations, then M1 and M2 does denote the same type: https://go.dev/play/p/nR3-nts_kc7

@leabit
Copy link
Author

leabit commented Sep 14, 2024

Then how is conversion possible?

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

Then how is conversion possible?

It's the same manner with converting from a float to a int, following a set of rules, which you can read here: https://go.dev/ref/spec#Conversions

@zigo101
Copy link

zigo101 commented Sep 14, 2024

Because in example 1, M1 and M2 shares the same underlying type.

The point (inaccuracy in spec) mentioned by @leabit can still be demonstrated in this example: #69472 (comment)

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

Because in example 1, M1 and M2 shares the same underlying type.

The point (inaccuracy in spec) mentioned by @leabit can still be demonstrated in this example: #69472 (comment)

As I explained above, M1 and M2 don't have the same sequence of fields. If you laid them out literally, M1 is:

struct{main.MyType1}

while M2 is:

struct{MyType1 main.MyType1}

They don't satisfy the type identity definition in spec:

two types are identical if their underlying type literals are structurally equivalent; that is, they have the same literal structure and corresponding components have identical types

@zigo101
Copy link

zigo101 commented Sep 14, 2024

Okay, I think it is indeed some nitpicking.

A named type is always different from any other type. Otherwise, two types are identical if their underlying type literals are structurally equivalent; that is, they have the same literal structure and corresponding components have identical types. In detail:

The final in detail means re-explaining it in detail. So the above quotation will not overlap the following detailed explanations. Even they are in the intersection relation. It looks struct type type literals are not explained elsewhere. (nitpick, :D)

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@cuonglm Yes, you are right about the first example (my bad, I forgot they are named in main, not aliases like in pac1 and pac2). any(M1{}) == any(M2{}) should prints false since they are two different types (they are named). Also, they can convert to each following the next rules from the spec:

ignoring struct tags (see below), x's type and T are not type parameters but have identical underlying types.

However, I think the inconsistency still exists for the rest (examples 3 and 4). For me act like and their underlying type literals are structurally equivalent are ambiguous.

If you look at it that way, then the following should print false since struct {pac1.MyType1} is structurally different than struct {pac2.MyType1}:

pac1:

package pac1

type MyType1 = struct{}

pac2:

package pac2

type MyType1 = struct{}

main:

package main

import (
	"exeexe/pac1"
	"exeexe/pac2"
)

type M1 = struct {
	pac1.MyType1
}

type M2 = struct {
	pac2.MyType1
}

func main() {
	indentify := any(M1{}) == any(M2{})
	println(indentify)
}

@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

If you look at it that way, then the following should print false since struct {pac1.MyType1} is structurally different than struct {pac2.MyType1}

How could they be different?

These fields are both embedded, have the same name MyType1, and have the same underlying type struct{}.

The package pac1 and pac2 does not play any role in this identity checking because MyType1 is a exported field.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

@leabit In you last example, pac1.MyType1 and pac2.MyType1 denote the same type: struct{}. So M1 and M2 are also the same type. (edit: BTW, here is an example which indeed should print false: https://go.dev/play/p/FF3lzncR9gJ)

The core of the issue is whether or not field sequence is defined clearly. I re-read the struct section of spec and am inclined to believe its definition is okay now. https://go.dev/ref/spec#Struct_types

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@cuonglm Well, the specification treats the following two as the same in all segments: struct{SomeType} is same as struct{SomeType SomeType}. That's how I understood and interpreted "act like" statement for the embedded fields (I assume a lot of people would too; It looks like @zigo101 also did). Why would we consider ordinary and embedded fields different, and not embedded fields from the different packages (pac1 and pac2). That was the point of my last example (probably not the best).

Just to be clear, I fully understand what you are trying to say and you might be right (we are in the same team :D ), but something is wrong here, and it's either the ambiguity and vagueness of the specification or maybe implementation.

If you are right, it must be clearly stated in the specification and your "As I understand the spec..." says that it is not.

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@zigo101 It states:

A struct is a sequence of named elements, called fields, each of which has a name and a type. Field names may be specified explicitly (IdentifierList) or implicitly (EmbeddedField).

I don't see any difference here between embedded and ordinary fields. A struct is a sequence of named elements (field sequence), where the name can be implicit (embedded field) or explicit.

So according to it struct {MyType} and struct {MyType MyType} are the same, since the first one is considered as struct {MyType MyType}.

Specification for type identity states:

Two struct types are identical if they have the same sequence of fields...

and they definitely have (it's just whether you specified name explicitly or field got a name implicitly).

@zigo101
Copy link

zigo101 commented Sep 14, 2024

I agree it is not very clear. Maybe the wording can be improved.

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@zigo101 Yes, or implement it in a way it doesn't make a difference between embedded and ordinary field (as specification states, in my interpretation and understanding).

Anyway, something must be done, whether it involves fixing the compiler or updating the specification. At least, we can all agree that the problem exists.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

or implement it in a way it doesn't make a difference between embedded and ordinary field

Then the "embedding field" feature will totally lost its meaningfulness. :D

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@zigo101 I meant, in this case. Not generally (not like losing promotion of methods).

@zigo101
Copy link

zigo101 commented Sep 14, 2024

Promoted fields/methods are intrinsic characters of a type. If two types have different promoted fields/methods, then they must be two distinct types.

@leabit
Copy link
Author

leabit commented Sep 14, 2024

@zigo101 You are talking about the following?

package main

type MyType struct {
	Field int
}

func (MyType) Method1() {}

type M1 = struct {
	MyType
}

type M2 = struct {
	MyType MyType
}

func main() {
	indentify := any(M1{}) == any(M2{})
	println(indentify)
}

Both M1 and M2 are actually struct {MyType MyType}, but M2 doesn't have Method1 in its method set and Field as a field (it has, but can't be accessed directly). So logically they can't be considered as the same types. Right.

I assume, then the conclusion is that the specification is not good (and needs improvement) since according to it, these two types are the same.

@zigo101
Copy link

zigo101 commented Sep 14, 2024

The spec intends to view M1 and M2 in your last comment as two distinct types. The current descriptions might be not perfect but I think they are acceptable.

For this specific case in 3rd and 4th examples of your first comment, a.k.a. the case in which the embedding fields don't provide promoted fields/methods, it might be okay to treat the containing unnamed struct types as identical, but I don't think it is worth making this difference from embedding fields providing promoted fields/methods.

@cagedmantis cagedmantis changed the title spec/compiler: Inconsistency between specification and implementation related to struct type identity when struct contains embedded alias on unnamed type cmd/compile: Inconsistency between specification and implementation related to struct type identity when struct contains embedded alias on unnamed type Sep 17, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 17, 2024
@cagedmantis cagedmantis changed the title cmd/compile: Inconsistency between specification and implementation related to struct type identity when struct contains embedded alias on unnamed type cmd/compile: inconsistency between specification and implementation related to struct type identity when struct contains embedded alias on unnamed type Sep 17, 2024
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 17, 2024
@cagedmantis cagedmantis added this to the Backlog milestone Sep 17, 2024
@griesemer griesemer modified the milestones: Backlog, Go1.24 Sep 25, 2024
@griesemer griesemer changed the title cmd/compile: inconsistency between specification and implementation related to struct type identity when struct contains embedded alias on unnamed type cmd/compile: clarify prose/examples regarding struct identity when embedded types match equally named non-embedded fields Sep 30, 2024
@griesemer griesemer added Documentation Issues describing a change to documentation. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 30, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 30, 2024
@griesemer
Copy link
Contributor

It's not clear to me what this issue has to do with alias or unnamed types: if I understand correctly, the issue is about whether two struct fields are identical if one of them is embedded and the other one is not but they appear in the same "spot" in the struct, have the same name, same type, and same tag.

It is the intent of the spec that two corresponding fields are not identical if one of them is embedded and the other one is not. I agree that this doesn't seem to be spelled out explicitly. A suitable example or perhaps some prose fine tuning should address that.

The implementation is very clear though: for two structs to be identical, two otherwise matching fields must be both embedded or both not embedded.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616697 mentions this issue: spec: clarify that identical structs must have identical field embeddings

@leabit
Copy link
Author

leabit commented Oct 1, 2024

@griesemer

It's not clear to me what this issue has to do with alias or unnamed types

Named types are different without any further check, so issue only makes sense when unnamed types are used.

First example would fail to compile in case pac1.MyType1 and pac2.MyType2 are defined as follows:

pac1:

package pac1

type MyType1 struct{} // defined (named) type

pac2:

package pac2

type MyType1 struct{} // defined (named) type

main:

package main

import (
	"exeexe/pac1"
	"exeexe/pac2"
)

type M1 struct {
	pac1.MyType1
}

type M2 struct {
	pac2.MyType1
}

func main() {
	_ = M2(M1{})
}

Types of the field in M1 in M2 are not the same (unlike in case of unnamed types and aliases where they are the same and the program successfully compiles).

if I understand correctly, the issue is about whether two struct fields are identical if one of them is embedded and the other one is not but they appear in the same "spot" in the struct, have the same name, same type, and same tag.

Yes, you did understand it correctly.

@griesemer
Copy link
Contributor

Thanks, @leabit for confirming. Please comment on the CL if you believe it is not sufficient, thanks.

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation Issues describing a change to documentation.
Projects
Development

No branches or pull requests

7 participants