Fix #14546 FN uninitMemberVar (in-class initializers but no constructor) (#8274)

Co-authored-by: chrchr-github <noreply@github.com>
Co-authored-by: Daniel Marjamäki <daniel.marjamaki@gmail.com>

Author: 3 people

lib/checkclass.cpp

@@ -116,6 +116,90 @@ CheckClass::CheckClass(const Tokenizer *tokenizer, const Settings *settings, Err
     mSymbolDatabase(tokenizer?tokenizer->getSymbolDatabase():nullptr)
 {}
 
+bool CheckClass::isInitialized(const CheckClass::Usage& usage, FunctionType funcType) const
+{
+    const Variable& var = *usage.var;
+
+    if (usage.assign || usage.init || var.isStatic())
+        return true;
+
+    if (!var.nameToken() || var.nameToken()->isAnonymous())
+        return true;
+
+    if (var.valueType() && var.valueType()->pointer == 0 && var.type() && var.type()->needInitialization == Type::NeedInitialization::False && var.type()->derivedFrom.empty())
+        return true;
+
+    if (var.isConst() && funcType == FunctionType::eOperatorEqual) // We can't set const members in assignment operator
+        return true;
+
+    // Check if this is a class constructor
+    if (!var.isPointer() && !var.isPointerArray() && var.isClass() && funcType == FunctionType::eConstructor) {
+        // Unknown type so assume it is initialized
+        if (!var.type()) {
+            if (var.isStlType() && var.valueType() && var.valueType()->containerTypeToken) {
+                if (var.valueType()->type == ValueType::Type::ITERATOR)
+                {
+                    // needs initialization
+                }
+                else if (var.getTypeName() == "std::array") {
+                    const Token* ctt = var.valueType()->containerTypeToken;
+                    if (!ctt->isStandardType() &&
+                        (!ctt->type() || ctt->type()->needInitialization != Type::NeedInitialization::True) &&
+                        !mSettings->library.podtype(ctt->str())) // TODO: handle complex type expression
+                        return true;
+                }
+                else
+                    return true;
+            }
+            else
+                return true;
+        }
+
+        // Known type that doesn't need initialization or
+        // known type that has member variables of an unknown type
+        else if (var.type()->needInitialization != Type::NeedInitialization::True)
+            return true;
+    }
+
+    // Check if type can't be copied
+    if (!var.isPointer() && !var.isPointerArray() && var.typeScope()) {
+        if (funcType == FunctionType::eMoveConstructor) {
+            if (canNotMove(var.typeScope()))
+                return true;
+        }
+        else {
+            if (canNotCopy(var.typeScope()))
+                return true;
+        }
+    }
+    return false;
+}
+
+void CheckClass::handleUnionMembers(std::vector<Usage>& usageList)
+{
+    // Assign 1 union member => assign all union members
+    for (const Usage& usage : usageList) {
+        const Variable& var = *usage.var;
+        if (!usage.assign && !usage.init)
+            continue;
+        const Scope* varScope1 = var.nameToken()->scope();
+        while (varScope1->type == ScopeType::eStruct)
+            varScope1 = varScope1->nestedIn;
+        if (varScope1->type == ScopeType::eUnion) {
+            for (Usage& usage2 : usageList) {
+                const Variable& var2 = *usage2.var;
+                if (usage2.assign || usage2.init || var2.isStatic())
+                    continue;
+                const Scope* varScope2 = var2.nameToken()->scope();
+                while (varScope2->type == ScopeType::eStruct)
+                    varScope2 = varScope2->nestedIn;
+                if (varScope1 == varScope2)
+                    usage2.assign = true;
+            }
+        }
+    }
+}
+
 //---------------------------------------------------------------------------
 // ClassCheck: Check that all class constructors are ok.
 //---------------------------------------------------------------------------
@@ -145,6 +229,7 @@ void CheckClass::constructors()
         });
 
         // There are no constructors.
+        std::set<const Variable*> diagVars;
         if (scope->numConstructors == 0 && printStyle && !usedInUnion) {
             // If there is a private variable, there should be a constructor..
             int needInit = 0, haveInit = 0;
@@ -163,8 +248,10 @@ void CheckClass::constructors()
                 if (haveInit == 0)
                     noConstructorError(scope->classDef, scope->className, scope->classDef->str() == "struct");
                 else
-                    for (const Variable* uv : uninitVars)
+                    for (const Variable* uv : uninitVars) {
                         uninitVarError(uv->typeStartToken(), uv->scope()->className, uv->name());
+                        diagVars.emplace(uv);
+                    }
             }
         }
 
@@ -201,85 +288,15 @@ void CheckClass::constructors()
             std::list<const Function *> callstack;
             initializeVarList(func, callstack, scope, usageList);
 
