Add isValid to nix::Value #10555

jlesquembre · 2024-04-18T18:19:01Z

Motivation

Add a method to check if a value has been initialized. This helps avoid segfaults when calling type().
Useful in the context of the new C API.

Context

Closes #10524

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

roberth

This is a valid problem, but "unset" could be part of the language, whereas this is decidedly about the implementation of the evaluator+bindings.

We have some non-null checks for Value *; perhaps we should also check against uninitialized values in those call sites, and have a helper for that? We could name the helpers after the in and out documented in the parameters.
We might even check that out is uninitialized. We don't want callers to create mutable variables; that would lead to horribly unpredictable outcomes.

roberth · 2024-04-19T13:48:40Z

src/libexpr/value.hh

@@ -23,6 +23,7 @@ class BindingsBuilder;


 typedef enum {
+    tUnset,


Suggested change

tUnset,

tInvalid = 0,

This suggests being part of the implementation more so than the language itself.
For instance, an attribute can be unset, but in that case we return NULL and set NIX_ERR_KEY.
(Also make the 0 explicit)

Actually this might be better

Suggested change

tUnset,

tUninitialized,

roberth · 2024-04-19T13:52:47Z

src/libexpr/value.hh

@@ -294,6 +296,11 @@ public:
        internalType = newType;
    }

+    inline bool isInitialized()


Suggested change

inline bool isInitialized()

/** A value becomes valid when it is initialized. We don't use this in the evaluator; only in the bindings, where the slight extra cost is warranted because of inexperienced callers. */

inline bool isValid()

jlesquembre · 2024-04-20T20:24:32Z

@roberth thanks for your suggestions, I made those changes.

Regarding the extra checks, see: 7835d55
Is that what you had in mind?

roberth

Close to ready for merge, but also have suggestions to make it a bit more ergonomic.

roberth · 2024-04-21T11:07:19Z

src/libexpr-c/nix_api_value.cc

@@ -20,7 +20,7 @@
 #  include "gc_cpp.h"
 #endif

-// Helper function to throw an exception if value is null
+// Helper function to throw an exception if value is null or in an invalid state


This wasn't actually changed.

Suggested change

// Helper function to throw an exception if value is null or in an invalid state

// Helper function to throw an exception if value is null

roberth · 2024-04-21T11:12:33Z

tests/unit/libexpr/nix_api_value.cc


+TEST_F(nix_api_expr_test, nix_value_set_get_float)
+{
    float myDouble = 1.0;


Unrelated, but this freaked me out for a sec. It doesn't seem to serve a purpose.

Suggested change

float myDouble = 1.0;

double myDouble = 1.0;

oops, good catch, addressed it here: f7da544

roberth · 2024-04-21T11:14:04Z

tests/unit/libexpr/nix_api_value.cc

-    ASSERT_DEATH(nix_get_list_byidx(ctx, value, state, 0), "");
-    ASSERT_DEATH(nix_get_list_size(ctx, value), "");


🎉
Was considering to get rid of these, because ASSERT_DEATH is slow.

roberth · 2024-04-21T11:20:08Z

tests/unit/libutil-support/tests/nix_api_util.hh

@@ -33,5 +35,14 @@ protected:
        std::string msg(p, n);
        FAIL() << "nix_err_code(ctx) != NIX_OK, message: " << msg;
    }
+
+    inline void assert_ctx_err()


Not immediately necessary, but it would be good to check the kind of error, and return the message (for when that's relevant to check).

Suggested change

inline void assert_ctx_err()

inline std::string assert_ctx_err(nix_err expected_err)

or const char *

Agreed, but I'd prefer to open a new PR, this one is getting big enough.

roberth · 2024-04-21T11:28:27Z

src/libexpr-c/nix_api_value.cc

        auto & v = check_value_not_null(value);
+        check_value_initialized(v);


check_value_initialized is only used after check_value_not_null, which suggests that we should perhaps have a helper function that does both?
We can then also rephrase it in higher level terms:
check_value_in: check_value_not_null + check_value_initialized
check_value_out: check_value_not_null + check_value_uninitialized

The only other cases I can think of now, where we might want to use check_value_initialized separately is

nullable parameters. For those we could have check_value_in_or_null.

checking multiple values in a buffer, e.g. list or attrset builder. I'm not entirely sure we should do that, because it makes constructing cyclic structures difficult.

Makes sense, I added check_value_in and check_value_out helpers. I'm not happy about having separate versions for const and non-const data, but I can't think of a good solution. link to the code

nullable parameters. For those we could have check_value_in_or_null.

Good point. However, I don't believe we currently have any functions that accept nullable values. If necessary, we can add a helper function for that in the future.

checking multiple values in a buffer, e.g. list or attrset builder. I'm not entirely sure we should do that, because it makes constructing cyclic structures difficult.

In addition, the order of calling nix_init_* and nix_[list,builder]_builder_insert shouldn't matter. I've updated these functions to only check for a null value.

@roberth Kindly pinging you, since I think you might have missed the latest changes. (If not, no worries, just wanted to make sure you're aware, and take your time to review it)

Add a method to check if a value has been initialized. This helps avoid segfaults when calling `type()`. Useful in the context of the new C API. Closes NixOS#10524

jlesquembre requested a review from edolstra as a code owner April 18, 2024 18:19

jlesquembre force-pushed the jl/c-api_check-init branch from ec0d8f6 to 0be906b Compare April 18, 2024 21:06

roberth reviewed Apr 19, 2024

View reviewed changes

roberth added language The Nix expression language; parser, interpreter, primops, evaluation, etc c api Nix as a C library with a stable interface labels Apr 19, 2024

jlesquembre changed the title ~~Add isInitialized to nix::Value~~ Add isValid to nix::Value Apr 20, 2024

roberth reviewed Apr 21, 2024

View reviewed changes

jlesquembre added 6 commits April 21, 2024 22:44

Add isInitialized to nix::Value

5cc4af5

Add a method to check if a value has been initialized. This helps avoid segfaults when calling `type()`. Useful in the context of the new C API. Closes NixOS#10524

nix::Value: Use more descriptive names

9d7dee4

C API: add (un)initialized value checks

ccad6e9

C API: fix test, nix float is a double internally

ff76dd2

C API: add check_value_[in,out] helper functions

8d70db3

C API: source argument to nix_copy_value should be const

6acf02b

jlesquembre force-pushed the jl/c-api_check-init branch from d770b4b to 6acf02b Compare April 21, 2024 20:50

roberth approved these changes May 1, 2024

View reviewed changes

roberth merged commit e17aad2 into NixOS:master May 1, 2024
9 checks passed

jlesquembre deleted the jl/c-api_check-init branch May 2, 2024 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isValid to nix::Value #10555

Add isValid to nix::Value #10555

jlesquembre commented Apr 18, 2024

roberth left a comment

roberth Apr 19, 2024

roberth Apr 19, 2024

roberth Apr 19, 2024 •

edited

This comment was marked as resolved.

jlesquembre commented Apr 20, 2024

roberth left a comment

roberth Apr 21, 2024

roberth Apr 21, 2024

jlesquembre Apr 21, 2024

roberth Apr 21, 2024

roberth Apr 21, 2024

jlesquembre Apr 21, 2024

roberth Apr 21, 2024

jlesquembre Apr 21, 2024 •

edited

jlesquembre Apr 30, 2024

		@@ -23,6 +23,7 @@ class BindingsBuilder;


		typedef enum {
		tUnset,

	inline bool isInitialized()
	/** A value becomes valid when it is initialized. We don't use this in the evaluator; only in the bindings, where the slight extra cost is warranted because of inexperienced callers. */
	inline bool isValid()

	// Helper function to throw an exception if value is null or in an invalid state
	// Helper function to throw an exception if value is null

		ASSERT_DEATH(nix_get_list_byidx(ctx, value, state, 0), "");
		ASSERT_DEATH(nix_get_list_size(ctx, value), "");

	inline void assert_ctx_err()
	inline std::string assert_ctx_err(nix_err expected_err)

		auto & v = check_value_not_null(value);
		check_value_initialized(v);

Add isValid to nix::Value #10555

Add isValid to nix::Value #10555

Conversation

jlesquembre commented Apr 18, 2024

Motivation

Context

Priorities and Process

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth Apr 19, 2024 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

jlesquembre commented Apr 20, 2024

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlesquembre Apr 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth Apr 19, 2024 •

edited

jlesquembre Apr 21, 2024 •

edited