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

defaults for non-optional mutable members #156

Merged
merged 4 commits into from
Apr 10, 2024
Merged

defaults for non-optional mutable members #156

merged 4 commits into from
Apr 10, 2024

Conversation

snosenzo
Copy link
Collaborator

@snosenzo snosenzo commented Mar 22, 2024

Public-Facing Changes

  • non-optional fields for mutable structs and unions now receive default values

Description

Most of this is done in the DeserializationInfoCache since we generally want to cache default values for complex types.
Primitive fields receive default values per spec. Default values for fixed size arrays are given
Default unions use a default case if one exists, otherwise they use the case selected by the default value of the switch case type.
Default structs are filled with non-optional members' default values.

Reference: https://www.omg.org/spec/DDS-XTypes/1.3/PDF
7.2.2.4.4.4.7
image

FG-6554

@snosenzo snosenzo requested a review from achim-k March 22, 2024 17:16
Copy link
Contributor

@achim-k achim-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main question is, if we can't compute default values at MessageReader instantiation time (when creating the deserialization info cache). This PR seems to compute (or lookup from a map) default values during message deserialization time which could be avoided when pre-computing them.

return deserInfo.defaultValue;
}

return this.#updateFieldDeserInfoWithDefaultValue(deserInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure exactly what this function does, but I don't think you need it. You should be able to just call getFieldDefault recursively if required. Maybe something like this:

  public getFieldDefault(deserInfo: FieldDeserializationInfo): unknown {
    if (deserInfo.defaultValue != undefined) {
      return deserInfo.defaultValue;
    }

    if (deserInfo.typeDeserInfo.type === "primitive") {
      deserInfo.defaultValue = PRIMITIVE_DEFAULT_VALUE_GETTERS.get(deserInfo.type);
      return deserInfo.defaultValue;
    } else if (deserInfo.typeDeserInfo.type === "array-primitive") {
      deserInfo.defaultValue = [];
      return deserInfo.defaultValue;
    } else if (deserInfo.typeDeserInfo.type === "struct") {
      const defaultValue: Record<string, unknown> = {};
      for (const field of deserInfo.typeDeserInfo.fields) {
        defaultValue[field.name] = this.getFieldDefault(field);
      }
      deserInfo.defaultValue = defaultValue;
      return deserInfo.defaultValue;
    } else if (deserInfo.typeDeserInfo.type === "union") {
      // not sure if the union case should be handled here. At this point the type of the union should be resolved already? 
    }

    throw new Error();
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a bit verbose, but I wanted to be encapsulate the mutation of the deserInfo in its own function and be clear in the function name that it was updateing the variable. Anyway this is definitely confusing, going to make another pass at it and try to consolidate the complex info handling

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright updated to remove this function. hopefully it's a bit more clear now.

@snosenzo snosenzo requested a review from achim-k April 8, 2024 20:06
]);

export function makeNestedArray(
getValue: () => unknown,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just pass value: unknown here instead of a getter function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use getValue instead of value here because I want to prevent assigning all of the values to the same reference if the value is not a primitive.

@@ -269,6 +382,43 @@ export const PRIMITIVE_ARRAY_DESERIALIZERS = new Map<string, ArrayDeserializer>(
["string", readStringArray],
]);

export const PRIMITIVE_DEFAULT_VALUE_GETTERS = new Map<string, () => unknown>([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can that be a <string, unknown> map instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can, but because this is how the default values are accessed by complex types as well I think it's preferable to keep them consistent.

Comment on lines +286 to +287
// if `structuredClone` is part of the environment, use it to clone the default message
// want to avoid defaultValues having references to the same object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what cases would this be bad?

Copy link
Collaborator Author

@snosenzo snosenzo Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a message contains several instances of a single default value, and the user of the library mutates a single one of those values, then all of the instances will be changed because they point to the same reference. This also includes default values across all messages read by an instance of this parser.
We don't do this in studio, and it is an added cost to instantiate a new instance of this value each time, but I think that's what users of this library will expect and it will save time fighting against potential errors that could come from manipulating (accidentally or otherwise) these objects sharing the same reference.

@snosenzo snosenzo merged commit 52adfbd into main Apr 10, 2024
2 checks passed
@snosenzo snosenzo deleted the optional-defaults branch April 10, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants