Mark methods of wrapper classes const

[seishun]

Fixes #870.

Some const methods weren't documented as const, so I think it makes sense to separate this into two commits.

Are there any other methods that should fall under this change?

[seishun]

Help, lint is complaining about lines I didn't touch.

[mhdawson]

Tagged for discussion in the N-API team meeting this week: nodejs/abi-stable-node#418

I still wonder about const even when if effectively mutates internal state and want to get more team discussion to see what people think.

[gabrielschulhof]

I think this PR takes advantage of the fact that napi_value is a plain pointer. If we were to store V8 objects (like v8::Local<v8::Value>) directly inside Napi::* classes instead of the intermediate napi_value objects, which (if any) functions would we be able to mark as const?

For example, I don't think we could render Napi::Object::Set as const.

I think if we mark any of these functions as const we should keep the underlying concepts in mind.

[gabrielschulhof]

This does conceptually modify the environment, because it attaches instance data to it. I know it doesn't modify the state of the Napi::Env object, but it does modify the state of napi_env.

[seishun]

If attaching instance data to the environment is conceptually modifying the Env object, then this modification shouldn't affect copies of the object. But copies of Env share the same environment, similarly to how copies of shared_ptr point to the same object.

[gabrielschulhof]

Same here.

[gabrielschulhof]

As with the above, conceptually, Object::Set does modify the value.

[gabrielschulhof]

Same for Object::Delete.

[gabrielschulhof]

... and the same for these.

[seishun]

If we were to store V8 objects (like v8::Localv8::Value) directly inside Napi::* classes instead of the intermediate napi_value objects, which (if any) functions would we be able to mark as const?

All of them I think, since you can access the non-const inner object from a const Local: https://v8docs.nodesource.com/node-15.0/de/deb/classv8_1_1_local.html#aca5a4236c43b7b3bc530c6bb88cf9272

[mhdawson]

We discussed that "const" should mean. We ended up discussing - https://google.github.io/styleguide/cppguide.html#Use_of_const

Which indicates that methods should only be const if they dont affect the "logical" state of the object.

[mhdawson]

@seishun based on the discussion today we think only a very small subset of those which were being changed should be const. For example GetInstanceData(). If you are still able to work on this could you revise based on the interpretation of const only being applicable if it does not change any "logical" state of the object?

[seishun]

Which indicates that methods should only be const if they dont affect the "logical" state of the object.

Consider this example:

Napi::Object CreateObject(const Napi::CallbackInfo& info) {
  Napi::Env env = info.Env();
  Napi::Object obj1 = Napi::Object::New(env);
  Napi::Object obj2 = obj1; // two independent objects
  obj1.Set(Napi::String::New(env, "msg"), info[0].ToString());
  return obj2;
}

If the obj1.Set call changes the "logical" state of obj1, then you have to explain how the "logical" state of obj2 changes as well.

[mhdawson]

@seishun I don't think I follow your point. In the interpretation we've made const tells you that calling the method won't change the logical state of the object when you call it. Not that the state can't be changed in some other way?

[seishun]

My point is that changing the state of one object doesn't normally change the state of another object. See also #874 (comment).

Another example:

const Napi::Object obj1 = Napi::Object::New(env);
Napi::Object obj2 = obj1;
obj2.Set(Napi::String::New(env, "msg"), info[0].ToString());

I don't know how exactly you define "logical state" but if it's something that can change in a const object then it's not a very useful term.

[mhdawson]

We were not talking about changing some unrelated object.

For example, If the C++ object contains an internal reference to a handle to JavaScript object. Calling a method on the C++ object may not change any of the content in the C++ object but instead changes the content of the JavaScript object that the handle points to. That in turn may result in a different answer when you call another method on C++ object if it gets the result from the related JavaScript object. In that case we were considering the JavaScript object part of the "logical state".

[seishun]

I'm saying that the content of a JavaScript object can't be considered part of the "logical state" of any specific C++ object by any useful definition if that content can be shared by multiple C++ objects and can change in a const object.

[legendecas]

I'm finding the const-ness definition is tricky in node-addon-api. All classes in node-addon-api are wappers on opaque pointers. They don't contain any meaningful state themselves. However, they do have a logical state that they are pointed to.

So on the node-addon-api, I'm finding the const-ness is a guarantee that we are sure the operations don't have side effects either on the wrapper class itself and their underlying logical state, i.e. JavaScript values and operations. As such, I'd find any operation that would cause side effects yet didn't change the wrapper class in node-addon-api to be const is not intuitive and highly misleading.

Although Napi::Object::Set is not changing the wrapper class Napi::Object, but a Napi::Object in concept represents a JavaScript object and Napi::Object::Set is definitely changing the logical state and causing side effects.

For the example described at #874 (comment), I'd find it's a problem on how we define the semantics on assign-and-copy of a node-addon-api wrapper class:

const Napi::ObjectReference obj1 = Napi::ObjectReference::New(Napi::Object::New(info.Env()));
Napi::ObjectReference obj2 = obj1; // assign is disallowed.

[vmoroz]

In the standard C++ library smart pointers have the const qualifier on the get() method arrow -> operator and they return non-const T*. It means that while std::shared_ptr or std::unique_ptr are declared as const, they allow calling any non-const methods for the type they wrap. See:
https://en.cppreference.com/w/cpp/memory/shared_ptr/get
https://en.cppreference.com/w/cpp/memory/shared_ptr/operator*
https://en.cppreference.com/w/cpp/memory/unique_ptr/get
https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*

The only way to prohibit calling non-const methods is to wrap const type inside of the smart pointers: std::shared_ptr<const Foo>. By allowing to call the non-const methods out of const smart pointers, the standard makes it easy to use smart pointers in many contexts including the lambda captures. Without it all lambdas would require to be marked mutable which maybe not the best in the multithreaded environment.

In the Node-API, the wrappers around node_value are essentially smart pointers. Whether the underlying JS object referred by napi_value is allowed to be modified or not is controlled by the JS object logical state. E.g. if it is frozen or sealed. By marking all methods including setters const we make it easier to use these classes in lambdas and other contexts. We do not violate anything unless we have a const method that modifies the field that contains the napi_value itself.

[legendecas]

In the standard C++ library smart pointers have the const qualifier on the get() method arrow -> operator and they return non-const T*. It means that while std::shared_ptr or std::unique_ptr are declared as const, they allow calling any non-const methods for the type they wrap. See:
https://en.cppreference.com/w/cpp/memory/shared_ptr/get
https://en.cppreference.com/w/cpp/memory/shared_ptr/operator*
https://en.cppreference.com/w/cpp/memory/unique_ptr/get
https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*

Please note in node-addon-api the methods named with "set" and "get" are not semantically identical to smart pointers' get and set. They're "set" and "get" for the JavaScript values. The wrapper class in node-addon-api is concrete JavaScript type, like Napi::Value and Napi::Object, but not generic pointers like Napi::UniquePtr<Napi::Object>. They do look like smart pointers on the internal data structure, but semantically they are acting like a plain JavaScript value and provide JavaScript operations on it. Marking these JavaScript operations as const is very misleading.

[seishun]

Marking these JavaScript operations as const is very misleading.

What wrong idea does it give?

[vmoroz]

They do look like smart pointers on the internal data structure, but semantically they are acting like a plain JavaScript value and provide JavaScript operations on it. Marking these JavaScript operations as const is very misleading.

In JavaScript any object variable that is declared as const allows calling all object methods and do any modification to that object unless prohibited by the property and object descriptors. The only part that is restricted by const is the changing the variable itself.

In C++ const keyword for a method says that this method can be called for a const variable - the variable that cannot be changed. One possible optimization that compiler can do to such variable is to put it into a read-only memory. Though, in many practical cases developers try to implement const-correctness instead of true immutable bits. E.g. a const getter method could cache its calculated result and the const-correct code does it by protecting the variable under a mutex. See Herb Sutter's write ups on that topic:

• https://herbsutter.com/2013/05/24/gotw-6a-const-correctness-part-1-3/
• https://herbsutter.com/2013/05/28/gotw-6b-solution-const-correctness-part-2/

If we do add the the const keyword to the Node-API methods, then we

• Match the logical semantic of these classes to JavaScript.
• Match the logical semantic of C-based APIs.
• Make classes easier to use when they are passed as const& or captured by lambda. It allows to write more optimized code.
• Do not make C++ compiled code worse because the napi_value is kept unchanged and C++ optimizer can put it in read-only memory if needed.
• Do not need using mutex because the napi_value field is not modified and the most of the underlying C-APIs are single-threaded.

