Fix recursive type segfault in hiltic
#1881
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1867
tl;dr: I went back and forth but went with the almost-too-simple solution for this one, but I'm open to suggestions :)
There are two different segfaults that can happen, one from something like
Data = Data
, the other from likeData = Data&
. The first is solved by adding arecursion_depth
to the resolved type (which is just a magic 1000 limit but having a way to configure that would be nice, I just didn't find any). The second is solved with a cycle detection in the unifier (approximating an "occurs check")Note that this just disallows both cases (so
type Data = Data&
will error, not be accepted and properly generate code). Adding support to allow containers with recursive types would be a fair bit more work that should probably come after this, in my opinion. The crux there is that type definitions just end up being "find and replace" aliases in codegen, which will just loop infinitely. I think the solution would be to create a new struct or something in the C++ generated code and use its incomplete type, but that seemed a bit outside the scope here. Was that behavior ever allowed? Because it seems pretty deeply engrained that a type would never reference itself.FWIW, this solution is a bit mediocre. A lot of weird problems arose with other solutions. It would be nice to make
Name::resolvedType
return a result (since now there is a "fail state") but the number of changes for that is pretty substantial and probably not really worth it. So instead we get an iffy error message inresolver.cc
because I don't want to assume the only fail state is hitting the recursion limit.Then putting the cycle detection in the unifier is weird since there were two separate places that would segfault (initially the unifier, but if that's "fixed" codegen also segfaults). A separate validation would make sense, but since the fix slots in neatly to act like an occurs check, it seems fine there. Then we don't need to traverse the tree in the somewhat unique way required to get the infinite recursion - which the type unifier will do when following names.
but.. open to any other suggestion, this is mainly what worked in the easiest way with the fewest drawbacks, in my opinion.