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

Remove optionality from documents that are in unions #520

Merged
merged 2 commits into from
Jun 19, 2021
Merged

Remove optionality from documents that are in unions #520

merged 2 commits into from
Jun 19, 2021

Conversation

jdisanti
Copy link
Collaborator

⚠️ Breaking Change: Unions with Documents will see the inner document type change from Option<Document> to Document.

I don't think it makes sense for Documents to be optional inside of unions. Consider the following:

struct Input {
   union: MyUnion
}
union MyUnion {
    doc: Document,
    notDoc: String,
}

Currently, this generates:

enum MyUnion {
    Doc(Option<Document>),
    NotDoc(String),
}

But when you consider the following:

// Invalid JSON
{"union":undefined}
{"union":{"doc":undefined}}

// Invalid: variant not specified
{"union":{}}

// Valid, but is Document::Null
{"union":{"doc":null}}

// Valid, but uses the other variant
{"union":{"notDoc":"something"}}

It doesn't seem possible to ever have None as the document value in the union.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rcoh
Copy link
Collaborator

rcoh commented Jun 18, 2021

this is definitely an @mtdowling question. If we decide this, it should be brought into NullableIndex

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM, can't tell my left from my right.

@jdisanti
Copy link
Collaborator Author

Turns out the NullableIndex already states that all members in union containers should not be nullable by checking the container type and returning false if it is a union (the default case).

We were previously overriding that with the or in SymbolVisitor for reasons unknown.

I pushed another commit to the PR that simplifies handleOptionality down to just relying on the NullableIndex for this which resolves the issue.

@jdisanti jdisanti merged commit c4ee9f0 into smithy-lang:main Jun 19, 2021
@jdisanti jdisanti deleted the union-document-optionality branch June 19, 2021 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants