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

reflect: NumMethod includes non-exported methods for interface types #22075

Closed
mdempsky opened this issue Sep 28, 2017 · 12 comments
Closed

reflect: NumMethod includes non-exported methods for interface types #22075

mdempsky opened this issue Sep 28, 2017 · 12 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@mdempsky
Copy link
Contributor

Type.Method's docs say:

NumMethod returns the number of exported methods in the type's method set.

Value.Method's docs say:

NumMethod returns the number of exported methods in the value's method set.

But this program demonstrates that they both include the number of non-exported methods when applied to interface types: https://play.golang.org/p/Drv0u6n9bg

According to the documentation, it should print "0 0", but instead it prints "1 1".

/cc @ianlancetaylor @crawshaw

Related #22073.
@mdempsky
Copy link
Contributor Author

Incidentally, if we fix this such that you can only access exported methods (as appears to be the intent), reflect.Method's PkgPath field becomes unnecessary (it will always be ""). However, we can't remove it until Go 2.

@mvdan mvdan self-assigned this Feb 19, 2018
@mvdan
Copy link
Member

mvdan commented Feb 20, 2018

@mdempsky I see multiple potential solutions:

  • Use rtype for its exportedMethods method (although I presume this is not an option, sine interfaces currently handle methods on their own - is there a good reason to keep the code split in two?)
  • Add an exportedMethods method to interfaceType, with caching similar to methodCache
  • Add an exportedMethods field to interfaceType, statically containing the subset of exported methods

@mvdan
Copy link
Member

mvdan commented Mar 15, 2018

Ping, @mdempsky?

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 15, 2018
@mdempsky mdempsky added this to the Go1.11 milestone Mar 15, 2018
@mdempsky
Copy link
Contributor Author

@mvdan Assuming the NumMethods documentation is correct and the implementation is just wrong for interfaces, then those solutions sound reasonable.

I wanted some of the reflect API experts to weigh in first on whether the code needs to be fixed, or the document just needs to be updated.

@mvdan
Copy link
Member

mvdan commented Mar 15, 2018

Gotcha - unassigning self for now. I thought this didn't need a decision.

@mvdan mvdan removed their assignment Mar 15, 2018
@mdempsky
Copy link
Contributor Author

@mvdan For what it's worth, my 2c is we should fix the code to match the current documentation. The main hesitation in my mind is just whether users are inadvertently relying on the current behavior.

If you want to speculatively work on a fix, that might also help answer the question of whether users depend on the current behavior or not.

As for recommended fix, I'm not sure we need methodCache at all. I would think the compiler could sort all exported methods before nonexported methods, and then we can add a xcount field next to mcount to indicate how many methods are exported. I think as long as we sort concrete and interface methods the same way, the "Implements" algorithm should still work fine. (I feel like I've described this in an issue or during a CL review before, but can't find it off hand.)

I think uncommontype and interfacetype will need to keep separate exportedMethods code, because the former returns []method (i.e., concrete methods), while the latter returns []imethod (i.e., interface methods). However, if we eliminate methodCache, I think the code duplication will go down substantially.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/100846 mentions this issue: reflect: sort exported methods first

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/100845 mentions this issue: cmd/compile: sort method sets earlier

@mdempsky mdempsky mentioned this issue Mar 15, 2018
Open
@mdempsky
Copy link
Contributor Author

CL 100846 above gets rid of methodCache. It should be much easier to build on to fix NumMethod for interface types.

gopherbot pushed a commit that referenced this issue Mar 15, 2018
@mdempsky
cmd/compile: sort method sets earlier
91bbe53 
By sorting method sets earlier, we can change the interface
satisfaction problem from taking O(NM) time to O(N+M). This is the
same algorithm already used by runtime and reflect for dynamic
interface satisfaction testing.

For #22075.

Change-Id: I3d889f0227f37704535739bbde11f5107b4eea17
Reviewed-on: https://go-review.googlesource.com/100845
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Mar 15, 2018
@mdempsky
reflect: sort exported methods first
86a3389 
By moving exported methods to the front of method lists, filtering
down to only the exported methods just needs a count of how many
exported methods exist, which the compiler can statically
provide. This allows getting rid of the exported method cache.

For #22075.

Change-Id: I8eeb274563a2940e1347c34d673f843ae2569064
Reviewed-on: https://go-review.googlesource.com/100846
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ALTree ALTree mentioned this issue Apr 18, 2018
Closed
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 27, 2018
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Jun 29, 2018
@dmitshur
natives/reflect: Sort exported methods first; add xcount to uncommonT…
5d6fd67 
…ype.

What was previously an unused blank identifier field of type uint16 in
Go 1.10 has become the xcount field in Go 1.11. Update our uncommonType
to match.

Also remove the unused uint32 field at the end. In normal Go, it's used
for padding and to keep the struct size fixed size. GopherJS doesn't
need that, because it doesn't support unsafe memory operations.

Fixes:

	$ gopherjs build reflect
	reflect/type.go:2603:5: invalid operation: ut (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:643:7: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:88: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:97: invalid operation: t (variable of type *uncommonType) has no field or method xcount

Follows golang/go@86a3389.
Updates golang/go#22075.
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Jun 29, 2018
@dmitshur
natives/reflect: Sort exported methods first; add xcount to uncommonT…
2556912 
…ype.

What was previously an unused blank identifier field of type uint16 in
Go 1.10 has become the xcount field in Go 1.11. Update our uncommonType
to match.

Also remove the unused uint32 field at the end. In normal Go, it's used
for padding and to keep the struct size fixed size. GopherJS doesn't
need that, because it doesn't support unsafe memory operations.

Fixes:

	$ gopherjs build reflect
	reflect/type.go:2603:5: invalid operation: ut (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:643:7: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:88: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:97: invalid operation: t (variable of type *uncommonType) has no field or method xcount

Follows golang/go@86a3389.
Updates golang/go#22075.
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Jun 29, 2018
@dmitshur
natives/reflect: Sort exported methods first; add xcount to uncommonT…
4867319 
…ype.

What was previously an unused blank identifier field of type uint16 in
Go 1.10 has become the xcount field in Go 1.11. Update our uncommonType
to match.

Also remove the unused uint32 field at the end. In normal Go, it's used
for padding and to keep the struct size fixed size. GopherJS doesn't
need that, because it doesn't support unsafe memory operations.

Fixes:

	$ gopherjs build reflect
	reflect/type.go:2603:5: invalid operation: ut (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:643:7: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:88: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:97: invalid operation: t (variable of type *uncommonType) has no field or method xcount

Follows golang/go@86a3389.
Updates golang/go#22075.
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Aug 21, 2018
@dmitshur
natives/reflect: Sort exported methods first; add xcount to uncommonT…
884657e 
…ype.

What was previously an unused blank identifier field of type uint16 in
Go 1.10 has become the xcount field in Go 1.11. Update our uncommonType
to match.

Also remove the unused uint32 field at the end. In normal Go, it's used
for padding and to keep the struct size fixed size. GopherJS doesn't
need that, because it doesn't support unsafe memory operations.

Fixes:

	$ gopherjs build reflect
	reflect/type.go:2603:5: invalid operation: ut (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:643:7: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:88: invalid operation: t (variable of type *uncommonType) has no field or method xcount
	reflect/type.go:646:97: invalid operation: t (variable of type *uncommonType) has no field or method xcount

Follows golang/go@86a3389.
Updates golang/go#22075.
@rsc
Copy link
Contributor

rsc commented Nov 14, 2018

Let's do this early in Go 1.13. I'm scared of changing it now but it seems like a reasonable change.

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 14, 2018
@andybons andybons removed this from the Go1.13 milestone Jul 8, 2019
@andybons andybons added this to the Go1.14 milestone Jul 8, 2019
gazerro added a commit to open2b/scriggo that referenced this issue Jul 29, 2019
@gazerro
compiler/checker: use t.Implements instead of t.NumMethod() == 0
67576be 
See golang/go#22075
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@deadprogram deadprogram mentioned this issue Mar 16, 2020
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/259237 mentions this issue: cmd/compile: split exported/non-exported interface methods in mhdr/methods

This was referenced Oct 9, 2020
Closed
Closed
This was referenced Oct 20, 2020
Closed
Closed
Merged
@rsc rsc mentioned this issue Oct 21, 2020
Closed
@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

Filed #42123 to roll this back. It looks like it breaks too much.

@golang golang locked and limited conversation to collaborators Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@mdempsky @rsc @andybons @ianlancetaylor @mvdan @gopherbot