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

Fix dedicated io transform leaving unused shapes #1419

Conversation

milesziemer
Copy link
Contributor

@milesziemer milesziemer commented Sep 22, 2022

This commit changes the way CreateDedicatedInputOutput transform checks for singular references to a shape being used as input or output in an operation. It now checks to make sure the shape is only referenced by the specified operation. This fixes an edge case where a shape was being used as both the input and output for an operation, and would be left unused in the model after the transformation was applied. Two new test cases were added to verify the behavior when a shape is used as both input and output.

Issue #, if available: #1373

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@milesziemer milesziemer requested a review from a team as a code owner September 22, 2022 17:37
output: GetFooOutput
}

structure GetFooOutput {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the naming of this output structure get updated so it's clear that it will be removed in the -after.smithy file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this test is to show that it we don't get a GetFooOperationOutput shape after the transform. Before this change, the transformer would think that because the GetFooOutput shape is being used in two places, it should create a new shape for the GetFoo output, when in reality the GetFoo input is going to be changed and thus it should have kept the GetFooOutput shape and just added the @output trait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a second test that verifies that the unused shape will be removed, I'll leave this open to make sure that looks ok

This commit changes the way `CreateDedicatedInputOutput` transform checks for
singular references to a shape being used as input or output in an operation.
It now checks to make sure the shape is only referenced by the specified
operation. This fixes an edge case where a shape was being used as both the
input and output for an operation, and would be left unused in the model
after the transformation was applied. Two new test cases were added to
verify the behavior when a shape is used as both input and output.
@milesziemer milesziemer force-pushed the fix-create-dedicated-io-transformer branch from faf1d2f to e25db28 Compare September 28, 2022 13:47
@mtdowling mtdowling requested a review from srchase November 29, 2022 18:59
@milesziemer milesziemer merged commit f915ead into smithy-lang:main Dec 9, 2022
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