improved errorhandling of --rule= and --rule-file= / some related cleanups

cli/cmdlineparser.cpp

@@ -1065,6 +1065,12 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
 #ifdef HAVE_RULES
                 Settings::Rule rule;
                 rule.pattern = 7 + argv[i];
+
+                if (rule.pattern.empty()) {
+                    mLogger.printError("no rule pattern provided.");
+                    return Result::Fail;
+                }
+
                 mSettings.rules.emplace_back(std::move(rule));
 #else
                 mLogger.printError("Option --rule cannot be used as Cppcheck has not been built with rules support.");
@@ -1125,6 +1131,11 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
                             return Result::Fail;
                         }
 
+                        if (rule.id.empty()) {
+                            mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking an id.");
+                            return Result::Fail;
+                        }
+
                         if (rule.tokenlist.empty()) {
                             mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking a tokenlist.");
                             return Result::Fail;
@@ -1135,6 +1146,11 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
                             return Result::Fail;
                         }
 
+                        if (rule.severity == Severity::none) {
+                            mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule has an invalid severity.");
+                            return Result::Fail;
+                        }
+
                         mSettings.rules.emplace_back(std::move(rule));
                     }
                 } else {

lib/cppcheck.cpp

@@ -1298,13 +1298,14 @@ void CppCheck::executeRules(const std::string &tokenlist, const TokenList &list)
         return;
 
     // Write all tokens in a string that can be parsed by pcre
-    std::ostringstream ostr;
-    for (const Token *tok = list.front(); tok; tok = tok->next())
-        ostr << " " << tok->str();
-    const std::string str(ostr.str());
+    std::string str;
+    for (const Token *tok = list.front(); tok; tok = tok->next()) {
+        str += " ";
+        str += tok->str();
+    }
 
     for (const Settings::Rule &rule : mSettings.rules) {
-        if (rule.pattern.empty() || rule.id.empty() || rule.severity == Severity::none || rule.tokenlist != tokenlist)
+        if (rule.tokenlist != tokenlist)
             continue;
 
         if (!mSettings.quiet) {
@@ -1379,28 +1380,30 @@ void CppCheck::executeRules(const std::string &tokenlist, const TokenList &list)
             pos = (int)pos2;
 
             // determine location..
-            std::string file = list.getSourceFilePath();
+            int fileIndex = 0;
             int line = 0;
 
             std::size_t len = 0;
             for (const Token *tok = list.front(); tok; tok = tok->next()) {
                 len = len + 1U + tok->str().size();
                 if (len > pos1) {
-                    file = list.getFiles().at(tok->fileIndex());
+                    fileIndex = tok->fileIndex();
                     line = tok->linenr();
                     break;
                 }
             }
 
+            const std::string& file = list.getFiles()[fileIndex];
+
             ErrorMessage::FileLocation loc(file, line, 0);
 
             // Create error message
-            std::string summary;
-            if (rule.summary.empty())
-                summary = "found '" + str.substr(pos1, pos2 - pos1) + "'";
-            else
-                summary = rule.summary;
-            const ErrorMessage errmsg({std::move(loc)}, list.getSourceFilePath(), rule.severity, summary, rule.id, Certainty::normal);
+            const ErrorMessage errmsg({std::move(loc)},
+                                      list.getSourceFilePath(),
+                                      rule.severity,
+                                      !rule.summary.empty() ? rule.summary : "found '" + str.substr(pos1, pos2 - pos1) + "'",
+                                      rule.id,
+                                      Certainty::normal);
 
             // Report error
             reportErr(errmsg);

lib/errortypes.cpp

@@ -72,6 +72,7 @@ std::string severityToString(Severity severity)
     throw InternalError(nullptr, "Unknown severity");
 }
 
+// TODO: bail out on invalid severity
 Severity severityFromString(const std::string& severity)
 {
     if (severity.empty())

test/cli/other_test.py

@@ -1182,7 +1182,7 @@ def test_unknown_extension(tmpdir):
     assert stderr == ''
 
 
-def test_multiple_define_rules(tmpdir):
+def test_rule_file_define_multiple(tmpdir):
     rule_file = os.path.join(tmpdir, 'rule_file.xml')
     with open(rule_file, 'wt') as f:
         f.write("""
@@ -1201,6 +1201,7 @@ def test_multiple_define_rules(tmpdir):
         <message>
             <severity>error</severity>
             <id>ruleId2</id>
+            <summary>define2</summary>
         </message>
     </rule>
 </rules>""")
@@ -1213,18 +1214,136 @@ def test_multiple_define_rules(tmpdir):
 void f() { }
 ''')
 
-    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
+    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), '-DDEF_3', test_file])
     assert exitcode == 0, stderr
     lines = stdout.splitlines()
     assert lines == [
         'Checking {} ...'.format(test_file),
         'Processing rule: DEF_1',
-        'Processing rule: DEF_2'
+        'Processing rule: DEF_2',
+        'Checking {}: DEF_3=1...'.format(test_file)
     ]
     lines = stderr.splitlines()
     assert lines == [
         "{}:2:0: error: found 'DEF_1' [ruleId1]".format(test_file),
-        "{}:3:0: error: found 'DEF_2' [ruleId2]".format(test_file)
+        "{}:3:0: error: define2 [ruleId2]".format(test_file)
+    ]
+
+
+def test_rule_file_define(tmpdir):
+    rule_file = os.path.join(tmpdir, 'rule_file.xml')
+    with open(rule_file, 'wt') as f:
+        f.write("""
+<rule>
+    <tokenlist>define</tokenlist>
+    <pattern>DEF_.</pattern>
+</rule>
+""")
+
+    test_file = os.path.join(tmpdir, 'test.c')
+    with open(test_file, 'wt') as f:
+        f.write('''
+#define DEF_1
+#define DEF_2
+void f() { }
+''')
+
+    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), '-DDEF_3', test_file])
+    assert exitcode == 0, stdout
+    lines = stdout.splitlines()
+    assert lines == [
+        'Checking {} ...'.format(test_file),
+        'Processing rule: DEF_.',
+        'Checking {}: DEF_3=1...'.format(test_file)
+    ]
+    lines = stderr.splitlines()
+    assert lines == [
+        "{}:2:0: style: found 'DEF_1' [rule]".format(test_file),
+        "{}:3:0: style: found 'DEF_2' [rule]".format(test_file)
+    ]
+
+
+def test_rule_file_normal(tmpdir):
+    rule_file = os.path.join(tmpdir, 'rule_file.xml')
+    with open(rule_file, 'wt') as f:
+        f.write("""
+<rule>
+    <pattern>int</pattern>
+</rule>
+""")
+
+    test_file = os.path.join(tmpdir, 'test.c')
+    with open(test_file, 'wt') as f:
+        f.write('''
+#define DEF_1
+#define DEF_2
+typedef int i32;
+void f(i32) { }
+''')
+
+    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
+    assert exitcode == 0, stdout
+    lines = stdout.splitlines()
+    assert lines == [
+        'Checking {} ...'.format(test_file),
+        'Processing rule: int',
+    ]
+    lines = stderr.splitlines()
+    assert lines == [
+        "{}:5:0: style: found 'int' [rule]".format(test_file)
     ]
 
-# TODO: test "raw" and "normal" rules
+
+def test_rule_file_raw(tmpdir):
+    rule_file = os.path.join(tmpdir, 'rule_file.xml')
+    with open(rule_file, 'wt') as f:
+        f.write("""
+<rule>
+    <tokenlist>raw</tokenlist>
+    <pattern>i32</pattern>
+</rule>
+""")
+
+    test_file = os.path.join(tmpdir, 'test.c')
+    with open(test_file, 'wt') as f:
+        f.write('''
+#define DEF_1
+#define DEF_2
+typedef int i32;
+void f(i32) { }
+''')
+
+    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
+    assert exitcode == 0, stdout
+    lines = stdout.splitlines()
+    assert lines == [
+        'Checking {} ...'.format(test_file),
+        'Processing rule: i32',
+    ]
+    lines = stderr.splitlines()
+    assert lines == [
+        "{}:4:0: style: found 'i32' [rule]".format(test_file),
+        "{}:5:0: style: found 'i32' [rule]".format(test_file)
+    ]
+
+
+def test_rule(tmpdir):
+    test_file = os.path.join(tmpdir, 'test.c')
+    with open(test_file, 'wt') as f:
+        f.write('''
+#define DEF_1
+#define DEF_2
+void f() { }
+''')
+
+    exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule=f', test_file])
+    assert exitcode == 0, stdout
+    lines = stdout.splitlines()
+    assert lines == [
+        'Checking {} ...'.format(test_file),
+        'Processing rule: f',
+    ]
+    lines = stderr.splitlines()
+    assert lines == [
+        "{}:4:0: style: found 'f' [rule]".format(test_file)
+    ]

test/testcmdlineparser.cpp

@@ -332,6 +332,7 @@ class TestCmdlineParser : public TestFixture {
         TEST_CASE(addonMissing);
 #ifdef HAVE_RULES
         TEST_CASE(rule);
+        TEST_CASE(ruleMissingPattern);
 #else
         TEST_CASE(ruleNotSupported);
 #endif
@@ -348,6 +349,9 @@ class TestCmdlineParser : public TestFixture {
         TEST_CASE(ruleFileUnknownElement2);
         TEST_CASE(ruleFileMissingTokenList);
         TEST_CASE(ruleFileUnknownTokenList);
+        TEST_CASE(ruleFileMissingId);
+        TEST_CASE(ruleFileInvalidSeverity1);
+        TEST_CASE(ruleFileInvalidSeverity2);
 #else
         TEST_CASE(ruleFileNotSupported);
 #endif
@@ -2161,6 +2165,13 @@ class TestCmdlineParser : public TestFixture {
         auto it = settings->rules.cbegin();
         ASSERT_EQUALS(".+", it->pattern);
     }
+
+    void ruleMissingPattern() {
+        REDIRECT;
+        const char * const argv[] = {"cppcheck", "--rule=", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: no rule pattern provided.\n", logger->str());
+    }
 #else
     void ruleNotSupported() {
         REDIRECT;
@@ -2343,6 +2354,48 @@ class TestCmdlineParser : public TestFixture {
         ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
         ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is using the unsupported tokenlist 'simple'.\n", logger->str());
     }
+
+    void ruleFileMissingId() {
+        REDIRECT;
+        ScopedFile file("rule.xml",
+                        "<rule>\n"
+                        "<pattern>.+</pattern>\n"
+                        "<message>\n"
+                        "<id/>"
+                        "</message>\n"
+                        "</rule>\n");
+        const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking an id.\n", logger->str());
+    }
+
+    void ruleFileInvalidSeverity1() {
+        REDIRECT;
+        ScopedFile file("rule.xml",
+                        "<rule>\n"
+                        "<pattern>.+</pattern>\n"
+                        "<message>\n"
+                        "<severity/>"
+                        "</message>\n"
+                        "</rule>\n");
+        const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule has an invalid severity.\n", logger->str());
+    }
+
+    void ruleFileInvalidSeverity2() {
+        REDIRECT;
+        ScopedFile file("rule.xml",
+                        "<rule>\n"
+                        "<pattern>.+</pattern>\n"
+                        "<message>\n"
+                        "<severity>none</severity>"
+                        "</message>\n"
+                        "</rule>\n");
+        const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule has an invalid severity.\n", logger->str());
+    }
 #else
     void ruleFileNotSupported() {
         REDIRECT;