cpp:S3230 in templated class

[vincesp (Vincent Spallek)]

The following code yields cpp:S3230:

#include <napi.h>

template <class T, typename P>
class CommonAwaitWorker : public T
{
public:
  explicit CommonAwaitWorker(const P &p)
      : T(p), deferred(T::Env()) {} // Do not use the constructor's initializer list for data member "deferred". Use the in-class initializer instead. sonarlint(cpp:S3230)

private:
  const Napi::Promise::Deferred deferred;
};

class AwaitWorker : public CommonAwaitWorker<Napi::AsyncWorker, Napi::Env>
{
public:
  using CommonAwaitWorker::CommonAwaitWorker;
};

class AwaitWorkerWithProgress : public CommonAwaitWorker<Napi::AsyncProgressWorker<float>, Napi::Function>
{
public:
  using CommonAwaitWorker::CommonAwaitWorker;
};

(Implementation details omitted.)

Napi::Promise::Deferred() is initialized either with Napi::AsyncWorker::Env() or with Napi::AsyncProgressWorker<float>::Env().

Replacing T with a concrete class does not yield cpp:S3230:

class AwaitWorker : public Napi::AsyncWorker
{
public:
  explicit AwaitWorker(const Napi::Env &p)
      : Napi::AsyncWorker(p), deferred(Napi::AsyncWorker::Env()) {}

private:
  const Napi::Promise::Deferred deferred;
};

So I am assuming class T is confusing the rule checker somehow, and this is a false positive.

[Amelie (Amélie Renard)]

Hello @vincesp,

Thank you for your feedback

Indeed, the behavior of the rule seems weird… To better understand what is going on:

• why do you think it is a false positive in the first code snippet instead of a false negative in the second code snippet? Does the following not work (in the first scenario):

private:
  const Napi::Promise::Deferred deferred = T::Env();

• I could not reproduce the second scenario, I am missing information to do so. Would you be able to give me one reproducer per scenario? This way I could understand exactly what is going on.

To generate a reproducer on SonarQube or SonarCloud:

• Search in the analysis log for the full path of the source file for which you want to create a reproducer (for instance, a file that contains a false-positive). You will have to use exactly this name (same case, / or \…)
• Add the reproducer option to the scanner configuration:
  sonar.cfamily.reproducer=“Full path to the .cpp”
• Re-run the scanner to generate a file named sonar-cfamily.reproducer in the project folder.

To generate a reproducer with SonarLint, you can follow the guidelines for Visual Studio or VS Code.

If you think the reproducer file contains private information, let me know, and I’ll send you a private message that will allow you to send the file privately.

Thanks!

Have a nice day,
Amélie

[vincesp (Vincent Spallek)]

Unfortunately, I cannot reproduce this behavior anymore. Even though the code in question is still in place, I might have changed something else about my project so that the false positive is not being reported anymore.

To your question: Yes I think it was a false positive, as there would be no other ways to write this code:

const Napi::Promise::Deferred deferred = T::Env(); // error: could not convert ...

This would not work, as T::Env() would return a Napi::Env which is not assignable to Napi::Promise::Deferred.

Calling the constructor of Napi::Promise::Deferred with T::Env() also would not work as T::Env() is not a type, but a getter function: Napi::AsyncWorker::Env().

const Napi::Promise::Deferred deferred(T::Env()); // error: ‘T::Env’ is not a type

[marco.b]

Hello @vincesp,

I reach the same conclusion as @Amelie.

The second scenario is a False Negative. I’ve reproduced and reduced the issue to a simple example. You can track our progress in fixing this bug here: [CPP-4631] - Jira. Thank you for bringing this topic to us.

The first scenario is a True Positive. You can rewrite your code as follows:

template <class T, typename P>
class CommonAwaitWorker : public T
{
public:
  explicit CommonAwaitWorker(const P &p)
    : T(p) {} 

private:
  // typename is not required but works.
  const Napi::Promise::Deferred deferred = typename T::Env();
  // const Napi::Promise::Deferred deferred = T::Env();
}

You mentioned some compilation errors. The second one is a syntax issue:

const Napi::Promise::Deferred deferred(T::Env());

This is invalid because the compiler thinks you are trying to define a function, and T::Env() is not a type. You can either use the syntax I shared above with = or use {} instead of (). The syntax is explained here: Non-static data members - cppreference.com.

I could not reproduce the other error about type conversion with the example you shared. There is, however, one subtle difference between the two initialization forms. When using the constructor initialization list, : deferred(T::Env()), the compiler applies direct-initialization, which allows implicit conversions from the return type of T::Env() to the type of deferred. When using an in-class initializer, the compiler applies copy-initialization, which does not allow such implicit conversions.

In other words, depending on T and its Env member function, you may have to explicitly write the conversion with:

const Napi::Promise::Deferred deferred = Napi::Promise::Deferred(T::Env());

It’s a bit more verbose, but it should work.

(FWIW, I tested your code sample with the latest revision of the node-addon-api project.)

I hope this helps. Let us know if you have other questions or more feedback.