Skip to content

Conversation

@Yoorkin
Copy link
Contributor

@Yoorkin Yoorkin commented Jun 19, 2025

This PR is part of #20.

  • add Optional::unwrap and Nullable::unwrap
  • deprecate Optional::get_exn and Nullable::get_exn

The get_exn function is merely a %identity operation and does not validate input. If misused, it fails silently in some random places, compromising null-safety (or undefined-safety). To prevent potential errors, especially in Rabbit-Tea, the conversion between JavaScript types across the N hierarchy is painful:

let html_element = event
            .target()
            .to_node()
            .to_option()
            .unwrap()
            .to_element()
            .to_option()
            .unwrap()
            .to_html_element()
            .to_option()
            .unwrap()
            .to_html_dialog_element()
            .to_option()
            .unwrap()

I added two unwrap methods to check null or undefined cases.

@peter-jerry-ye-code-review
Copy link

peter-jerry-ye-code-review bot commented Jun 19, 2025

Asymmetric error handling between Optional::unwrap and Nullable::unwrap

Category
Correctness
Code Snippet
fn[T] Optional::unwrap(self : Self[T]) -> T { ... } vs fn[T] Nullable::unwrap(self : Nullable[T]) -> T { ... }
Recommendation
Use consistent error messages between the two implementations:

fn[T] Optional::unwrap(self : Self[T]) -> T {
  if self.is_undefined() {
    abort("Attempted to unwrap an undefined Optional value")
  }
  self.get_exn()
}

fn[T] Nullable::unwrap(self : Nullable[T]) -> T {
  if self.is_null() {
    abort("Attempted to unwrap a null Nullable value") 
  }
  self.get_exn()
}

Reasoning
The error messages should be symmetrical and descriptive to help developers understand which type (Optional vs Nullable) caused the failure

Missing deprecation notice documentation for get_exn

Category
Correctness
Code Snippet
#deprecated("get_exn does not check for undefined values. Use unwrap instead")
Recommendation
Add more context in the deprecation message:

#deprecated("get_exn performs unsafe conversion without validation. Use unwrap() instead which validates the value before conversion.")
**Reasoning**
The current message doesn't fully explain why get_exn is unsafe and what unwrap does differently. A more detailed message helps developers understand the risks and make an informed decision.

</details>
<details>

<summary> Inconsistent use of abort vs panic in error handling </summary>

**Category**
Maintainability
**Code Snippet**
abort("Cannot unwrap...") vs panic()
**Recommendation**
Standardize on abort() for runtime assertions and add more context:
```moonbit
abort("Assertion failed: Cannot unwrap undefined/null value at <location>")

Reasoning
Using different termination mechanisms (abort vs panic) makes the code harder to maintain. Standardizing on abort() with consistent error message format improves debuggability.

@Yoorkin Yoorkin requested a review from rami3l June 19, 2025 05:37
@rami3l rami3l merged commit f786da4 into main Jun 19, 2025
4 checks passed
@rami3l rami3l deleted the unwrap branch June 19, 2025 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants