reflect: factor out special channel assignability rule from haveIdenticalUnderlyingType #29739

go101 · 2019-01-15T00:12:38Z

Go specification says: A value x is assignable to a variable of type T if x
is a bidirectional channel value, T is a channel type, x's type V and T have
identical element types, and at least one of V or T is not a defined type.
However, the current reflection implementation is incorrect which makes
"x is assignable to T" even if type V and T are both defined type.

The current reflection implementation also mistakes the base types of two
non-defined pointer types share the same underlying type when the two
base types satisfy the above mentioned special channel assignability rule.

Fixes #29469

fixed golang#29469

gopherbot · 2019-01-15T00:13:40Z

This PR (HEAD: 54527fc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-01-15T00:33:37Z

Message from Ian Lance Taylor:

Patch Set 1:

(4 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T13:20:25Z

This PR (HEAD: ccd30dc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-01-15T13:26:58Z

Message from Tapir Liu:

Patch Set 3:

Patch Set 1:

(4 comments)

I modified the commit message and test file.

T and V can have the same identical underlying type even if one of them is named.

This PR doesn't deny this.

And it looks, this is only possible place which could cause the bug.
The only thing I can't make sure is whether or not this PR will affect other reflection functions/methods, for the function "haveIdenticalUnderlyingType" is called several times elsewhere.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T13:31:26Z

This PR (HEAD: e5f3a18) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-01-15T14:52:34Z

Message from Tapir Liu:

Patch Set 5:

In other "haveIdenticalUnderlyingType" calls, three are for function and three are for struct types, the modification in this PR doesn't affect the six uses.

The 2nd "haveIdenticalUnderlyingType" call in "convertOp" causes the new bug #29469 (comment) (the PR unrelated).
I will submit a new patch to fix this bug too.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T14:56:15Z

Message from Ian Lance Taylor:

Patch Set 5:

I modified the commit message and test file.

T and V can have the same identical underlying type even if one of them is named.

This PR doesn't deny this.

And it looks, this is only possible place which could cause the bug.
The only thing I can't make sure is whether or not this PR will affect other reflection functions/methods, for the function "haveIdenticalUnderlyingType" is called several times elsewhere.

Yes, exactly. In particular, it seems that this might change the behavior when working with types like

type C1 chan int
type C2 chan int
type F1(C1)
type F2(C2)

The Go spec section that you mention talks only about converting between types at the top level, while it appears that this change is going to affect converting between types at other levels.

This does not seem to me to be the only place that could cause this bug. Since the bug seems to only be about the top level types being converted, we could add a case to the directlyassignable function.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T15:18:48Z

Message from Tapir Liu:

Patch Set 5:

The PR also doesn't affect the "haveIdenticalUnderlyingType" call in the "haveIdenticalType" function.

So now, the only side effects may happen is for the 2nd
"haveIdenticalUnderlyingType" call in function "convertOp".
However, here the use of "haveIdenticalUnderlyingType"
is already problematic before the PR.

..., we could add a case to the directlyassignable function.

Agree, and we also need add a case (for the special chnannel assignability) in the convertOp function.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T16:03:05Z

This PR (HEAD: 37b24b2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-01-15T16:05:01Z

Message from Tapir Liu:

Patch Set 5:

pushed a new patch.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T17:42:37Z

Message from Ian Lance Taylor:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-15T21:47:42Z

Message from Tapir Liu:

Patch Set 7:

done.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-08T19:01:00Z

Message from Brad Fitzpatrick:

Patch Set 10:

Ian, I fixed the commit message. Want to review?

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-08T19:01:01Z

Message from Brad Fitzpatrick:

Patch Set 10: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T15:49:16Z

Message from Ian Lance Taylor:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T15:58:54Z

Message from Tapir Liu:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T16:00:00Z

Message from Tapir Liu:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T16:03:19Z

Message from Ian Lance Taylor:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T16:47:23Z

Message from Tapir Liu:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T17:57:18Z

Message from Ian Lance Taylor:

Patch Set 14:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T23:06:02Z

This PR (HEAD: bf2718c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-11-13T23:09:17Z

Message from Tapir Liu:

Patch Set 15:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-13T23:54:58Z

Message from Ian Lance Taylor:

Patch Set 15:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-14T13:39:21Z

This PR (HEAD: e7cde2b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-11-14T13:51:47Z

Message from Tapir Liu:

Patch Set 16:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T00:13:42Z

Message from Ian Lance Taylor:

Patch Set 16:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T00:38:13Z

This PR (HEAD: 487c20a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/157822 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-11-15T00:40:47Z

Message from Tapir Liu:

Patch Set 17:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T02:38:17Z

Message from Ian Lance Taylor:

Patch Set 17: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T02:38:18Z

Message from Gobot Gobot:

Patch Set 17:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8575560a

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T02:42:25Z

Message from Gobot Gobot:

Patch Set 17:

Build is still in progress...
This change failed on misc-compile-android:
See https://storage.googleapis.com/go-build-log/8575560a/misc-compile-android_422fde26.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T02:45:28Z

Message from Gobot Gobot:

Patch Set 17: TryBot-Result-1

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T03:27:43Z

Message from Ian Lance Taylor:

Patch Set 18: Patch Set 17 was rebased

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T03:27:44Z

Message from Ian Lance Taylor:

Patch Set 18: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T03:27:45Z

Message from Gobot Gobot:

Patch Set 18:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ca32bcb1

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T03:30:56Z

Message from Gobot Gobot:

Patch Set 18:

Build is still in progress...
This change failed on misc-compile-android:
See https://storage.googleapis.com/go-build-log/ca32bcb1/misc-compile-android_3157c5b8.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T03:38:30Z

Message from Gobot Gobot:

Patch Set 18: TryBot-Result-1

1 of 21 TryBots failed:
Failed on misc-compile-android: https://storage.googleapis.com/go-build-log/ca32bcb1/misc-compile-android_3157c5b8.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

…icalUnderlyingType Go specification says: A value x is assignable to a variable of type T if x is a bidirectional channel value, T is a channel type, x's type V and T have identical element types, and at least one of V or T is not a defined type. However, the current reflection implementation is incorrect which makes "x is assignable to T" even if type V and T are both defined type. The current reflection implementation also mistakes the base types of two non-defined pointer types share the same underlying type when the two base types satisfy the above mentioned special channel assignability rule. Fixes #29469 Change-Id: Ia4b9c4ac47dc8e76a11faef422b2e5c5726b78b3 GitHub-Last-Rev: 487c20a GitHub-Pull-Request: #29739 Reviewed-on: https://go-review.googlesource.com/c/go/+/157822 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2019-11-15T15:21:32Z

Message from Ian Lance Taylor:

Patch Set 18: Code-Review+2

Thanks for pushing through this.

Please don’t reply on this GitHub thread. Visit golang.org/cl/157822.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-11-15T15:23:43Z

This PR is being closed because golang.org/cl/157822 has been merged.

fix haveIdenticalUnderlyingType for channels

54527fc

fixed golang#29469

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jan 15, 2019

remove one duplicate line in test file

ccd30dc

update comment

e5f3a18

fix another reflect bug

37b24b2

go101 changed the title ~~reflect: fix haveIdenticalUnderlyingType for channels~~ reflect: seperate the special channel assignability rule from haveIdenticalUnderlyingType Jan 15, 2019

add a "of" in comment

4259623

gopherbot force-pushed the master branch from 53bd915 to 6139019 Compare October 3, 2019 21:09

gopherbot force-pushed the master branch 7 times, most recently from 4a7ed1f to 0f992b9 Compare November 5, 2019 02:57

bradfitz changed the title ~~reflect: seperate the special channel assignability rule from haveIdenticalUnderlyingType~~ reflect: factor out special channel assignability rule from haveIdenticalUnderlyingType Nov 8, 2019

replace chan int to chan NoDup

bf2718c

NoDup -> []byte

e7cde2b

add IntChan test cases

487c20a

gopherbot closed this Nov 15, 2019

reflect: factor out special channel assignability rule from haveIdenticalUnderlyingType #29739

reflect: factor out special channel assignability rule from haveIdenticalUnderlyingType #29739

Conversation

go101 commented Jan 15, 2019 • edited by bradfitz

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Jan 15, 2019

gopherbot commented Nov 8, 2019

gopherbot commented Nov 8, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 13, 2019

gopherbot commented Nov 14, 2019

gopherbot commented Nov 14, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

gopherbot commented Nov 15, 2019

go101 commented Jan 15, 2019 •

edited by bradfitz