Reject recursive user-defined types at type-check time - #3546
Reject recursive user-defined types at type-check time#3546Ian Davis (idavis) wants to merge 2 commits into
Conversation
|
The added coverage looks good to me: it already exercises direct recursion, recursion through arrays and callables, mutual recursion, longer cycles, duplicate-field reporting, use-site behavior, acyclic cases, and the diagnostic code. I think it would be worth adding a couple more tests before merging, mainly to cover cases where the cycle is not just a same-namespace/simple-shape recursion. In decreasing order of relevance:
This seems like the most important missing case to me, since it would help ensure the cycle detection is based on resolved item identities rather than accidentally relying on same-namespace or unqualified-name behavior. namespace A {
struct Foo { B : B.Bar }
}
namespace B {
struct Bar { F : A.Foo }
}
namespace A {
struct Foo { F : Int -> (Bool, Bar) }
struct Bar { F : Int -> (Bool, Foo) }
}
The current Tree case covers recursion through a tuple field and an array together. A minimal tuple-only case would isolate tuple traversal more directly. namespace A {
newtype Foo = (Int, Foo);
}
This would verify that diagnostics are emitted for all independent cycles in the same compilation, not just the first recursive component encountered. namespace A {
struct Foo { B : Bar }
struct Bar { F : Foo }
struct Baz { Q : Qux }
struct Qux { Z : Baz }
}
Lower priority, but cheap coverage for repeated type constructors. namespace A {
struct Foo { Children : Foo[][] }
}If we only add one more test, I think the cross-namespace mutual recursion case gives the most additional confidence beyond the existing coverage. |
|
Domingo Morales Lizama (@domorale) Good ideas, I'll add the tests |
Domingo Morales Lizama (domorale)
left a comment
There was a problem hiding this comment.
Thanks for adding the tests. They look good to me.
What this fixes
Closes #911.
Today you can write a Q# type that contains itself:
Nothing stops you at compile time. The compiler accepts it and then falls over later while trying to work with a type that has no bottom — in the worst case the process dies with a stack overflow and no error message at all. That is a bad experience: no file, no line, no explanation.
This PR catches the problem where it belongs, in the type checker, and reports a normal error pointing at the field that closes the loop.
What counts as recursive
A type is rejected if it can reach itself by any path, including indirectly:
struct A { B : A }Aholds aB, andBholds anALeaf=>Mid=>Top=>LeafWrapping does not help. An array of itself, a tuple containing itself, or a function or operation type that takes or returns itself are all rejected.
For the value-carrying cases the reason is size: a Foo that contains a Foo[] or a (Foo, Int) has no finite layout, so there is no bottom to the type.
Function and operation types are a separate case. A
Foo->Foofield does not by itself give Foo infinite size, the signature is just a reference to the type, not an embedded copy of it. The check rejects it anyway, deliberately: it walks every type mentioned by a field, including the inputs and outputs of arrow types, and does not carve out an exception for them. Allowing arrows would mean admitting a narrow, hard-to-explain class of self-referential types that the rest of the pipeline is not built to handle. Rejecting them keeps the rule one sentence long, a type may not reach itself, and leaves the door open to relaxing it later if there is real demand.The error names the field that closes the cycle and states that arrays, tuples, and arrow types still count as recursive, so the reason Foo[] was rejected is visible in the message itself.
What still works
Ordinary nesting is untouched. A type may contain other types as deeply as you like, may use arrays and function types, and may reference types from libraries you depend on — as long as nothing eventually points back at itself.
Breaking change
This is a language-level change: code that compiled before will now fail to compile. In practice such code could not run correctly anyway, it either crashed or misbehaved downstream, so the realistic impact is turning a crash into a readable error. Still, it needs a release note, and I'd flag it for the release/communication owner rather than letting it ship silently.
Behavior notes worth a reviewer's attention