If we do not add const keyword to methods

• We try to mimic the immutability semantic that is not guaranteed and may mislead developers in believing that an object cannot be mutated, while there are many other ways to mutate it from JS or another napi_value pointer.
• Require to copy objects or use const_cast if they are passed as const& or a lambda capture.

While I understand the desire to use the const keyword to indicate that some methods logically do not have side effects in Node-API objects and others do, in practice it is not achievable because even simple operations like getting a pointer to the array buffer may internally cause the array buffer allocation, or calling a method may trigger an error which will be saved in the environment field. That way almost all methods would be non-const. It would make it much difficult to use such wrapper types. It is better to admit that these are only wrapper classes like smart pointers and not the actual JS objects. They should focus only on the immutability of the napi_value pointer itself and allow running any underlying C-based APIs on it without any restrictions by adding the const modifier to methods. The logical immutability of JS objects must be covered by the corresponding JS property and object descriptors.

[legendecas]

I'm very appreciative of the explanation @vmoroz gave.

I gave this another thought and find that the actual reason I don't find it correct is that the original idea of the change is allowing calling into non-const JavaScript engine methods from const c++ functions. The reason we can call into these node-api c-functions from const c++ functions is that they are c interfaces and these c interfaces can not have c++ const marks and c++ simply ignores the reality these c-functions implementation is not const, even if they are implemented in c++ and calling non-const c++ functions, which definitely breaks the assumptions that const c++ functions can only call into const functions.

That is:

(const) user functions -> (const) node-addon-api c++ wrappers -> node-api c interface (the barrier that invalidates the constness check) -> (non-const) node-api c++ implementations -> (non-const) Javascript c++ implementations.

So when considering the fact that const c++ functions can only call into const c++ functions, this behavior already breaks the assumption.

[mhdawson]

We discussed in the meeting again today and @vmoroz is going to find add the text from the spec in terms of what adding const to a method promises.

[vmoroz]

I did some research and asked our Visual C++ discussion group about this issue.
Based on the feedback, the following definitions seems to be relevant regarding the const qualifier for methods:

3.37 [defns.observer]
observer function
class member function that accesses the state of an object of the class but does not alter that state
[Note 1 to entry: Observer functions are specified as const member functions. —end note]

9.5.1 Function definitions - In general [dcl.fct.def.general]/p5
[Note 1 : A cv-qualifier-seq affects the type of this in the body of a member function; see 7.5.2. —end note]

7.5.2 This [expr.prim.this]/p3
If a declaration declares a member function or member function template of a class X, the expression this
is a prvalue of type “pointer to cv-qualifier-seq X” wherever X is the current class between the optional
cv-qualifier-seq and the end of the function-definition, member-declarator, or declarator.

9.2.9.2 The cv-qualifiers [dcl.type.cv]/p4
Any attempt to modify (7.6.19, 7.6.1.6, 7.6.2.3) a const object (6.8.4) during its lifetime (6.7.3) results in
undefined behavior.

The standard does not say about any possible optimizations for const methods. Though the 4.1.2 [intro.abstract]/p5 says "A conforming implementation executing a well-formed program shall produce the same observable behavior as
one of the possible executions of the corresponding instance of the abstract machine with the same program
and the same input."

The example of a program where using const_cast causes an undefined behavior:

struct S1 { 
    void f() const { i = 0; }
    mutable int i;
};
 
struct S2 {
    void f() const { const_cast<S2*>(this)->i = 0; }
    int i;
};
 
const S1 s1{};
const S2 s2{};
 
int main()
{
    s1.f(); // ok
    s2.f(); // crash
}

Visual C++ compiler puts const fields into a read-only memory and any attempt to modify them causes a write access violation.

In our case we do not need to use any const_cast because we do not modify the internal state of classes. The following code compiles and runs without issues when all methods of the CppValue class are const:

// C-based APIs
struct S1 { int i; };
int get_property(S1* s) { return s->i; }
void set_property(S1* s, int value) { s->i = value; }
 
// C++ wrapper for the C-based APIs
struct CppValue {
   int GetProperty() const { return get_property(s); }
   void SetProperty(int value) const { set_property(s, value); }
   S1* s;
};
 
int main()
{
   S1 s1;
   const CppValue c{ &s1 };
   c.SetProperty(5);
   printf("%d\n", c.GetProperty());
}

