Skip to content

Commit

Permalink
Fix encoding of optional custom types (#3819)
Browse files Browse the repository at this point in the history
  • Loading branch information
bernardnormier authored Nov 30, 2023
1 parent 0e213a6 commit b06a0d0
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 17 deletions.
4 changes: 0 additions & 4 deletions tests/IceRpc.Slice.Tests/StructTests.Slice1.slice
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ mode = Slice1

module IceRpc::Slice::Tests

/*
TODO: re-enable these tests when '#3812' is fixed.

compact struct MyCompactStructWithNullableProxy {
a: int32
i: PingableProxy?
Expand All @@ -19,4 +16,3 @@ compact struct MyCompactStructWithSequenceOfNullableProxies {
compact struct MyCompactStructWithDictionaryOfNullableProxies {
i: Dictionary<int32, PingableProxy?>
}
*/
4 changes: 0 additions & 4 deletions tests/IceRpc.Slice.Tests/StructTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ namespace IceRpc.Slice.Tests;
[Parallelizable(ParallelScope.All)]
public sealed class StructTests
{
/*
TODO: re-enable these tests when 'https://github.com/icerpc/icerpc-csharp/issues/3812' is fixed.
[Test]
public void Decode_slice1_compact_struct_with_nullable_proxy(
[Values("icerpc://localhost/service", null)] string? serviceAddress)
Expand Down Expand Up @@ -294,5 +291,4 @@ public void Encode_slice2_compact_struct_with_dictionary_of_nullable_proxies()
(ref SliceDecoder decoder) => decoder.DecodeProxy<PingableProxy>() as PingableProxy?),
Is.EqualTo(expected.I));
}
*/
}
32 changes: 25 additions & 7 deletions tools/slicec-cs/src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ fn encode_type(
concrete_typeref => {
let value = if type_ref.is_optional && type_ref.is_value_type() {
format!("{param}.Value")
} else if type_ref.is_optional && matches!(type_ref.concrete_type(), Types::CustomType(_)) {
format!("{param} ?? default!")
} else {
param.to_owned()
};
Expand Down Expand Up @@ -169,6 +171,9 @@ fn encode_tagged_type(

let value = if data_type.is_value_type() {
format!("{param}.Value")
} else if matches!(data_type.concrete_type(), Types::CustomType(_)) {
// We don't know if the mapped C# type is a value type or a reference type.
format!("{param} ?? default!")
} else {
param.to_owned()
};
Expand Down Expand Up @@ -351,9 +356,14 @@ fn encode_action_body(
let mut code = CodeBlock::default();
let is_optional = type_ref.is_optional && !is_tagged;

let value = match (is_optional, type_ref.is_value_type()) {
(true, false) => "value!",
(true, true) => "value!.Value",
let value = match (
is_optional,
type_ref.is_value_type(),
matches!(type_ref.concrete_type(), Types::CustomType(_)),
) {
(true, false, false) => "value!",
(true, true, false) => "value!.Value",
(true, false, true) => "(value ?? default!)",
_ => "value",
};

Expand Down Expand Up @@ -394,10 +404,18 @@ fn encode_action_body(
let encoder_extensions_class =
custom_type_ref.escape_scoped_identifier_with_suffix("SliceEncoderExtensions", namespace);
let identifier = custom_type_ref.cs_identifier(Case::Pascal);
write!(
code,
"{encoder_extensions_class}.Encode{identifier}(ref encoder, value)",
)

if type_ref.is_optional && encoding == Encoding::Slice1 {
write!(
code,
"{encoder_extensions_class}.EncodeNullable{identifier}(ref encoder, value)",
)
} else {
write!(
code,
"{encoder_extensions_class}.Encode{identifier}(ref encoder, {value})",
)
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion tools/slicec-cs/src/member_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ pub fn initialize_required_fields(fields: &[&Field], field_type: FieldType) -> C

for field in fields {
let data_type = field.data_type();
// `is_value_type` returns false for custom types since we can't know what type the user mapped it to.
if !data_type.is_optional && !data_type.is_value_type() {
// This is to suppress compiler warnings for non-nullable fields.
// We don't need it for value fields since they are initialized to default.
writeln!(code, "this.{} = default!;", field.field_name(field_type));
}
}
Expand Down
2 changes: 1 addition & 1 deletion tools/slicec-cs/src/slicec_ext/type_ref_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use slicec::grammar::*;
use slicec::utils::code_gen_util::TypeContext;

pub trait TypeRefExt {
/// Is the type a value type (eg. Struct)
/// Is this type known to map to a C# value type?
fn is_value_type(&self) -> bool;

/// The C# mapped type for this type reference.
Expand Down

0 comments on commit b06a0d0

Please sign in to comment.