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

Jump to bottom

dev.go2go: can take address from map indexing when map inside a type list #42616

Closed
tdakkota opened this issue Nov 15, 2020 · 5 comments
Closed

dev.go2go: can take address from map indexing when map inside a type list #42616

tdakkota opened this issue Nov 15, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@tdakkota
Copy link

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

$ go version
go version devel +2298dab082 Thu Oct 22 00:19:08 2020 +0000 windows/amd64

Does this issue reproduce with the latest release?

n/a

What operating system and processor architecture are you using (go env)?

  • windows/amd64
  • go2go playground

What did you do?

https://go2goplay.golang.org/p/bq7UO-0NYT9
That code passes go2go type checker.
Error is printing when go1 compiler tries to compile translated code.

This bug is caused by:

go/src/go/types/expr.go

Lines 1402 to 1437 in 2298dab

case *Sum:
// A sum type can be indexed if all the sum's types
// support indexing and have the same element type.
var elem Type
if typ.is(func(t Type) bool {
var e Type
switch t := t.Under().(type) {
case *Basic:
if isString(t) {
e = universeByte
}
case *Array:
e = t.elem
case *Pointer:
if t := t.base.Array(); t != nil {
e = t.elem
}
case *Slice:
e = t.elem
case *Map:
e = t.elem
case *TypeParam:
check.errorf(x.pos(), "type of %s contains a type parameter - cannot index (implementation restriction)", x)
case *instance:
panic("unimplemented")
}
if e != nil && (e == elem || elem == nil) {
elem = e
return true
}
return false
}) {
valid = true
x.mode = variable
x.typ = elem
}

There are:

				valid = true
				x.mode = variable
				x.typ = elem

x.mode should be mapindex if type list contains a map, because in that case index expression is not addressable.

Same on dev.typeparams

go/src/cmd/compile/internal/types2/expr.go

Lines 1510 to 1512 in 6877ee1

valid = true
x.mode = variable
x.typ = elem

What did you expect to see?

An error like cannot take the address of s[0] from go2go.
Or, maybe, go2 will allow to take an address from map indexing.

What did you see instead?

Code passes go2go type checker.
@ianlancetaylor
Copy link
Contributor

The bug here is that type checker should report an error, but does not.

Note that we don't expect to continue to support the dev.go2go branch. But we should make sure that the current type checking code doesn't accept this code. Thanks.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 15, 2020
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Nov 15, 2020
@griesemer
Copy link
Contributor

@tdakkota Thanks for finding this and locating the source of the bug! Very helpful.

Note that changing the mode to mapindex is not correct either in general: If the type list contains non-map types and we switch to mapindex, one would be able to write a comma-ok expression in cases where that would be incorrect. The correct approach is to select the mode variable if there are no maps, value if there are maps and other types, and mapindex if there are only maps.

Fix forthcoming.

@griesemer
Copy link
Contributor

As an aside, there's another bug in this code: If we have a map with a key that's not an integer, we cannot index that map.

@gopherbot
Copy link

Change https://golang.org/cl/270657 mentions this issue: [dev.go2go] go/types: fix indexing of type parameter values constraint by maps

gopherbot pushed a commit that referenced this issue Nov 17, 2020
@griesemer
[dev.go2go] go/types: fix indexing of type parameter values constrain…
bac01b0 
…t by maps

For instance, given a constraint

	interface { type []E, map[K]E }

and a variable x of type parameter type constraint by above constraint,
the index expression x[i] is only permitted if K is an integer type.
Also, &x[i] is not permitted if the type list contains a map type since
map index expressions are not addressable. Finally, if the type list
contains only compatible map types, the index expression x[i] can be
used in a comma-ok assignment.

Fixes #42616.

Change-Id: I6287de47619acee25efbcd73a7b04b835d26a885
Reviewed-on: https://go-review.googlesource.com/c/go/+/270657
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

This is now fixed on the go2go playground.

@golang golang locked and limited conversation to collaborators Nov 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@ianlancetaylor @griesemer @gopherbot @tdakkota