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

C#: Consider exposing protobuf optional fields as nullable properties #9083

Closed
LeonarddeR opened this issue Oct 8, 2021 · 42 comments
Closed
Assignees
Labels

Comments

@LeonarddeR
Copy link

Originally posted as grpc/grpc#27501 as I thought it belonged to @grpc

Is your feature request related to a problem? Please describe.

The way optional fields are implemented in C# is slightly hacky. For Strings, the underlying private field can be null, but the public property raises an exception upon null assignments. For non nullable (value type) fields, the underlying magic is even more scary.

Describe the solution you'd like

Consider implementing optional fields as nullable properties in C#. This has the following benefits:

  1. It takes away the need for using Protobuf's Well Known Types to have fields that translated to nullable C# properties.
  2. It allows you to use optional fields in expressions, such as when projecting EF Core models into protobuf messages. For strings for example, you currently need to create a partial class to add an additional property that exposes the internal private field without the null check.
  3. It exposes info about whether a field has been set in a language natural way, in contrast to the several HasY properties and ClearY methods currently necessary. With the nullable wrapper types, null is considered a real value, whereas with optional fields, null can be considered a way to communicate that the value of an optional field is kept empty.
@elharo elharo added the c# label Oct 9, 2021
@deannagarcia
Copy link
Member

We get requests in some other languages that have null support to allow optional fields to have nullable properties. The main problem is that we like to keep languages consistent and other languages don't have those same options. Additionally, it's not clear with some primitive fields what a null value would be (is it the default? What if users meant to set it to that option?). For those reasons, we will not support this in the near future.

@JohnGalt1717
Copy link

JohnGalt1717 commented Jul 15, 2022

Literally the point to optional is to re-enable nullable support that was readily available in v2. (Literally someone in the protobuf team thought they knew better and removed it and got hammered into putting it back) There is no other case. If no value is passed (because it’s optional) it’s ALWAYS a null value in all languages because that’s literally what null means: no entry.

In languages without nullable types the property of the object is still set to null they just have to check for null.

This is how ALL serializers ever have ALWAYS handled this case.

This is the #1 reason we refuse to use GRPC and protobuf. It results in bad coding technique because null in data entry is a baseline requirement. If someone has the option of not entering a value, the value is null. Ie no entry. Not supporting nullable types because other languages are in the dark ages is not a valid argument.

As it stands GRPC+protobuf requires bad hacks to deal with a base use case of all CRUD operations that is unacceptable. And no there isn’t some magic value that isn’t null for every data type that can be used in its stead. Only enums have the possibility to overcome the limitation you’re creating and all others (especially bool) have no possible way to represent no entry without using 2 fields in your data model which makes protobuf pointless because it bloats the payload.

Please reconsider, and provide baseline functionality that literally ALL serializers in ALL languages supports and is required to provide a good UX without hacks for serializing. And use the full functionality of the language in question. Dart and c# should just work with nullable types as a small example.

And this should be ultra high priority for all languages. If the language supports explicit nullability then use it. If not, optional still sets the property to null. If some insane language doesn’t allow a null to be set on a property then it should just throw with optional fields.

And honestly its inconceivable that protobuf still doesn’t have basic support required for effective form entry CRUD in 2022 given that Microsoft had this same argument all of the way back in the VB days and created an entire cottage industry of 3rd party controls that charged money to fix Microsoft’s refusal that allow null entry on date and combo controls to name two. It took them until winforms v2 to finally admit they were wrong and fix their mistake. This is a solved problem and a resolved argument that millions of developers voted with their money to prove that the ideology of those that think otherwise is wrong. And I’ve talked to other devs that have evaluated GRPC and protobuf and they’ve have rejected it for this very reason.

@fowles
Copy link
Contributor

fowles commented Jul 15, 2022

proto2 supports explicitly specifying the default values of fields to be non-zero/empty values. When types get pushed out as language level optional, that fact is immediately lost. Doing what you suggest leads directly to a proliferation of bugs where some callers respect the explicitly specified default from the proto file and others force the question of what value to use instead to every person accessing such a default. The problem is made even worse by languages that provide a very convenient default to zero/empty mapping.

@JohnGalt1717
Copy link

So you outlined bad behavior by languages and implementations not reasons not to do it right.

If there is a default value specified and the field optional then the default value goes over the wire. (Or it should fail to compile the proto because it should be required and have a default. It’s up to you which way you want to go). If there is no default value and optional is specified then null is passed and serialized and deserialized on the other side not some magic default value of the language. Optional and not entered is always, 100% of the time in all cases in all language NULL. Not some default, NULL because null is no entry by all definitions dating back 70 years. It has no other meaning in computer science.

Again I’ve used dozens of languages not one serializer I have ever used hasn’t worked exactly as I just described.

And that includes Google languages like Dart and Go. Json in Dart for example uses nullable types and works exactly as I outlined above (including default values). All serializers in c# do the same. I’m not aware of any in Java that don’t also. (I’m not as fluent in Java so it’s possible there is). JavaScript either does null or undefined and the language doesn’t support nullable types. So even archaic languages do it correctly. Even VB sets null properly and WSDL and xml in all Languages support null and explicitly define how to serialize/deserialize null values and fields with defaults and the rules are explicitly what I outlined above.

And yes, if you do it like 70 years of computer science has solved you eliminate bugs, not create them. We know this because the original case for nullable types when first implemented by c# was explicitly to serialize and deserialize null to relational databases and eliminate all of the bugs caused by failure to check for null. And no it didn’t introduce new bugs because of database default values. Even if the field in the object model was nullable and the database set a default it just meant you’d never get a null, it wouldn’t cause a bug. But the opposite case almost always introduces bugs. (I know because I helped write the specs for nullable types in c# back in the day when no languages had them and the #1 most frequent bug in all languages was “object not set to an instance of an object” or some variation thereof. In nullable type languages that is no longer the case specifically because of this solved problem. )

@LeonarddeR
Copy link
Author

How about this alternative for optional fields that it is again allowed to set the value to null (at least for C# reference types) instead of raising an exception when setting the value to null? This latter behaviour is pretty confusing. As said, clearing the value of an optional sets the underlying field to null already. It's just that it was somehow decided that the property setter needed to be overriden to raise an exception when setting it to null. This is a bit upside down for me.

@jskeet
Copy link
Contributor

jskeet commented Jul 15, 2022

Here's my short analysis of the situation, taking into account where we already are (rather than where we might be in an ideal world). The TL;DR is that I can see one change we could make, but I don't propose we actually make it.

  • We need to consider numeric types, string/bytes, and messages as three separate concerns. I believe those are the only categories though, and that all numeric types can be treated the same way as each other, and likewise string/bytes can be treated in the same way as each other, and all message types (except the well-known wrapper types) can be treated the same way as each other too.
  • We can't break existing code: code that doesn't currently throw an exception mustn't start throwing an exception. (It would probably be okay for code that currently throws to stop throwing though.)
  • My understanding is that the intention is that making an existing proto3 field optional should not be a breaking change. It's okay for removing the optional keyword to be a breaking change though, and indeed it is. (Making a field optional adds field presence members.)
  • We should start by considering the "regular" API surface for access. We could potentially consider the reflection API separately, but that's probably lower impact anyway.

The last of these bullets means we can't change the compile-time type of any properties based on whether the field is optional or not. So we can't represent an optional int32 field with int? for example. That means the null value is irrelevant for numeric fields.

Message-type fields are already nullable (the value is null by default, and you can set it to null), and that's the way it's always been (in the modern C# implementation starting in 2015, rather than the legacy implementation). So I don't think any work is needed there, for this issue. (There may be further consideration required in terms of what difference, if any, the optional keyword makes for message-type fields.)

That leaves just string and bytes fields. These always have a non-null value. In other words, if we have a message of:

message TestMessage {
  string text1 = 1;
  optional string text2 = 2;
}

... then the generated TestMessage.Text1 and TestMessage.Text2 properties will always be non-null, regardless of whether they have been ever populated or cleared. The second bullet point means they have to continue to always be non-null, to avoid breaking users.

We could allow the property to be set with a null value for optional fields, which would be equivalent to clearing it. So:

var message = new TestMessage();
message.Text1 = null; // Throws now and would continue to throw
message.Text2 = null; // Throws now, would not throw after this change
Console.WriteLine(message.Text2 == ""); // Would still print true after this change

The fact that after setting the property to null, fetching it still returns an empty string feels really surprising to me. I suspect that the asymmetry would confuse users even more than the current inability to set the property with a null value.

Now of course we could consider abandoning the "can't break existing users" axiom above, but that's a much bigger, long-term change. I suspect it's more likely that if we ever create a separate C# generator/support-library for immutable messages (see #9016) we would think about nullity again from scratch for that implementation.

@LeonarddeR
Copy link
Author

Here's my short analysis of the situation, taking into account where we already are (rather than where we might be in an ideal world). The TL;DR is that I can see one change we could make, but I don't propose we actually make it.

Wow, that's very helpful and really improves understanding for me.

The fact that after setting the property to null, fetching it still returns an empty string feels really surprising to me. I suspect that the asymmetry would confuse users even more than the current inability to set the property with a null value.

This makes much sense and I can follow your decision here.
As a flip side, building further on your example:

String someNullString = null;
var message = new TestMessage();
Console.WriteLine(message.HasText2); // Is false
message.Text2 = someNullString ?? String.Empty; // Ensure we don't assign null
Console.WriteLine(message.HasText2); // Is true, as far as I understand

This still breaks projection. In a normal world, I can check HasText2 before assigning, but in a projection, that's not going to work, it is either assign or do not assign. Therefore it is pretty difficult to compose a message right from a database model using projection when the db model contains null strings and the protobuf message has an optional equivalent.

@jskeet
Copy link
Contributor

jskeet commented Jul 15, 2022

Glad that helped, @LeonarddeR. Going on:

This still breaks projection.

Please clarify exactly what you mean by "projection". It's the kind of term that's used in a wide range of ways, and I want to make sure we don't talk past each other.

From your example, I can imagine either:

if (someNullString is string)
{
    message.Text2 = someNullString;
}

or, if you're not mapping to a new message:

if (someNullString is string)
{
    message.Text2 = someNullString;
}
else
{
    message.ClearText2();
}

One option we could explore is introducing a new method:

message.SetOrClearText2(someNullString);

Using a method rather than a property makes the "I called it with a null value, but the property is still an empty string" significantly less confusing IMO.

The downside is that it increases the API surface significantly. I'm on the fence about it personally at the moment, erring on the side of caution (i.e. not making a change we can't undo). I'd be interested to hear your thoughts though.

@LeonarddeR
Copy link
Author

I created an example:
ProjectionExample.zip
There is also this tutorial about projection in EF Core.

In short, when using projection, it is not possible to deal with optional values as you either have to call the setter of every property you want to set or leave out the property entirely.
I know that there is alternative support for nullable types, but they feel as workarounds to me. Do they work with other languages at all?

@jskeet
Copy link
Contributor

jskeet commented Jul 20, 2022

I know that there is alternative support for nullable types, but they feel as workarounds to me. Do they work with other languages at all?

I don't know what you mean by "do they work with other languages at all" - as far as I'm aware, other languages don't have special handling for the wrapper types in the generator... the types "work" but the generated code just refers to them as normal. (There's special handling in JSON parsing and formatting though.) That was the case when I last looked, but things may have changed since then, of course.

It does feel to me like this is one scenario where there's just an impedance mismatch between Protobuf and EF Core, and I don't want to break existing Protobuf users for the sake of removing that.

@JohnGalt1717
Copy link

It does feel to me like this is one scenario where there's just an impedance mismatch between Protobuf and EF Core, and I don't want to break existing Protobuf users for the sake of removing that.

It's an impedance mismatch between Protobuf and C#. EF Core is just a single example.

But more so, Protobuf has an impedance mismatch with all up-to-date programming languages because it breaks best practices coding (nullable types) that have eliminated the most often seen bug in coding. By not supporting this properly, you're turning back the clock and causing an entire new set of problems, and in specific the Dart and C# version is even worse because you can't even set a field to null ever.