-            // Assign 1 union member => assign all union members
-            for (const Usage &usage : usageList) {
-                const Variable& var = *usage.var;
-                if (!usage.assign && !usage.init)
-                    continue;
-                const Scope* varScope1 = var.nameToken()->scope();
-                while (varScope1->type == ScopeType::eStruct)
-                    varScope1 = varScope1->nestedIn;
-                if (varScope1->type == ScopeType::eUnion) {
-                    for (Usage &usage2 : usageList) {
-                        const Variable& var2 = *usage2.var;
-                        if (usage2.assign || usage2.init || var2.isStatic())
-                            continue;
-                        const Scope* varScope2 = var2.nameToken()->scope();
-                        while (varScope2->type == ScopeType::eStruct)
-                            varScope2 = varScope2->nestedIn;
-                        if (varScope1 == varScope2)
-                            usage2.assign = true;
-                    }
-                }
-            }
+            handleUnionMembers(usageList);
 
             // Check if any variables are uninitialized
             for (const Usage &usage : usageList) {
-                const Variable& var = *usage.var;
-
-                if (usage.assign || usage.init || var.isStatic())
-                    continue;
-
-                if (!var.nameToken() || var.nameToken()->isAnonymous())
-                    continue;
-
-                if (var.valueType() && var.valueType()->pointer == 0 && var.type() && var.type()->needInitialization == Type::NeedInitialization::False && var.type()->derivedFrom.empty())
-                    continue;
-
-                if (var.isConst() && func.isOperator()) // We can't set const members in assignment operator
+                if (isInitialized(usage, func.type))
                     continue;
 
-                // Check if this is a class constructor
-                if (!var.isPointer() && !var.isPointerArray() && var.isClass() && func.type == FunctionType::eConstructor) {
-                    // Unknown type so assume it is initialized
-                    if (!var.type()) {
-                        if (var.isStlType() && var.valueType() && var.valueType()->containerTypeToken) {
-                            if (var.valueType()->type == ValueType::Type::ITERATOR)
-                            {
-                                // needs initialization
-                            }
-                            else if (var.getTypeName() == "std::array") {
-                                const Token* ctt = var.valueType()->containerTypeToken;
-                                if (!ctt->isStandardType() &&
-                                    (!ctt->type() || ctt->type()->needInitialization != Type::NeedInitialization::True) &&
-                                    !mSettings->library.podtype(ctt->str())) // TODO: handle complex type expression
-                                    continue;
-                            }
-                            else
-                                continue;
-                        }
-                        else
-                            continue;
-                    }
-
-                    // Known type that doesn't need initialization or
-                    // known type that has member variables of an unknown type
-                    else if (var.type()->needInitialization != Type::NeedInitialization::True)
-                        continue;
-                }
-
-                // Check if type can't be copied
-                if (!var.isPointer() && !var.isPointerArray() && var.typeScope()) {
-                    if (func.type == FunctionType::eMoveConstructor) {
-                        if (canNotMove(var.typeScope()))
-                            continue;
-                    } else {
-                        if (canNotCopy(var.typeScope()))
-                            continue;
-                    }
-                }
-
                 // Is there missing member copy in copy/move constructor or assignment operator?
+                const Variable& var = *usage.var;
                 bool missingCopy = false;
 
                 // Don't warn about unknown types in copy constructors since we
@@ -326,6 +343,38 @@ void CheckClass::constructors()
                 }
             }
         }
+
+        if (scope->numConstructors == 0) {
+
+            // Mark all variables not used
+            clearAllVar(usageList);
+
+            // Variables with default initializers
+            bool hasAnyDefaultInit = false;
+            for (Usage& usage : usageList) {
+                const Variable& var = *usage.var;
+
+                // check for C++11 initializer
+                if (var.hasDefault()) {
+                    usage.init = true;
+                    hasAnyDefaultInit = true;
+                }
+            }
+            if (!hasAnyDefaultInit)
+                continue;
+
+            handleUnionMembers(usageList);
+
+            // Check if any variables are uninitialized
+            for (const Usage& usage : usageList) {
+                if (isInitialized(usage, FunctionType::eConstructor))
+                    continue;
+
+                const Variable& var = *usage.var;
+                if (diagVars.count(&var) == 0)
+                    uninitVarError(scope->bodyStart, false, FunctionType::eConstructor, var.scope()->className, var.name(), false, false, true);
+            }
+        }
     }
 }
 
@@ -1118,17 +1167,22 @@ void CheckClass::noExplicitConstructorError(const Token *tok, const std::string
     reportError(tok, Severity::style, "noExplicitConstructor", "$symbol:" + classname + '\n' + message + '\n' + verbose, CWE398, Certainty::normal);
 }
 
-void CheckClass::uninitVarError(const Token *tok, bool isprivate, FunctionType functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive)
+void CheckClass::uninitVarError(const Token *tok, bool isprivate, FunctionType functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive, bool noCtor)
 {
-    std::string ctor;
-    if (functionType == FunctionType::eCopyConstructor)
-        ctor = "copy ";
-    else if (functionType == FunctionType::eMoveConstructor)
-        ctor = "move ";
-    std::string message("Member variable '$symbol' is not initialized in the " + ctor + "constructor.");
+    std::string message("Member variable '$symbol' ");
+    if (noCtor)
+        message += "has no initializer.";
+    else {
+        message += "is not initialized in the ";
+        if (functionType == FunctionType::eCopyConstructor)
+            message += "copy ";
+        else if (functionType == FunctionType::eMoveConstructor)
+            message += "move ";
+        message += "constructor.";
+    }
     if (derived)
         message += " Maybe it should be initialized directly in the class " + classname + "?";
-    std::string id = std::string("uninit") + (derived ? "Derived" : "") + "MemberVar" + (isprivate ? "Private" : "");
+    std::string id = std::string("uninit") + (derived ? "Derived" : "") + "MemberVar" + (isprivate ? "Private" : "") + (noCtor ? "NoCtor" : "");
     const std::string verbose {message + " Member variables of native types, pointers, or references are left uninitialized when the class is instantiated. That may cause bugs or undefined behavior."};
     reportError(tok, Severity::warning, id, "$symbol:" + classname + "::" + varname + '\n' + message + '\n' + verbose, CWE398, inconclusive ? Certainty::inconclusive : Certainty::normal);
 }

lib/checkclass.h

@@ -155,7 +155,7 @@ class CPPCHECKLIB CheckClass : public Check {
     void noCopyConstructorError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive);
     void noOperatorEqError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive);
     void noDestructorError(const Scope *scope, bool isdefault, const Token *alloc);
-    void uninitVarError(const Token *tok, bool isprivate, FunctionType functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive);
+    void uninitVarError(const Token *tok, bool isprivate, FunctionType functionType, const std::string &classname, const std::string &varname, bool derived, bool inconclusive, bool noCtor = false);
     void uninitVarError(const Token *tok, const std::string &classname, const std::string &varname);
     void missingMemberCopyError(const Token *tok, FunctionType functionType, const std::string& classname, const std::string& varname);
     void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive);
@@ -254,6 +254,10 @@ class CPPCHECKLIB CheckClass : public Check {
         bool init{};
     };
 
+    static void handleUnionMembers(std::vector<Usage>& usageList);
+
+    bool isInitialized(const Usage& usage, FunctionType funcType) const;
+
     static bool isBaseClassMutableMemberFunc(const Token *tok, const Scope *scope);
 
     /**

lib/pathmatch.h

@@ -274,9 +274,9 @@ class PathMatch::PathIterator {
     /* Position struct */
     struct Pos {
         /* String pointer */
-        const char *p;
+        const char *p{};
         /* Raw characters left */
-        std::size_t l;
+        std::size_t l{};
         /* Buffered character */
         int c {EOF};
     };

lib/tokenize.h

@@ -700,12 +700,12 @@ class CPPCHECKLIB Tokenizer {
         std::string name;
         std::string originalName;
         std::string filename;
-        int lineNumber;
-        int column;
+        int lineNumber{};
+        int column{};
         int tagLine{-1};
         int tagColumn{-1};
-        bool used;
-        bool isFunctionPointer;
+        bool used{};
+        bool isFunctionPointer{};
         std::vector<TypedefToken> typedefInfoTokens;
     };
     std::vector<TypedefInfo> mTypedefInfo;

releasenotes.txt

@@ -7,6 +7,7 @@ Major bug fixes & crashes:
 New checks:
 - MISRA C 2012 rule 10.3 now warns on assigning integer literals 0 and 1 to bool in C99 and later while preserving the existing C89 behavior.
 - funcArgNamesDifferentUnnamed warns on function declarations/definitions where a parameter in either location is unnamed
+- uninitMemberVarNoCtor warns on user-defined types where some but not all members requiring initialization have in-class initializers.
 
 C/C++ support:
 -

test/testconstructors.cpp

@@ -92,6 +92,7 @@ class TestConstructors : public TestFixture {
         TEST_CASE(noConstructor13); // #9998
         TEST_CASE(noConstructor14); // #10770
         TEST_CASE(noConstructor15); // #5499
+        TEST_CASE(noConstructor16);
 
         TEST_CASE(forwardDeclaration); // ticket #4290/#3190
 
@@ -758,6 +759,18 @@ class TestConstructors : public TestFixture {
         ASSERT_EQUALS("[test.cpp:3:5]: (warning) Member variable 'C::i2' is not initialized in the constructor. [uninitMemberVar]\n", errout_str());
     }
 
+    void noConstructor16() {
+        check("struct S {\n" // #14546
+              "    int a = 0, b;\n"
+              "};\n");
+        ASSERT_EQUALS("[test.cpp:1:10]: (warning) Member variable 'S::b' has no initializer. [uninitMemberVarNoCtor]\n", errout_str());
+
+        check("struct S {\n"
+              "    int a, b;\n"
+              "};\n");
+        ASSERT_EQUALS("", errout_str());
+    }
+
     // ticket #4290 "False Positive: style (noConstructor): The class 'foo' does not have a constructor."
     // ticket #3190 "SymbolDatabase: Parse of sub class constructor fails"
     void forwardDeclaration() {