Harden exnref literals

[dcodeIO]

Outsourced safety guards for exnref Literals I came up with in #3084.

• Make Literal::type immutable to guarantee that we do not lose track of Literal::exn by changing the literal's type
• Add an assert to guarantee that we don't create exnref literals without an ExceptionPackage (for now)
• Enforce rvalue reference when creating an exnref Literal from a std::unique_ptr<ExceptionPackage>, avoiding a redundant copy by means of requiring std::move

[tlively]

Seems fine to me, but we should still aim to switch to using std::variant at some point.

[dcodeIO]

(will still take me a while to get used to this 😄)

[aheejin]

I don't understand what this is about?

[dcodeIO]

Do you mean the PR in general or my last comment?

[aheejin]

In general. I think it is better to make a PR description self-contained and understandable without reading another big PR and to other people who didn't review that big PR.

[dcodeIO]

Sorry again, I thought the description is fine. To give some context:

• Make Literal::type immutable to guarantee that we do not lose track of Literal::exn by changing the literal's type

Other code sets Literal::type, which is dangerous since the literal may contain a std::unique_ptr<ExceptionPackage> exn that we can only recognize while the literal's type is exnref. So I made Literal::type immutable to prevent other code from changing the type, so it must instead do something like myLiteral = Literal(newType) which will do the right thing by calling the destructor of exn via copy assignment.

• Add an assert to guarantee that we don't create exnref literals without an ExceptionPackage (for now)

Previously one could construct Literal(Type(Type::exnref)) leading to an uninitialized std::unique_ptr<ExceptionPackage> exn, which is again dangerous since it may lead to a nullptr dereference. While we do not do this anywhere intentionally, I figured that it is best to add an assert to guarantee that we can't.

• Enforce rvalue reference when creating an exnref Literal from a std::unique_ptr, avoiding a redundant copy by means of requiring std::move

Previously the signature of makeExnref and the Literal(std::unqiue_ptr<ExceptionPackage>) constructor took the unique ptr by value, yet used std::move to move it. While I don't know for sure, this looked dangerous (if not dangerous, then like a redundant copy), so I made the signature harder to use in a wrong way by enforcing an rvalue reference by appending &&. The compiler will now complain if the method or constructor is not called with an rvalue reference, i.e. the argument is std::moved.

While I can't undo merging this already, does this better explain the changes?

[aheejin]

• Add an assert to guarantee that we don't create exnref literals without an ExceptionPackage (for now)

Previously one could construct Literal(Type(Type::exnref)) leading to an uninitialized std::unique_ptr<ExceptionPackage> exn, which is again dangerous since it may lead to a nullptr dereference. While we do not do this anywhere intentionally, I figured that it is best to add an assert to guarantee that we can't.

exnref is nullable. There should be a way to create exnref with a null or empty unique_ptr.

• Enforce rvalue reference when creating an exnref Literal from a std::unique_ptr, avoiding a redundant copy by means of requiring std::move

Previously the signature of makeExnref and the Literal(std::unqiue_ptr<ExceptionPackage>) constructor took the unique ptr by value, yet used std::move to move it. While I don't know for sure, this looked dangerous (if not dangerous, then like a redundant copy), so I made the signature harder to use in a wrong way by enforcing an rvalue reference by appending &&. The compiler will now complain if the method or constructor is not called with an rvalue reference, i.e. the argument is std::moved.

This doesn't matter. unique_ptr cannot be copied anyway, so the user of this signature is forced to use move anyway. I think this is a choice of style, but passing unique_ptr by value is safe and AFAIK that's how smart pointers are usually passed. Some coding standards also recommend passing smart pointers by value. (I'm not suggesting that we should revert this; just saying that passing unique_ptr by value is safe.)

[dcodeIO]

exnref is nullable. There should be a way to create exnref with a null or empty unique_ptr.

Just not yet, since master is still using nullref for this purpose, that correct? At least my understanding was that null literals are always nullref currently, and that this is going to change with the removal of nullref soon-ish.

This doesn't matter. unique_ptr cannot be copied anyway, so the user of this signature is forced to use move anyway.

Oh, interesting, didn't know. This part of the change is indeed unnecessary then.