It would be a passable hack to allow setting null on a nullable property but not use nullable types, but better would be a property in the csproj file that allows you to opt in to optional fields generating nullable typed fields for ALL types. Other languages then can do whatever other languages want to do and people using C# that want the way it is, which is problematic from the above example but dozens of others, can keep it the way it is.

I'd argue that Dart also needs the same flag, as does every other language that supports nullable types.

This solves the entire problem, because optional still means optional as defined in the spec, and all languages don't change behavior unless you opt in. Once you get telemetry on the people that use the flag versus those that don't, assuming that you take into account the people that don't know it exists, I think you'll find in very short order you'll be able to phase out the flag and make it the default and invert the flag for the legacy way of doing so.

The other alternative is add to proto3 modifier for "nullable" and treat it the way optional should have always been treated: the value is optional, if not entered, it's null, and in all languages that support nullables will generate nullable types, and in all languages that don't, the value will be set to null and you'll have to check to see if it is.

This would eliminate the mismatch between protobuf and all current languages that handle nulls properly, AND doesn't break anything in the past AND brings protobuf in line with basically all serializers in all languages which means it becomes far more adoptable.

@LeonarddeR
Copy link
Author

I like the first proposal from @JohnGalt1717

Note that Strings still need special handling. Therefore the opt in should only work if nullable reference types are on. If not, there's no way to distinguish between string and optional string. having said that, I guess nullable reference type support in C# isn't at all implemented for the code generator, right?

@jskeet
Copy link
Contributor

jskeet commented Jul 20, 2022

I think we've reached the end of fruitful discussion at the moment. Allowing a generator option would effectively pass on a lot of complexity to the generator, and the support library and anyone using a protobuf generated library (as they may well have to deal with different semantics for different libraries).

If we start work on a new major version, where backward compatibility with the current generated code isn't an issue, I promise we'll revisit this - but until that point, I don't have the time even to research and plan what I'd consider to be the most appropriate greenfield generated code (which still needs to bear in mind all kinds of constraints for non-C# use cases).

@JohnGalt1717
Copy link

@jskeet Then Protobuf and GRPC is not viable in any real-world project for even simple crud forms entry and submission to an API because as currently written this adds endless additional code and logic that is ripe for bugs and bloat. Literally every developer that I've talked to that has any exposure to GRPC/Protobuf either complains about all of the noise code because of this or refuses to use it because of this reason. As it stands, I can't create a form using a DTO and submit directly as an example. I have to have a form object and then a translation layer that converts that to the protobuf DTO and works around this for any fields that allow users to not enter a value. That is a MASSIVE waste in productivity, and all of that translation code means bugs.

My flag creates a fork in the code. It would be completely separate and would generate nullable typed code in all cases. It would not break any other implementations because as far as I'm aware, only the .NET one enforces never putting nulls in fields in Protobuf. The rest allow you to set null if nullable types is not enabled and the field is optional (Dart has this problem catastrophically and blocking now that nullable types is the default so it will throw errors when assigning null to non-nullable fields just like C#)

And for V4, the separate code that was under the flag could be used going forward as it would be the most compatible with the new standard written as it should have always been written (presumably).

This is the #1 blocker for any project to use GRPC/Protobuf as soon as anyone dives into it deeply. It was raised in V2, was outright ignored and derided for V3 (and then partially reversed although optional is largely useless as it stands now) and now being delayed to V4 for any substantive fix. Either V4 needs to be accelerated to aggressively fix this, the flag needs to be added to all languages with nullable types, or Protobuf is blocked from serious usage.

@jskeet
Copy link
Contributor

jskeet commented Jul 20, 2022

@JohnGalt1717: I'm certainly not trying to persuade you to use Protobuf. As I said before, I think we've reached the end of fruitful discussion.

@reitowo
Copy link

reitowo commented May 22, 2023

It is ridiculous that we need to check for null for strings that otherwise introduce an exception. Why it can't just set HasXXX to false when set null to XXX (optional)?

@jskeet
Copy link
Contributor

jskeet commented May 22, 2023

@cnSchwarzer: I'm afraid I don't follow the scenario you're talking about at all. Given the range of topics in this issue, I would suggest you create a new issue with a very precise description of what you're seeing. (Ideally, a proto file and sample C# code using it.)

(I'm guessing you're setting a property to null, and it's throwing ArgumentNullException when you wouldn't expect it to, but it's not fully clear. Note that I don't expect to change that behavior, as it would be very odd to be able to set a property to null without it then returning null when fetched - and that change would be very, very breaking.)

@reitowo
Copy link

reitowo commented May 22, 2023

@cnSchwarzer: I'm afraid I don't follow the scenario you're talking about at all. Given the range of topics in this issue, I would suggest you create a new issue with a very precise description of what you're seeing. (Ideally, a proto file and sample C# code using it.)

(I'm guessing you're setting a property to null, and it's throwing ArgumentNullException when you wouldn't expect it to, but it's not fully clear. Note that I don't expect to change that behavior, as it would be very odd to be able to set a property to null without it then returning null when fetched - and that change would be very, very breaking.)

Sorry for being rude and not expressing myself well. I know it is very breaking, but consider this situation:

array.Select(a => new SomeGrpcObject {
    Name = a.Name
});

Which now we have to do a.Name ?? string.Empty for all reference types like string.

I think this behaviour is understandable when this field is not optional: string name = 1, but when it comes to optional string name = 1 it is fully nullable type, and we should use HasName to determine whether the message contains this field.

Expect behavior: Name = null will set Name to null without exception, and also set HasName to false internally, when fetched, the Name == null is equal to HasName == false, and I can't see why it is odd?

Moreover, If the name is not optional, an exception should be thrown because this field is not set, and if is optional, it should works like I said, as it makes no difference because it is a reference type, its default value is null.

@jskeet
Copy link
Contributor

jskeet commented May 22, 2023

Expect behavior: Name = null will set Name to null without exception, and also set HasName to false internally, when fetched, the Name == null is equal to HasName == false, and I can't see why it is odd?

Because that's never the way it's worked before - when you fetch Name, it will never be null - which means if it starts being null, that's a breaking change.

If we'd designed all of the generated code knowing everything we'd end up wanting to do, including the optional feature (which was introduced in proto3 much later) it's possible (but not certain) that we'd have done various things differently. It's worth noting that the way it's designed at the moment, a field can go from default behavior to optional without that being a breaking change for users - because the type doesn't change, and it will still never be null. That can be a pretty significant benefit. (The other way round is a breaking change because the HasXyz property would be removed.)

Fundamentally, the answer is the same as I've given in various places in this issue: the change you're describing is a significant breaking change. We may consider that sort of change for a new major version, but not until then.

I hope you'll agree now that while you may not like the current behavior, it's not really "ridiculous" after all.

@reitowo
Copy link

reitowo commented May 22, 2023

Yeah, I mentioned ridiculous because it will introduce much more unnessesary code like:

array.Select(a => {
    var ret = new SomeGrpcObject {
        Xyz = a.Xyz 
    };
    if (a.Name != null)
        ret.Name = a.Name;
    return ret;
});

It is sure not ridiculous, but ugly if I may say so because it breaks the fluent code writing.

@jskeet
Copy link
Contributor

jskeet commented May 22, 2023

I agree it's inconvenient - but I don't think we can fix that inconvenience without breaking things.

Of course, if you don't actually care about the presence bit, you could use new SomeGrpcObject { Xyz = a.Xyz, Name = ret.Name ?? "" }.

@muratyaman
Copy link

this is a fundamental architectural design flaw, not expected from a big company. messagepack seems like a good alternative: https://msgpack.org/index.html

@reitowo
Copy link

reitowo commented Dec 16, 2023

Can you bump a major version to break change, and redesign using modern .NET? If you can't I would plan to write my own lib.

@jskeet
Copy link
Contributor

jskeet commented Dec 16, 2023

If we do create a new major version, we'd look at this as one potential change. But we're not going to do it just for this. (The cost of a new major version is going to be very significant to the whole community, due to the number of things that depend on protobuf.)

@chase-miller
Copy link

chase-miller commented Apr 5, 2024

Quite some time later, but I find working with protobuf-generated c# objects painful for this reason. I have code like @cnSchwarzer mentioned here all over the place, and I'd argue that it makes code harder to reason about (and certainly more burdensome to write).

@cnSchwarzer did you ever write your own lib?

Edit: I just realized that the google-provided wrappers do what I'm looking for. Doesn't really solve the underlying issue (in cases where you have to use optional), but is a great workaround.

@reitowo
Copy link

reitowo commented Apr 6, 2024

@cnSchwarzer did you ever write your own lib?

Unforturnately no, this thing does very tight integration with Visual Studio and I found not that worty to spend time implementing another one.
Probably we can fork some CodeGenerator and change the behavior of generating optional fields.

Anyway, I still stand by my point, not using optional with C#'s ? is definetly a design flaw and should be fixed ASAP in next major version.

@git-torrent
Copy link

git-torrent commented May 14, 2024

Is edition 2023 enough next major version for considering this issue?

@jskeet
Copy link
Contributor

jskeet commented May 14, 2024

Is edition 2023 enough next major version for considering this issue?

No. We've implemented edition 2023 in C# without any breaking changes.

(To remove any doubt: taking a major version is going to be a huge amount of work, not just for Google but for MS as well, given all the gRPC templates etc. There has to be a really good reason to do so.)

@JohnGalt1717
Copy link

Nullable types is a really good reason to do so.

Like earth shatteringly good reason.

@reitowo
Copy link

reitowo commented May 14, 2024

How about: Bad API design?

@git-torrent
Copy link

git-torrent commented May 15, 2024

(To remove any doubt: taking a major version is going to be a huge amount of work, not just for Google but for MS as well, given all the gRPC templates etc. There has to be a really good reason to do so.)

I still struggle to see what breaking change is required for adding an option to C# proto compiler, something like "emit_defaults" in Go protojson formatter. It would turn this

  public double DoubleValue {
    get { if ((_hasBits0 & 1) != 0) { return doubleValue_; } else { return DoubleValueDefaultValue; } }
    set {
      _hasBits0 |= 1;
      doubleValue_ = value;
    }
  }

into this:

  public double? DoubleValue {
    get { if ((_hasBits0 & 1) != 0) { return doubleValue_; } else { return null } }
    set {
      if ( value.HasValue ) {
       _hasBits0 &= ~1;
      } else {
        _hasBits0 |= 1;
        doubleValue_ = value;
      }
  }

or this:

  public string StringValue {
    get { return stringValue_ ?? StringValueDefaultValue; }
    set {
      stringValue_ = pb::ProtoPreconditions.CheckNotNull(value, "value");
    }
  }

into:

  public string StringValue {
    get { return stringValue_}
    set {
      stringValue_ = value; // nullity condition relaxed
    }
  }

You would make many people happy and you can still keep the current "dialect" as default.

@jskeet
Copy link
Contributor

jskeet commented May 16, 2024

@git-torrent: It would be possible, yes. We've tried to keep the command-line options less semantically-meaningful than that usually. (Reasoning: if two people generate code from the same proto, it's odd if code using the result compiles for one and doesn't for the other.) It would make things harder in the reflection parts of the library, but it would at least be feasible. (I would say that making the library harder to maintain is not appealing - we have few resources for maintaining protobuf on .NET as it is, without making life harder for ourselves.)

I'll mention it to folks on the protobuf team and see whether the policy on command-line options is as it was before.

@jskeet
Copy link
Contributor

jskeet commented May 16, 2024

Another possibility that I've raised with the protobuf folks just now is having a field descriptor extension for this - that would avoid the problem of "two people generating the same proto getting different results", and make the runtime reflection work easier. I'll post a reply with whatever I hear back.

@jskeet
Copy link
Contributor

jskeet commented May 20, 2024

Thanks for the comments, everyone. I've discussed this with the protobuf team, and in doing so gained new insight into how presence is expected to work. The experience of most C# developers with protobuf is with proto3, which uses implicit presence for primitive fields other than those with the optional keyword (which introduces explicit presence). In proto2, explicit presence was the default - and that's also the case in Edition 2023 (see FeatureSetDescriptor for full defaults). Importantly, the expectation is that even when field presence is explicit, most usage will still not care - which means the generated code should make access to the value (whether present or not) as simple as possible. This also allows for custom default values, which would be hard to achieve in the nullable design.

Effectively, this means that "protobuf explicit field presence" and "C# nullable value types" sound the same on paper, but have different use cases, and we don't want to use the latter to model the former. The protobuf team's experience with trying to use a nullable-like model in other languages has been that it doesn't work out in practice. As such, we don't plan to implement this feature request. I understand this is not the result most of the subscribers to this issue are looking for, and I don't expect everyone to be convinced by this line of reasoning, but this is the path we're taking.

@reitowo
Copy link

reitowo commented May 20, 2024

So, why are we throwing exceptions when setting null to non-primitive types? Can you at least remove that null check when optional is present? It is so wierd that we have bunch of _hasBits0 but still throw exceptions when we just don't want to set the value, "implicit presence".

@jskeet
Copy link
Contributor

jskeet commented May 20, 2024

So, why are we throwing exceptions when setting null to non-primitive types?

To avoid inconsistency. (This was something I remember considering a very long time ago. I doubt that I could find the design notes now.) I think it would be odd to have:

message.SomeStringField = null;
Console.WriteLine(message.SomeStringField.Length); // No exception, just prints 0

I think that would be odd regardless of whether presence is explicit or implicit.

This is basically what I said earlier:

Note that I don't expect to change that behavior, as it would be very odd to be able to set a property to null without it then returning null when fetched - and that change would be very, very breaking

I think this is one of those situations where reasonable people can disagree on the best design.

Note that for message types, where setting a property to null is allowed, you get null back for unset fields too - so for any given property, it's consistent. (It means that message type handling is inconsistent with String/ByteString handling, but in a way that I believe ends up giving the desired outcome in most cases.)

@reitowo
Copy link

reitowo commented May 20, 2024

Why not just letting set null results in clearing _hasBitForStringField? and returns null (or StringFieldDefaultValue) when this bit is not present? It is not odd if the default value could be null, and if a default value is set the user should be expected to have a value even it was set null just line above. It is just same as clearing bit method but can simplify some code. It is not even breaking, as setting null, just skip the field being serialized.

@jskeet
Copy link
Contributor

jskeet commented May 20, 2024

It would be very breaking for unset string values to start returning null instead of the empty string.

@reitowo
Copy link

reitowo commented May 20, 2024

Anyway, I think setting null could be an alias for clearing the bit, it is truly odd to not returning null but it is not breaking, just expect some missing exception while it make no harm. I think it is acceptable to me if well documented, as it can make code much more fluent.

@jskeet
Copy link
Contributor

jskeet commented May 20, 2024

I disagree. The documented behaviour is that it throws is the value is null - so not throwing an exception can be used to infer that the value is not null, leading to a later change in behaviour.

Even if we were to change this in a major version, I definitely wouldn't do so in a minor. (And I don't think I would want to do it even in a major, due to the oddity of the behaviour.)

@JohnGalt1717
Copy link

The issue is that there is already "optional" which should have always been if not entered or explicitly set to null, then null in every language that is nullable aware.

Now you're going to have to add yet another keyword because it was defined badly in a way that no one actually uses to mean something that doesn't even really make sense.

And we're all just asking for the people in charge of protobuf to fix the mistake and then every language implementation to align to it because it's just brutally bad as it stands right now making Protobuf useless in virtually all real world cases because it has no concept of the most basic of concepts: (i.e. character 0 in ASCII !!!) null.

@googleberg
Copy link
Member

As @jskeet indictated, we know this is not the decision that will make folks happy, but it is the decision that aligns with current design and prioritization/staffing considerations. If there is a major redesign in the future, we will definitely use this discussion as an input.

As this issue has been discussed for > 2 years and the conversation seems to be looping back on itself, I'll be locking it here.

Thanks Everyone!

@googleberg googleberg closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
@protocolbuffers protocolbuffers locked as resolved and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests