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

proposal: Go 2: Add typednil keyword for checking whether an interface value is a typed nil. #24635

Closed
jaekwon opened this issue Apr 2, 2018 · 15 comments
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Milestone

Comments

@jaekwon
Copy link

jaekwon commented Apr 2, 2018

Following from this proposal: #21538, I think an alternative proposal would be to "simply" introduce a new keyword called typednil. It can only used for comparison, and it would have behavior identical to the following code:

func IsTypedNil(o interface{}) bool {
	rv := reflect.ValueOf(o)
	switch rv.Kind() {
	case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.Slice:
		return rv.IsNil()
	default:
		return false
	}
}

This way, the current nil keyword behavior is preserved -- a zero value for interfaces as well as channels, funcs, maps, pointers, and slices, but we could now do the following:

var a interface{} = (*foo)(nil)
a == nil // false
a == typednil // true
var b *foo = nil
var b *foo = typednil // compile time error
b == typednil // compile time error
@mvdan mvdan added LanguageChange Suggested changes to the Go language v2 An incompatible library change labels Apr 2, 2018
@pcostanza
Copy link

I agree with @jaekwon that a simpler proposal than #22729 should be preferred. In #22729, a lot of additional identifiers are added, but most cases of ambiguities between different uses of ‘nil’ are already caught by the static type checker, so I don’t think it’s necessary to add these additional identifiers. The case of an empty-interface nil vs. a typed nil is the only case that is ambiguous, as far as I can tell, and is not caught by the type checker, and can therefore lead to confusing results that are potentially only caught later in testing or even after deployment. So I think a proposal that solves this particular case only is simpler and preferable.

However, I don’t agree with @jaekwon that a typednil is a good solution, because it doesn’t prevent the potentially erroneous ‘v == nil’ test when v is of an interface type, so doesn’t help in solving the issue that bugs may be lurking in code until testing or later. For this reason, I still prefer my suggestion in #21538. I’m not picky about the syntax, though, it can be my suggestion {}, or null, or nilinterface, or something else. What is more important to me is the semantics.

@slrz
Copy link

slrz commented Apr 2, 2018

The fundamental objections raised in #21538 and elsewhere remain unaddressed. If you shouldn't care about the specifics a caller chooses to implement an interface in the first place, why make it any easier to do?

In my opinion, the desire to put this weird constraint on interface values doesn't indicate a flaw in the language that needs to be fixed but rather points to a potential issue with your use of interfaces. These details are up to the implementor of the interface to decide, not on its users. If you want to provide special behaviour for a particular implementation, use a type assertion. Otherwise, you just assume that the passed value is a correct implementation of the interface contract. Using weird heuristics like "is not a nil value" or "is an integer value equal to 42" on the contained value won't do any good here.

@dsnet dsnet changed the title Go2: Add typednil keyword for checking whether an interface value is a typed nil. proposal: Go2: Add typednil keyword for checking whether an interface value is a typed nil. Apr 2, 2018
@gopherbot gopherbot added this to the Proposal milestone Apr 2, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 2, 2018

This proposal is missing background information. What concrete problems is it intended to address?

(Some illustrations or experience reports would be helpful.)

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 2, 2018
@bcmills bcmills changed the title proposal: Go2: Add typednil keyword for checking whether an interface value is a typed nil. proposal: Go 2: Add typednil keyword for checking whether an interface value is a typed nil. Apr 2, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 2, 2018

This clearly relates to @davecheney's experience report, but I think there is something missing from the story: where does the proposed typed-nil-check occur, and why? (That is, what are some concrete call sites where you would actually use this check?)

In particular: if the nil instance of the concrete type is not a valid implementation of the interface, how do you end up with the nil as an instance of that interface in the first place? How do you know that whoever put the nil pointer in that interface value didn't mean for it to be a meaningful implementation?

@jaekwon
Copy link
Author

jaekwon commented Apr 3, 2018

@slrz @bcmills

This came about while implementing Amino, an encoding system inspired by Protobuf3. I could find the exact place where it caused an issue, but it seems to apply to encoding libraries in general.

For example, using encoding/json as the example, https://golang.org/pkg/encoding/json/#Encoder.Encode

func (enc *Encoder) Encode(v interface{}) error

Here, we do care about "the specifics a caller chooses to implement an interface in the first place", because that's what codecs should care about. Or put another way, Golang objects are for more than conceptual-interface-implementations, or its "function". Sometimes what's more important is the form of the object, regardless of its function or how it's referred to. Amino takes this to its logical conclusion, and handles interfaces/concrete-types natively, and makes for a really nice codec.

To focus only on Golang's awesome interface system is to ignore a crucial aspect of the language, imo. Go2 should marry both form and function well.

@jaekwon
Copy link
Author

jaekwon commented Apr 3, 2018

@pcostanza I like your reasoning as well. I wonder how difficult the migration will be. If migration is easy, I agree with your proposal 100%.

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 3, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 3, 2018

Here, we do care about "the specifics a caller chooses to implement an interface in the first place", because that's what codecs should care about.

For the specific case of json.Encoder and similar APIs, a nil pointer that implements json.Marshaler can encode itself to an empty JSON struct (or the keyword null). And if we receive a nil pointer that does not implement json.Marshaler, we're going to use reflect anyway, so the typenil comparison in the language seems to provide no additional benefit.

For those use-cases, it seems like the only benefit of moving the check out of reflect and into the language proper is so that you can convert the codec into a generic API (#15292). But in that case, you would presumably be working with concrete pointer types anyway, so the interface comparison is still moot.

For other kinds of codecs, particularly involving generated code, it often makes sense to define a reflection API for the generated code rather than relying on Go's `reflect' package (see golang/protobuf#291 and golang/protobuf#364).

So I still don't really understand the concrete use-case. Some examples of how it would be used in existing code would be helpful.

@jaekwon
Copy link
Author

jaekwon commented Apr 5, 2018

we're going to use reflect anyway, so the typenil comparison in the language seems to provide no additional benefit.

The big difference between using reflection and using a native operator (e.g. == typednil) is that the latter implies a commitment by the language to ensure that the operation is fast. I believe it's within Go's design philosophy to ensure that what appears like a simple operation, is a simpler operation.

I haven't done any benchmarking recently so, there's that disclaimer. A bit busy now, maybe someone else can jump in and provide some numbers, perhaps disproving my point above. @odeke-em

@bcmills
Copy link
Contributor

bcmills commented Apr 5, 2018

The big difference between using reflection and using a native operator (e.g. == typednil) is that the latter implies a commitment by the language to ensure that the operation is fast.

If you're using reflection anyway, what matters is whether the reflective operation is fast — not the built-in one. (For example, reflect.Call is currently fairly inefficient even for trivial functions, whereas an ordinary function call is often completely free due to inlining: they're conceptually the same operation, but have very different performance properties.)

That's why I'm interested in concrete examples: I suspect that the vast majority of them would not benefit from the built-in operations. Evidence either way would be helpful.

@ianlancetaylor
Copy link
Contributor

There is a problem with nil, but the problem is that people think that v == nil should return true when a typed nil pointer is stored in an interface value (https://golang.org/doc/faq#nil_error). I don't see how adding a new possible test v == typednil will help with that confusion. And I don't understand what other problem is solved by this proposal. If you already understand the difference between a nil interface and a typed nil pointer stored in an interface, then it seems straightforward to check whether an interface value has the nil pointer value that you expect.

@pcostanza
Copy link

I believe the performance discussion is a red herring. There are enough known implementation techniques for efficiently implementing reflection. (For example, a compiler can have built-in translations for what looks like regular reflective API calls, perform partial evaluation, etc.)

The important question, I believe, is: Should you have to import the reflect package (and therefore give the appearance to the reader of your source code that you are indeed using reflection) for what at least to some people feels like shouldn't require reflection? Or is it indeed better that it's a bit harder to discover that you can use the reflect package for this purpose, because it increases the chance that people think harder about their design choices?

Since the proposal discussed here doesn't actually add anything beyond what you can already do in Go, I believe it is less important than the issue discussed in #24682, which I believe objectively improves the language.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 6, 2018
@champioj
Copy link

champioj commented Apr 7, 2018

I believe it is less important than the issue discussed in #24682, which I believe objectively improves the language.

I'm not sure it objectively improves the language.
Now, in this specific case, the rule for comparing an interface with a value is consistent:

  • The types must be the same
  • The value must be the same

You already can check if a pointer contains a nil value, but you have to know its type.
Just like other types:

type MyStr string
type MyInt *int

var istr interface{} = MyStr("")
fmt.Println(istr == "") // false, the type is not the same
fmt.Println(istr == MyStr("")) // true

var iint interface{} = MyInt(nil)
fmt.Println(iint == nil) // false, the type is not the same
fmt.Println(iint == MyInt(nil)) // true

Why must a pointer have a special case? It needs a better use case than a small snippet of code.

Edit: I've mixed up issue #24682 with #24635. I do like the issue #24682 try to address but I still feel uneasy with the special case it adds.

@pcostanza
Copy link

@champioj I apologize for bad wording. I believe #24682 objectively improves the language, not the proposal discussed here. (Your edit suggests that you understood this later, but I should have worded this clearer from the start.)

I understand the uneasiness, but #24682 doesn't actually add a special case. The treatment of nil when compared to interface variables is already a special case, otherwise it wouldn't need extra explanation in tutorials and FAQs. Go's current syntax just makes this case unnecessarily ambiguous, as I demonstrate in a comment on #24682

@ianlancetaylor ianlancetaylor removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 13, 2018
@ianlancetaylor
Copy link
Contributor

If the only problem this solves is a performance problem, then we aren't going to do it. We aren't going to introduce a new language feature only to avoid a type assertion or a function call.

@ianlancetaylor
Copy link
Contributor

We aren't going to add this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

8 participants