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

Recursive shape definitions restriction not restrictive enough part 2 #1247

Closed
david-perez opened this issue Jun 1, 2022 · 3 comments · Fixed by #1253 or #1269
Closed

Recursive shape definitions restriction not restrictive enough part 2 #1247

david-perez opened this issue Jun 1, 2022 · 3 comments · Fixed by #1253 or #1269

Comments

@david-perez
Copy link
Contributor

This is a follow-up on #1183.

I guess the phrase "unless one or more members in the path from the container back to itself targets a structure or union shape" needs to be tweaked to mention that the union shape must have more than one member (?).

You can otherwise have recursive unions:

structure AnOperationInputOutput {
    listShape: ListShape
}

list ListShape {
    member: UnionShape
}

union UnionShape {
    member: ListShape
}

But even if you tweaked that sentence, you could still have, more simply:

union RecursiveUnion {
    member: RecursiveUnion
}

So I guess a better way to disallow these is to add another bullet point mentioning that union shapes that refer back to themselves must have more than one member?

@mtdowling
Copy link
Member

Ah good point.

The first example is weird, but seems fine to me. The type would end up in PLs like List<UnionShape>. DynamoDB's AttributeValue has recursive union like this for L, for example. It's also not necessarily infinitely recursive when used because a list is bounded and could even have zero length.

The second example is something we shouldn't allow because it's impossible to provide a value for it. Because unions can have many members that refer to themselves, we would want at least one member of the union to not refer directly to itself in order to allow recursive unions.

mtdowling added a commit that referenced this issue Jun 2, 2022
mtdowling added a commit that referenced this issue Jun 2, 2022
@david-perez
Copy link
Contributor Author

I looked at the PR. I think it only addresses direct recursion. This should also be deemed invalid right?

union RecursiveUnion {
    a: RecursiveUnion2,
}

union RecursiveUnion2 {
    b: RecursiveUnion,
}

@mtdowling
Copy link
Member

Oh yeah, you’re right. We can follow up here to do a similar path traversal we do with structures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants