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.

Sign up for GitHub

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

Jump to bottom

go/types, types2: type-checker internal type printing doesn't print embedded fields that are alias types correctly #44410

Closed
griesemer opened this issue Feb 19, 2021 · 12 comments
Closed

go/types, types2: type-checker internal type printing doesn't print embedded fields that are alias types correctly #44410

griesemer opened this issue Feb 19, 2021 · 12 comments
Assignees
@griesemer
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@griesemer
Copy link
Contributor

Given

type E = map[string]int
type S struct {
   E
}

the struct S will be printed with an embedded field named map[string]int because we have lost information about its alias name.

This affects debugging and test cases, e.g., src/go/internal/gccgoimporter/importer_test.go:97 wants "type S struct{b int; A2" but we only have "type S struct{b int; map[Y]Z}".

The relevant printing code is in go/types/typestring.go, function writeType.
@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2021
@griesemer griesemer added this to the Go1.18 milestone Feb 19, 2021
@griesemer
Copy link
Contributor Author

If we solve the type alias related issues (by introducing an explicit type alias node), this issue would probably be fixed as well.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/293962 mentions this issue: [dev.typeparams] go/types, types2: revert fancy struct printing (fixes x/tools tests)

gopherbot pushed a commit that referenced this issue Feb 19, 2021
@griesemer @findleyr
[dev.typeparams] go/types, types2: revert fancy struct printing (fixe…
dfe0ef9 
…s x/tools tests)

An embedded struct field is embedded by mentioning its type.
The fact that the field name may be different and derived
from the type doesn't matter for the struct type.

Do print the embedded type rather than the derived field
name, as we have always done in the past. Remove the fancy
new code which was just plain wrong.

The struct output printing is only relevant for debugging
and test cases. Reverting to the original code (pre-generics)
fixes a couple of x/tools tests.

Unfortunately, the original code is (also) not correct for
embedded type aliases. Adjusted a gccgoimporter test
accordingly and filed issue #44410.

This is a follow-up on https://golang.org/cl/293961 which
addressed the issue only partially and left the incorrect
code in place.

Change-Id: Icb7a89c12ef7929c221fb1a5792f144f7fcd5855
Reviewed-on: https://go-review.googlesource.com/c/go/+/293962
Trust: Robert Griesemer <[email protected]>
Run-TryBot: Robert Griesemer <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 9, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/348737 mentions this issue: cmd/compile/internal/types2: print correct embedded field name

@ianlancetaylor
Copy link
Member

@griesemer This is in the 1.18 milestone; time to move to 1.19? Thanks.

@griesemer
Copy link
Contributor Author

griesemer commented Jan 28, 2022 •
edited
Loading

CL 381776, if it can be made to work, would fix this. Leaving open for a bit longer. But fine to move to 1.19 once we decide to move forward with the release.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/381958 mentions this issue: cmd/compile/internal/types2: correctly print embedded struct fields that are aliases

@griesemer
Copy link
Contributor Author

There's no easy fix that doesn't require major restructuring. We have lived with this for a long time, so not a regression. Leaving for 1.19.

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Feb 1, 2022
@mvdan
Copy link
Member

mvdan commented Feb 23, 2022

If we solve the type alias related issues (by introducing an explicit type alias node), this issue would probably be fixed as well.

I would absolutely love having recorded information about aliases in go/types. This information is currently missing, so I'm attempting to piece it back together for a tool of mine that needs to be aware of type aliases, because their name matters when they are used as an anonymous struct field. The code I came up with to do that reconstruction is complex, buggy, and very hard to debug.

I am more than happy to help any way I can, be it reviewing code, testing, etc. I would hope that the type alias information would be available to go/types users, perhaps via an extra field in the types.Info struct.

@mvdan mvdan mentioned this issue Feb 24, 2022
Closed
@griesemer
Copy link
Contributor Author

A proper fix will require quite a bit of work on alias type representation. We won't get to this for 1.19, but we may want to make it a goal for 1.20. Adjusting milestone.

@griesemer griesemer modified the milestones: Go1.19, Go1.20 Mar 30, 2022
@griesemer griesemer modified the milestones: Go1.20, Go1.21 Oct 24, 2022
@wyattanderson wyattanderson mentioned this issue Feb 20, 2023
Merged
21 tasks
@griesemer
Copy link
Contributor Author

Will be fixed once we address #25838.
Moving to 1.22.

@griesemer griesemer modified the milestones: Go1.21, Go1.22 May 16, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/521956 mentions this issue: go/types, types2: introduce _Alias type node

gopherbot pushed a commit that referenced this issue Nov 9, 2023
@griesemer @gopherbot
go/types, types2: introduce _Alias type node
bea5513 
This change introduces a new (unexported for now) _Alias type node
which serves as an explicit representation for alias types in type
alias declarations:

        type A = T

The _Alias node stands for the type A in this declaration, with
the _Alias' actual type pointing to (the type node for) T.
Without the _Alias node, (the object for) A points directly to T.

Explicit _Alias nodes permit better error messages (they mention
A instead of T if the type in the source was named A) and can help
with certain type cycle problems. They are also needed to hold
type parameters for alias types, eventually.

Because an _Alias node is simply an alternative representation for
an aliased type, code that makes type-specific choices must always
look at the actual (unaliased) type denoted by a type alias.
The new function

        func _Unalias(t Type) Type

performs the necessary indirection. Type switches that consider
type nodes, must either switch on _Unalias(typ) or handle the
_Alias case.

To avoid breaking clients, _Alias nodes must be enabled explicitly,
through the new Config flag _EnableAlias.

To run tests with the _EnableAlias set, use the new -alias flag as
in "go test -run short -alias". The testing harness understands
the flag as well and it may be used to enable/disable _Alias nodes
on a per-file basis, with a comment ("// -alias" or // -alias=false)
on the first line in those files. The file-based flag overrides the
command-line flag.

The use of _Alias nodes is disabled by default and must be enabled
by setting _EnableAlias.

Passes type checker tests with and without -alias flag set.

For #25838.
For #44410.
For #46477.

Change-Id: I78e178a1aef4d7f325088c0c6cbae4cfb1e5fb5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/521956
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Griesemer <[email protected]>
@griesemer
Copy link
Contributor Author

griesemer commented Nov 16, 2023 •
edited
Loading

This is fixed if Alias nodes are enabled via GODEBUG=gotypesalias=1. Closing.

[edit: corresponding test was added with CL 521956.]

@LandonTClipp LandonTClipp mentioned this issue Feb 14, 2024
Closed
4 tasks
@golang golang locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@griesemer griesemer

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
5 participants
@ianlancetaylor @mvdan @griesemer @gopherbot @findleyr