Skip to content

refactor: add null-safety API, rename unsafe function #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Yoorkin
Copy link
Contributor

@Yoorkin Yoorkin commented Apr 18, 2025

  • deprecate Optional::get_exn, add Optional::unsafe_get, Optional::unwrap
  • deprecate Nullable::get_exn, add Nullable::unsafe_get, Nullable::unwrap
  • deprecateValue::cast, add Value::into and Value::unsafe_into
  • rename Value::cast_from to Value::from

Copy link

Unsafe type casting should not be the default behavior

Category
Correctness
Code Snippet
pub fn Value::cast[T](self : Value) -> T = "%identity"
Recommendation
Remove the unsafe cast function and only retain the new safer alternatives into and unsafe_into
Reasoning
The cast function performs unchecked type conversion which can lead to runtime errors. The new into function with null/undefined checks is safer and should be preferred.

Inconsistent deprecation warnings and documentation

Category
Maintainability
Code Snippet
pub fn Nullable::get_exn[T](self : Nullable[T]) -> T = "%identity"
Recommendation
Add clear migration guidance in deprecation messages, e.g.: #deprecated("get_exn is unsafe - use unwrap or unsafe_get based on your needs")
Reasoning
Some deprecated functions lack clear guidance on which alternative to use. Consistent and helpful deprecation messages make migration easier.

Unnecessary double checking in Value::into implementation

Category
Performance
Code Snippet
pub fn Value::into[T](self : Value) -> T {
if self.is_null() { abort(...) }
if self.is_undefined() { abort(...) }
self.unsafe_into()
}
Recommendation
Consider combining the null and undefined checks: if self.is_null() || self.is_undefined() { abort(...) }
Reasoning
The current implementation requires two separate JS interop calls. A single combined check would be more efficient.

@Yoorkin Yoorkin requested a review from rami3l April 18, 2025 06:15
@Yoorkin Yoorkin marked this pull request as draft April 18, 2025 06:16
src/js/value.mbt Outdated
pub fn Value::cast[T](self : Value) -> T = "%identity"

///| Create a `Value` from any type, without checking for null or undefined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you wish to enforce the invariant that a Value cannot be null or undefined. If that is the case, you may wish to add caveats in docstrings for both the Value type and these two methods.

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.

2 participants