Add TryGetExtension API (with TryGetOption)

[ObsidianMinor]

Adds:

• IExtendableMessage.TryGetExtension
• *Descriptor.TryGetOption

[jtattermusch]

It looks like the GetExtension method was a mistake from the very beginning, since it doesn't allow simple handling of the case where the extension is not present (which is a very common case in practice).

The problem seems to be that ExtensionSet.Get returns default(T) if the value is not found, which makes it impossible to distinguish the case where extension is not present at all vs where extension is present, but is set to the default value. (e.g for int, bool, float, etc..)

[ObsidianMinor]

The problem seems to be that ExtensionSet.Get returns default(T) if the value is not found,

It returns the default value of the field which I believe matches up with get methods in other implementations.

[jtattermusch]

Can you provide and example how this is handled e.g. in java?
How can I distinguish between the extension value not being present at all from extension value being present, but set to its default value? Unless I'm missing something, it really feels that the API should allow you to do that. (does e.g. java allow that?).

[ObsidianMinor]

// in file scope
optional int32 bar = 99999 [default = 3];

// default
int value = msg.getExtension(FooFile.Bar); // 3

msg.setExtension(FooFile.Bar, 3);
if (msg.hasExtension(FooFile.Bar)) { // true
    value = msg.getExtension(FooFile.Bar); // 3
}

Custom options don't work the same way for us since java has builders and readers so they can just return the descriptor options reader and get the same API. We could add a HasOption API, but TryGet fixes the need for that.

[jtattermusch]

Thanks. Ok so to me it looks like we basically have two ways of handling this:

• introduce TryGetExtension method, at which point GetOption becomes pretty much useless (because it can return default value for an extension that isn't present, which seems semantically wrong), but the issue is we already have introduced GetExtension, so getting rid of it shortly after introducing is odd.

• we can keep GetExtension (because it already exists) and introduce a new HasExtension method which allows checking whether the value is present or not. If GetExtension() is called for a value that is not present, we can throw / return the default value (what the is getExtension method's behavior in java?).

[ObsidianMinor]

getExtension returns the default value.

[jtattermusch]

Ok, I didn't notice that we already have a "HasExtension" method so my last comment is partially wrong.
Because we already have the HasExtension/GetExtension pair, I don't think we need TryGetExtension at all (why increase the API surface if all the necessary functionality is already there?).

In terms of options, I think both GetOption and TryGetOption are unnecessary and we should get rid of them. See my PR: #7434

[jtattermusch]

Adding this method is basically just duplicating the Extension API and we should avoid that.
See #7434 and let me know what you think.

[jtattermusch]

Superseded by #7434 (and GetOption API is now obsolete).

[ObsidianMinor]

Do you think we could still use the IExtendableMessage.TryGetExtension API even though we aren't adding *Descriptor.TryGetOption with it?

[jtattermusch]

I think we should have either "GetExtension and HasExtension" or "TryGetExtension" model, not both. We should try to keep the API surface as small as possible.
We already have GetExtension and HasExtension, so I don't think we need TryGetExtension.
Besides that, adding methods to an existing interface is problematic (it's not backward compatible).