-
Notifications
You must be signed in to change notification settings - Fork 18k
proposal: spec: allow explicit conversion from function to 1-method interface #47487
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
Comments
To be pedantic, this would require also changing package reflect, so that Value.Convert can convert function values to interface types when the language allows it. I don't remember whether this was mentioned in #21670. |
@zephyrtronium You are correct, forgot to mention that, fixed. |
If we already take two steps in this direction why not take the third one with #25860? Is there a good reason to artificially limit this feature to single method interfaces (for e.g. simpler implementation)? |
When looking at the closure example above, I have to admit that in this case, this syntactic sugar would really help readability, because there is no need to go and read what the countingWriter is and does. All while the current level of type safety is maintained. So, I have to admit this makes me change my mind about this proposal. However as you state :
This points me to something I think is important. In stead of adding this conversion, There could be a convention in Go and iin the standard library where for every one-method reader, there should be an exported adapter function provided as you suggest. Of course, that would still have the performance cost of wrapping the function. This brings me to why I still come to support this conversion proposal: it provides a possibility for improving performance by not having to wrap the closure in a struct and then unwrapping it again. In my experience closures are often used in this way, so it would help to avoid this wrapping. About #25860: interface literals proposed there have a very serious problem with nil safety. One could easily fill in one of the multiple fields with a nil value, causing a panic when using the interface. AFAICS, this conversion here proposed, can be implemented in such a way that it does not have that problem. A conversion from a pointer-to-function to single method interface should be prohibited. |
I don't think the two proposals differ significantly in this regard. I would still allow converting a type T struct{}
func (T) Foo() {}
type Fooer interface {
Foo()
}
func main() {
var f Fooer = (*T)(nil)
f.Foo()
} |
Well, for this proposal, at least we could limit the allowed conversion to the case of a closure as you use in your example, or perhaps anything the compiler can statically see that it cannot be nil. As for the example you show, I consider that unfortunate. I am all in favor of making Go more nil safe whenever possible. So I'd like either proposal to do better than what we have now. |
I am against doing that. We are doing a cost-benefit analysis and I'm only proposing this, because the cost is low. If we have to start distinguishing between different |
Well, yes, perhaps the cost does outweigh the benefit in this case. If I want a static nil checker for go, for this case, I should probably contribute to staticcheck (https://staticcheck.io/docs/checks). |
Based on the discussion in #25860 around |
To be clear, I take it the behavior of type Fooer interface{ Foo() }
type FooFunc func()
func (f FooFunc) Foo() { f() }
func main() {
var f func()
g := FooFunc(f).Foo
fmt.Println(f == nil, g == nil)
g()
}
|
@carlmjohnson That would be my proposal, yes. |
The spec says this about type assertions
We'd have to change that to "must implement or be convertible to" |
This proposal has been added to the active column of the proposals project |
The way I intended, this isn't needed. The dynamic type of the converted interface wouldn't be |
If you know what you want to go to, that's fine. But if you are asking "what kind of implementation of io.Reader is this?" you want to be able to answer that question when the answer is "this was an ordinary function". At least I think you do. |
IMO there is a choice between a) don't make it possible to use type-assertions/-switches to "unpack" the interface, or b) violate the invariant that only defined types can have methods (possibly breaking code using I agree that it's a downside not being able to find out what the dynamic type is. It just seems like the lesser evil. In particular as I don't think the usual use-cases for type-assertions/-switches really apply to this
But if you think supporting it is worth the cost, I won't argue against it. I suggested what seemed the more palatable, less intrusive choice, but I'd take the convenience no matter how it comes :) |
Oh and just to be safe: I assume it is a place-holder, but I don't think "must implement or be convertible to" is a good wording, either way. It would allow to type-assert something with underlying type |
What would |
@fzipp If the underlying type is an autogenerated defined type, that autogenerated name. e.g. Of course, if the underlying type ends up the |
I believe you're looking for #25860. |
@adonovan, that is very similar to what a lot of test mocks/fakes for interfaces do, and I strongly support that way of constructing interface values, if only to get rid of the mocks 😁. |
Change https://go.dev/cl/572835 mentions this issue: |
We've worked on a prototype for the originally suggested direction. Please feel free to use go.dev/cl/572835 to test out the prototype. The composite literal approach mentioned by @adonovan sounds like a promising idea which we may want to consider. I will be working on a prototype for that approach. |
As @ainar-g pointed out, the composite literal approach would be quite handy for mocks/fakes. That would remove a good amount of boiler plate from certain classes of test code I've written or reviewed. In my experience the interfaces involved in these tests often have more than one or even a couple of methods. I think it is typical for different test cases to exercise different subsets of the interface. For that purpose it should be possible to only provide functions for a subset of the interface methods in the composite literal, with the remaining methods causing a panic if called. |
While admittedly convenient, that seems hostile to code evolution. If I add a method to an interface, I want the compiler to point out each place where it can tell statically that a method is missing, not just turn it into a latent crash. |
That is a valid point and it may indeed be a reason not to do as I suggest. However, it seems to me adding methods to interfaces introduces other problems that act as a counterbalance to your concern. If the interface is exported then adding a method is not backwards compatible, so you either have to find all the uses anyway or do a major version bump and then it isn't the same interface anymore. If it is not exported then finding all the uses should be easy. |
Also, I think interface embedding already sets the precedent for panicking in a similar situation. Adding a method to an interface already risks the same problem if the interface is embedded in any struct that would then fail to provide an implementation of the new method. https://go.dev/play/p/FoAENfAYkQZ
|
@adonovan @ChrisHines Let's move the discussion about whether to permit nil/absent methods in interface literals to #25860 which is about interface literals. Thanks. |
When comparing this proposal and interface literals (#25860), consider also this suggestion which eliminates the (syntactic) size difference for single-method interfaces and which makes these two proposals equivalent in that case. |
This proposal and especially #47487 (comment) are unbelievably helpful. Some time ago I wrote about this in https://groups.google.com/g/golang-nuts/c/Oo9Nwm4AZRI/m/uQ6xFWgNCAAJ. Would it make sense to move this into the regular proposal review meeting? |
There is an implementation of this proposal in https://go.dev/cl/572835. Taking off hold. |
It would be good to see concrete examples where this feature is clearly better/more readable etc. than the corresponding manual creation of a struct with associate method. |
@griesemer see linked google discussions code above which has generated code of this form: googlegroups.com. These are all one-method interface that each take an embedding structure to be generated. Only purpose of that struct is to hold the single method that actually makes up that interface. See https://github.com/evcc-io/evcc/blob/master/meter/e3dc_decorators.go for example output generated. I've found that the generated code, used in various places, adds about 10% to binary size. I'd hope that getting rid of the embedding struct might contribute to smaller binary side, too. |
A recent example of mine:
I would like to rewrite this as the following, which is shorter, and lets me keep all the logic inline too:
Another recent example:
could be rewritten as simply:
|
I'll throw in a third example which is a thin
could be rewritten as:
|
It also seems really nice for testing. I’m going to be much more inclined to write fakes when I can easily create them. So there’s a category of code that I think is simply not being written in the first place because it’s just a bit too annoying to write. |
Going through the stdlib, with no claim to completeness:
All of these usages would be trivially obsoleted by this proposal. There is also a few cases where There is also another interesting usage, which definitely isn't solved by this proposal: There is also golang.org/x/text/transform.removeF, which implements a two-method interface (similarly to I believe these are all the cases I found (again, no promise) that used function types with methods in the stdlib. I think the first list (in particular the duplication) shows some demand. Notably, this only takes into account cases where the underlying type is already a function type, so we can conclude with certainty that these cases only exist because a lack of this proposal. Presumably, there are other cases where this could be useful, but they are harder to enumerate. For example io.byteAndEOFReader is an implementation of [edit] I do think this significantly undercounts the use cases of this proposal, because by nature it only includes such cases, where the authors already felt that the convenience of using closures outweighs the added boilerplate of defining a type. If my go source analysis foo was stronger, I'd look for any types that 1. have exactly one method, 2. are only created once and 3. are then assigned to a 1-method interface. All such types could use this proposal. Of course, they would then have to be filtered for whether or not to use a closure, from a readability perspective. |
I think the wording of the proposal here needs to address the problem of unexported names. For example, suppose a library uses the closed form of the variadic options pattern:
In which the library provides a finite set of Therefore, I think the proposal should be that such conversions are only allowed if the method name in question is accessible from the site of the conversion (similar to the rule for unkeyed composite literals). |
I don't think this has any real relevance. Your example supposedly allows the "Option"s to modify something in the server. Either these modifications happen on exported fields in the Server struct, which means that you don't really need options, or they modify unexported fields, which means that there is no way for someone else to provide different set of options. |
@urandom yes, I was just trying to give an example of a one-method interface that is intentionally unexported to control the library's API surface. The example is a little contrived, but the point remains: I think the proposal should disallow implementing methods that aren't accessible from the site of the conversion. Are you disagreeing with that conclusion? |
I do not disagree. Though I'd like to see some real-world examples that clearly illustrate the issue, since the one that's given doesn't really work |
This is forked from #21670. I'd be fine having this closed as a duplicate, but some people suggested that it was different enough to warrant its own tracking issue. As the title suggests, I would like to be able to convert function values to single-method interfaces, if their respective signatures match.
Motivation
My motivation is the same as that proposal. I don't want to have to repeatedly create a type of the form
to use a closure to implement an interface. #21670 mentions
http.HandlerFunc
, but for me the cases that comes up most often areio.Reader
andio.Writer
, which I want to wrap with some trivial behavior. For example, if I want anio.Writer
, which counts the numbers of bytes written, I can writeHowever, this makes the code look more heavyweight than it is, by adding a separate type, when really, the only really relevant line is
w.n += int64(n)
. It also looks like it breaks encapsulation.(*countingWriter).n
is used both by methods of the type and to communicate the end-result.Compare this to
Here, the usage of a closure removes the whole consideration of encapsulation. It's not just less code - it's also code that is more localized and makes the interactions clearer.
We can get part of that currently, by implementing a type like
http.HandlerFunc
. But especially if it's only used once, that smells of over-abstraction and still has the same boiler-plate.Proposal
I propose to add a bullet to the list of allowed conversions of a non-constant value to a type, saying (roughly):
One way to implement this, would be for the compiler to automatically generate a (virtual) defined type, with an arbitrary unexported name in the runtime package.
Discussion
io.Reader
andio.Writer
(for example) use the same signature for their method and it seems unsafe to have a function implement both, implicitly. This proposal addresses that concern, by requiring an explicit type-conversion - the programmer has to explicitly say if the function is intended to be anio.Reader
or anio.Writer
.reflect
(except to allow the conversion itself) or the use of type-assertions is needed.io
package defined an unexportedtype readerFunc func(p []byte) (int, error)
implementingio.Reader
and afunc ReaderFunc(f func(p []byte) (int, error)) Reader { return readerFunc(f) }
.func([]byte) (int, error)
that carries the method, butruntime.io_reader
.func
are not possible, as the type is unexported. That is a plus, because again, thefunc
type can't carry methods so it would be strange to use it in a type-assertion.var r io.Reader
to afunc([]byte) (int, error)
, by simply evaluatingr.Read
.runtime
package, because it seems like if two different packages do afunc
->interface
conversion for the same interface, they should use the same type. I don't think the difference is observable though (the types are not comparable) so we could also put the type in the current package, similar to how closures turn up aspkgpath.func1
in stack-traces.var f func(); g := Fooer(f).Foo
,g
should not benil
, even thoughf
is. Method expressions are nevernil
, currently, and we should maintain this invariant. This requires that the compiler generates wrapper-methods.Further, more general discussion can be found in #21670 - some arguments applying to that proposal, also apply here. As I said, I'm fine with this being closed as a duplicate, if it's deemed to similar - but I wanted to explicitly file this to increase the chance of moving forward.
The text was updated successfully, but these errors were encountered: