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

Handle structure with positional trait whose first member is not required. #727

Open
rishav-karanjit opened this issue Nov 19, 2024 · 1 comment
Labels
soundness Bugs that cause the generated code to compute the wrong value or crash

Comments

@rishav-karanjit
Copy link
Member

If I have a structure like this:

operation GetIntegerOutputPosition {
  input: GetIntegerOutputPositionInput
  output: GetIntegerOutputPositionOutput,
}

structure GetIntegerOutputPositionInput {
  value: Integer
}

@positional
structure GetIntegerOutputPositionOutput {
  value: Integer
}

From this model, the Dafny code will be polymorph'ed as

method GetIntegerOutputPosition ( input: GetIntegerOutputPositionInput )
      returns (output: Result<int32, Error>) 

Which is wrong because based on the model the return type should be Option.

To resolve this issue, there are two solutions:

  1. The polymorph should fail with a validation error.
  2. This should be generated as Option but not int32
@rishav-karanjit
Copy link
Member Author

rishav-karanjit commented Nov 19, 2024

From Slack chat:

Rishav Karanjit:
I like to believe positional trait should always have a required trait on its first member and polymorph MUST fail with a validation error and we should add validation check in positional trait class.

Robin Salkeld:
It does sound like a bug, but there's more than one way to address it:
As you suggest, validate that the singleton member of a structure with positional trait on it must have required trait
Allow optional values, but in that case generate Option instead of int32
I lean towards 2 as I don't think there's a strong reason to not support that use case, and that keeps the features nicely orthogonal.
Actually, 1 would be quicker and safer to implement as a stopgap IF we aren't already using the 2 case somewhere in production libraries.

Ryan Emery:
I would go with 1. Because we can change it. More and more I view @ positional as a way to model things to look nicer in the target language .e.g. they are to more closely match an existing interface, not how you would use Smithy for a new API
As such, more constraints seem fine here. If we ever come back and remove this constraint, then I would also remove the 1 value, and really let it model an existing API. But that can be much later things 🙂

Lucas McDonald:
If we go with 1, I'd suggest noting that we should update this comment)

Robin Salkeld:
For the record I don’t think it’s (edit: UN)reasonable to use in new APIs too - avoiding the extra response wrapper object with the tradeoff of not being able to add new members later is very reasonable. There’s precedent in core Smithy too - if you have no output structure or explicitly use Unit, you can’t add a return value later. (edited)

That’s a very heavily loaded comment :) Makes me think we do have optional positional members out there. Do you know the history?

Lucas McDonald
No clue, I just happened to notice it

@rishav-karanjit rishav-karanjit added the soundness Bugs that cause the generated code to compute the wrong value or crash label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
soundness Bugs that cause the generated code to compute the wrong value or crash
Projects
None yet
Development

No branches or pull requests

1 participant