Fix memleaks in work with struct types

[viktormalik]

Using std::shared_ptr to store the inner struct in SizedType causes memleaks when working with recursive types (due to cycles in shared pointers). This PR replaces the std::shared_ptr by a raw pointer.

Fixes #1879.

Checklist

• Language changes are updated in docs/reference_guide.md
• User-visible and non-trivial changes updated in CHANGELOG.md
• The new behaviour is covered by tests

[danobi]

So I see in SemanticAnalyser::visit(Tuple &tuple) I see an unconditional CreateTuple(). Doesn't this cause us to store 10x the values in StructManager::tuples_? If so, anything we can do to avoid that?

[viktormalik]

So I see in SemanticAnalyser::visit(Tuple &tuple) I see an unconditional CreateTuple(). Doesn't this cause us to store 10x the values in StructManager::tuples_? If so, anything we can do to avoid that?

You're right, this is unnecessary. Two solutions come to mind:

1. Only store the type in the first/last iteration of the semantic analyser, which is easy, though I'm not sure if it's correct.
2. Change StructManager::tuples_ into a hash map, then we'd have each tuple type stored only once. The hash could be the string representation of the SizedType (obtained by operator<<), which should be fine for this purpose, I hope.

[danobi]

Option 1 sounds incorrect although I haven't thought about it too hard.

Option 2 sounds reasonable to me. Couple thoughts on option 2:

• Using a string representation as key would work. But maybe also check if we can implement std::hash for SizedType so it could just be a a std::unordered_set. std::unique_ptr already implements std::hash: https://en.cppreference.com/w/cpp/memory/unique_ptr/hash . SizedType being hashable sounds generally useful to have (though I wouldn't go crazy over this to
• Tuples types may change at each pass. We'll still be leaving behind the old, unused tuple types. Out of scope for this PR but I wonder if we can get rid of is_final_pass() altogether. It not only is not guaranteed to work (what if something takes 11 passes to resolve?) but causes subtle issues like this.

[viktormalik]

• Using a string representation as key would work. But maybe also check if we can implement std::hash for SizedType so it could just be a a std::unordered_set. std::unique_ptr already implements std::hash: https://en.cppreference.com/w/cpp/memory/unique_ptr/hash . SizedType being hashable sounds generally useful to have (though I wouldn't go crazy over this to

I implemented std::hash for SizedType so now StructManager::tuples_ is an unordered_set. It seems to work without problems but it definitely needs a detailed review.

• Tuples types may change at each pass. We'll still be leaving behind the old, unused tuple types. Out of scope for this PR but I wonder if we can get rid of is_final_pass() altogether. It not only is not guaranteed to work (what if something takes 11 passes to resolve?) but causes subtle issues like this.

I'm not sure if getting rid of is_final_pass would help here as we leave unused types behind in cases when the type changes between passes, which would still happen.

Still, the approach of running a fixed number of 10 iterations is not very good, but that's for another discussion. We could be able to implement a more "clever" AST pass that would not re-visit nodes for which the state didn't change in 2 successive iterations. Then we could get rid of is_final_pass as we'd just run the semantic analysis until the fixpoint is found.

[danobi]

I'm not sure if getting rid of is_final_pass would help here as we leave unused types behind in cases when the type changes between passes, which would still happen.

So I think is_final_pass() exists b/c maps are not always declared before they are used (needs citation). So in theory, if we could figure out the exact types of all the maps before we figure out the rest of the types, we only need a single pass, as non-maps are declared + defined at the same time.

But I agree, we should discuss this somewhere else.

[fbs]

I originally tagged this for the 0.13 release but I'm hoping to do that release on the first. Should we try and get this in or just leave it for the next release? The leak isn't too bad

[danobi]

I originally tagged this for the 0.13 release but I'm hoping to do that release on the first. Should we try and get this in or just leave it for the next release? The leak isn't too bad

If we can get it in by the 1st that'd be nice. If not, not a huge deal IMO

[viktormalik]

I originally tagged this for the 0.13 release but I'm hoping to do that release on the first. Should we try and get this in or just leave it for the next release? The leak isn't too bad

If we can get it in by the 1st that'd be nice. If not, not a huge deal IMO

Updated based on @danobi's review. Should be ready for merge if we want to get this in.

[danobi]

Discussed on IRC: this PR has potential to cause subtle bugs. So better if we let this get tested through another development cycle to iron out issues.