[vmoroz]

This example shows that function const qualifier has a "shallow" scope - it only affects the class immediate fields. If its fields point to another class instance, then that instance does not have to be const:

#include <cstdio>

// Methods are not const and can modify the class instance state
struct NonConstValue {
   int GetProperty() { return prop; }
   void SetProperty(int value)  { prop = value; }
   int prop;
};

// All methods are const and do not modify this class instance state
struct ConstValue {
   int GetProperty() const { return val->GetProperty(); }
   void SetProperty(int value) const { val->SetProperty(value); }
   NonConstValue* val;
};

int main()
{
   NonConstValue v1;
   const ConstValue c1{ &v1 };
   c1.SetProperty(5);
   printf("%d\n", c1.GetProperty());
}

We can call non-const methods on the NonConstValue* val field as long as it is not declared in the ConstValue class as const NonConstValue* val.

[legendecas]

Thanks for explaining with the examples! I'd believe I don't have any objections to this PR with the great illustration @vmoroz
gave. Though I'm not very comfortable with the semantics of const "set". I'd leave the PR to other members in @nodejs/node-api to give review comments or approvals.

[mhdawson]

I think that example clarifies it.

[gabrielschulhof]

LGTM after rebase.

[mhdawson]

@seishun we talked through this today in the Node-API team meeting. Thanks for being so patient. Everybody seems comfortable with adding const now.

Could you rebase and see if some of the new methods should also have const added and we will work to land it soon after.

[seishun]

see if some of the new methods should also have const added

There was Object::Freeze and Object::Seal, can you think of any other candidates?

[mhdawson]

@seishun we discussed in the meeting and agreed those 2 could have const added as well.

[seishun]

Just in case, those two already have const added.

[mhdawson]

@seishun I'm wondering about the removal of the const iterator as that might affect existing code?

[seishun]

Only if the code names the type explicitly, otherwise I don't think so.

[mhdawson]

@seishun we'll need to discuss some more since, it could break some existing code.

Is the removal necessary or could we get the rest of the changes landed first ?

[seishun]

It's not necessary, the const iterator just doesn't make sense anymore. Feel free to land without that commit.

[mhdawson]

LGTM minus the commit to remove the const operator. Will remove that when I land.

[mhdawson]

Landed as b8449e1 without the commit to remove the const operator. @seishun thanks for your wok on this and your patience.

[ShenHongFei]

This commit breaks the following code:

    void foo (const CallbackInfo& info) {
        const Object obj = info[0].As<Object>();
        obj["xxx"].As<Boolean>();
    }

because the following methods were deleted in this commit

    /// Gets or sets an indexed property or array element.
    PropertyLValue<Value> operator[](Value index  /// Property / element index
    ) const;

    /// Gets a named property.
    MaybeOrValue<Value> operator[](
        const char* utf8name  ///< UTF-8 encoded null-terminated property name
    ) const;

    /// Gets a named property.
    MaybeOrValue<Value> operator[](
        const std::string& utf8name  ///< UTF-8 encoded property name
    ) const;

    /// Gets an indexed property or array element.
    MaybeOrValue<Value> operator[](uint32_t index  ///< Property / element index
    ) const;

[ShenHongFei]

@seishun

[mhdawson]

@ShenHongFei could you open a new issue for this. Have you tried adding those 4 methods back or does that not work due to other changes?

[mhdawson]

@ShenHongFei created new issue here - #1158. You can follow discussion/questions there.

[alejandroclaro]

My two cents on this topic and why I think this was incorrect.

Using const correctness is about side effects and the mutability of an object's visible state.

If, for example, invoking the 'Set' method alters the result of invoking the 'Get' method, then 'Set' must not be qualified with const.

It appears that invoking "Get" does not in any way alter what happens when querying the object state in subsequent calls. Therefore, it has no visible side effects and should therefore be qualified as const.

Use const is contagious and it must be used correctly to not cause semantic problems and lose its purpose. If everything is qualified as const, it is the same or worse than leaving everything without the qualification. At least by leaving everything unqualified, whoever interacts with the object should be forced to mark it as mutable explicitly, and never pass it as const argument (useless), to interact in a context where care must be taken with side effects. This way it's clear that it’s forced to accept that maybe interacting with this will cause side effects and not hiding this fact.