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

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

Closed
wants to merge 11 commits into from

Conversation

@go101
Copy link

go101 commented Jan 15, 2019

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

@googlebot googlebot added the cla: yes label Jan 15, 2019
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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!

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

This comment has been minimized.

Copy link

gopherbot commented Jan 15, 2019

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 gopherbot force-pushed the golang:master branch from 53bd915 to 6139019 Oct 3, 2019
@gopherbot gopherbot force-pushed the golang:master branch 7 times, most recently from 4a7ed1f to 0f992b9 Nov 5, 2019
@bradfitz 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
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 8, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 8, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 14, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 14, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

Message from Gobot Gobot:

Patch Set 17: TryBot-Result-1

9 of 21 TryBots failed:
Failed on misc-compile-android: https://storage.googleapis.com/go-build-log/8575560a/misc-compile-android_422fde26.log
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/8575560a/freebsd-amd64-12_0_cecbc7a4.log
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/8575560a/android-amd64-emu_43c67e6f.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/8575560a/linux-amd64_b9ed6adb.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/8575560a/windows-amd64-2016_0f6afb20.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/8575560a/linux-386_80d154fd.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/8575560a/windows-386-2008_4604bc11.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/8575560a/openbsd-amd64-64_9324d96a.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/8575560a/linux-amd64-race_edfe9536.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!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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!

gopherbot pushed a commit that referenced this pull request Nov 15, 2019
…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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Nov 15, 2019

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

@gopherbot gopherbot closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.