Skip to content

Implement Value.TypeOf() #104

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement Value.TypeOf() #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4930f78
Implement Value.TypeOf()
stroiman May 20, 2025
1e5c096
Remove unnecessary null check
stroiman May 21, 2025
7f396a7
Use consistent C-style
stroiman May 21, 2025
9fdf60d
Change var assignments to declare + assignment
stroiman Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Add support for ObjectTemplate.MarkAsUndetectable.
- Add `Value.StrictEquals` providing strict equality checks in Go code
- Add support for `Value.TypeOf()` corresponding to JavaScript `typeof` operator.

### Changed

Expand Down
7 changes: 7 additions & 0 deletions errors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <stdlib.h>
#include <sstream>

#include "deps/include/v8-exception.h"
Expand Down Expand Up @@ -48,3 +49,9 @@ RtnError ExceptionError(TryCatch& try_catch, Isolate* iso, Local<Context> ctx) {

return rtn;
}

void ErrorRelease(RtnError err) {
free((void*)err.msg);
free((void*)err.location);
free((void*)err.stack);
}
2 changes: 2 additions & 0 deletions errors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ typedef struct {
RtnError error;
} RtnValue;

void ErrorRelease(RtnError err);

#ifdef __cplusplus
}
#endif
Expand Down
42 changes: 34 additions & 8 deletions value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#include "value.h"
#include <stdlib.h>
#include "context.h"
#include "deps/include/v8-context.h"
#include "deps/include/v8-exception.h"
#include "errors.h"
#include "isolate-macros.h"
#include "utils.h"
#include "value-macros.h"

#define ISOLATE_SCOPE_INTERNAL_CONTEXT(iso) \
Expand All @@ -11,6 +13,29 @@

using namespace v8;

RtnString StringToRtnString(v8::Isolate* iso, Local<String> val) {
RtnString res = {};
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the {} initialization does anything (the original code had {0} with no purpose there).

RtnString and RtnError should have constructors (or default initializers) to ensure values are cleared.

Copy link
Author

Choose a reason for hiding this comment

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

{} zeros the fields. But I don't know the difference between that and {0}.

However, these two types RtnString and RtnError are exposed to Go code, i.e. Go code actually looks up the field - so I don't think you can add constructors? Because then it would be a C++ type, not a C type?

You could make them "anonymous" (don't know the correct term), like m_ctx where Go code just has pointer values it can pass back to C functions - and then add functions to look up values, but that also seems somewhat cumbersome.

Copy link
Owner

Choose a reason for hiding this comment

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

As long as you don't add a vtable, it's just compile-time syntactic sugar for running a function every time a value is created.

Relying on a destructor might be dangerous, since the cgo code wouldn't run it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I what you mean. But this works - but it generates warnings.

struct RtnError {
  const char* msg;
  const char* location;
  const char* stack;
#ifdef __cplusplus
  RtnError() : msg(0), location(0), stack(0) {};
#endif
};

To get "methods" on the type, I changed from typedef struct { ... } RtnError to struct RtnError { }.

This caused compilation errors, both in .h and .go files.

  • In .h files, extern RtnValue SomeFunction(...); had to be changed to extern struct RtnValue SomeFunction(...)
  • In .go files, C.RtnValue had to be changed to C.struct_RtnValue.

I could avoid those changes by adding typedef struct RtnError RtnError; though I probably shouldn't use the same identifier?

The warnings are:

./context.h:80:17: warning: 'RunScript' has C-linkage specified, but returns user-defined type 'RtnValue' which is incompatible with C [-Wreturn-type-c-linkage]

But if I remove the initializer from some function creating a RtnError, all tests are still passing, and if I then remove the constructor, it fails at runtime.

But I had the impression from your comments that there is a simpler way?

Copy link
Owner

@tommie tommie May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I see, this needs to compile in C mode. Of course it does. My bad.

Ok, helper function it is...

Edit The helper function was about another comment... (Does it show there's too much going on for me to keep track. ;)

Copy link
Owner

Choose a reason for hiding this comment

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

But here: still need to add back the zero into the intializer to make it do something. https://stackoverflow.com/a/10828333

Copy link
Author

@stroiman stroiman May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The helper function was about another comment... (Does it show there's too much going on for me to keep track. ;)

Yeah, I know I added a lot in one batch. It was just one day I was like "hey, let me just get all these things I created over time made into isolated PRs".

After this batch, I think I just have two large WIP: ESM and Handlers/Interceptors, but I want to see them work reliably in my own project before considering them merge for PR. But I might consider creating refactor PRs based on some of the discussions regarding helpers/resource management/RAII

res.length = val->Utf8LengthV2(iso);
res.data = (char*)(malloc(res.length));
val->WriteUtf8V2(iso, (char*)res.data, res.length);
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest sticking to either C- or C++-style casts in one function. :)

Copy link
Author

Choose a reason for hiding this comment

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

I agree :)

return res;
}

RtnString ExceptionToRtnString(TryCatch& try_catch,
v8::Isolate* iso,
Local<Context> ctx) {
RtnString res = {};
res.error = ExceptionError(try_catch, iso, ctx);
return res;
}

void RtnStringRelease(RtnString rtnString) {
if (rtnString.data) {
free((void*)rtnString.data);
}
ErrorRelease(rtnString.error);
}

void ValueRelease(ValuePtr ptr) {
if (ptr == nullptr) {
return;
Expand Down Expand Up @@ -223,16 +248,11 @@ double ValueToNumber(ValuePtr ptr) {

RtnString ValueToDetailString(ValuePtr ptr) {
LOCAL_VALUE(ptr);
RtnString rtn = {0};
Local<String> str;
if (!value->ToDetailString(local_ctx).ToLocal(&str)) {
rtn.error = ExceptionError(try_catch, iso, local_ctx);
return rtn;
return ExceptionToRtnString(try_catch, iso, local_ctx);
}
String::Utf8Value ds(iso, str);
rtn.data = CopyString(ds);
rtn.length = ds.length();
return rtn;
return StringToRtnString(iso, str);
}

RtnString ValueToString(ValuePtr ptr) {
Expand Down Expand Up @@ -571,3 +591,9 @@ int ValueStrictEquals(ValuePtr ptr, ValuePtr otherPtr) {
Local<Value> other = otherPtr->ptr.Get(iso);
return value->StrictEquals(other);
}

RtnString ValueTypeOf(ValuePtr ptr) {
LOCAL_VALUE(ptr);
Local<String> result = value->TypeOf(iso);
return StringToRtnString(iso, result);
}
8 changes: 7 additions & 1 deletion value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (v *Value) Object() *Object {
// print their definition.
func (v *Value) String() string {
s := C.ValueToString(v.ptr)
defer C.free(unsafe.Pointer(s.data))
defer C.RtnStringRelease(s)
return C.GoStringN(s.data, C.int(s.length))
}

Expand Down Expand Up @@ -618,3 +618,9 @@ func (v *Value) SharedArrayBufferGetContents() ([]byte, func(), error) {
func (v *Value) StrictEquals(other *Value) bool {
return C.ValueStrictEquals(v.ptr, other.ptr) != 0
}

func (v *Value) TypeOf() string {
s := C.ValueTypeOf(v.ptr)
defer C.RtnStringRelease(s)
return C.GoStringN(s.data, C.int(s.length))
}
3 changes: 3 additions & 0 deletions value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ typedef struct {
} RtnString;

void ValueRelease(ValuePtr ptr);
void RtnStringRelease(RtnString rtnString);
extern RtnString ValueToString(ValuePtr ptr);
extern RtnString ValueTypeOf(ValuePtr ptr);
const uint32_t* ValueToArrayIndex(ValuePtr ptr);
int ValueToBoolean(ValuePtr ptr);
int32_t ValueToInt32(ValuePtr ptr);
Expand Down Expand Up @@ -164,5 +166,6 @@ extern BackingStorePtr SharedArrayBufferGetBackingStore(ValuePtr ptr);

#ifdef __cplusplus
}

#endif
#endif
42 changes: 39 additions & 3 deletions value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,10 @@ func TestValueBigInt(t *testing.T) {
iso := v8.NewIsolate()
defer iso.Dispose()

x, _ := new(big.Int).SetString("36893488147419099136", 10) // larger than a single word size (64bit)
x, _ := new(
big.Int,
).SetString("36893488147419099136", 10)
// larger than a single word size (64bit)

tests := [...]struct {
source string
Expand Down Expand Up @@ -697,7 +700,10 @@ func TestValueIsXXX(t *testing.T) {
t.Fatalf("failed to run script: %v", err)
}
if !tt.assert(val) {
t.Errorf("value is false for %s", runtime.FuncForPC(reflect.ValueOf(tt.assert).Pointer()).Name())
t.Errorf(
"value is false for %s",
runtime.FuncForPC(reflect.ValueOf(tt.assert).Pointer()).Name(),
)
}
})
}
Expand Down Expand Up @@ -814,7 +820,9 @@ func TestValueArrayBufferContents(t *testing.T) {
}
_, _, err = val.SharedArrayBufferGetContents()
if err == nil {
t.Fatalf("Expected an error trying call SharedArrayBufferGetContents on value of incorrect type")
t.Fatalf(
"Expected an error trying call SharedArrayBufferGetContents on value of incorrect type",
)
}
}

Expand Down Expand Up @@ -853,3 +861,31 @@ func TestValueStrictEquals(t *testing.T) {
t.Errorf("Comparing two different functions should not be strict equal")
}
}

func TestValueTypeOf(t *testing.T) {
t.Parallel()
iso := v8.NewIsolate()
defer iso.Dispose()
ctx := v8.NewContext(iso)
defer ctx.Close()

str1, _ := v8.NewValue(iso, "String")
if got := str1.TypeOf(); got != "string" {
t.Errorf(`NewValue("String"): expected string, got %s`, got)
}

str2, _ := ctx.RunScript("'string'", "")
if got := str2.TypeOf(); got != "string" {
t.Errorf("TypeOf('string'): expected string, got %s", got)
}

num1, _ := v8.NewValue(iso, 0.01)
if got := num1.TypeOf(); got != "number" {
t.Errorf(`NewValue(0.01): expected number, got %s`, got)
}

num2, _ := ctx.RunScript("0.01", "")
if got := num2.TypeOf(); got != "number" {
t.Errorf("TypeOf(0.01): expected number, got %s", got)
}
}
Loading