Skip to content

Commit f450285

Browse files
firewavefirewave
authored
improved errorhandling of --rule= and --rule-file= / some related cleanups (danmar#6212)
1 parent 9e7578e commit f450285

5 files changed

Lines changed: 210 additions & 18 deletions

File tree

‎cli/cmdlineparser.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,6 +1074,12 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
10741074
#ifdef HAVE_RULES
10751075
Settings::Rule rule;
10761076
rule.pattern = 7 + argv[i];
1077+
1078+
if (rule.pattern.empty()) {
1079+
mLogger.printError("no rule pattern provided.");
1080+
return Result::Fail;
1081+
}
1082+
10771083
mSettings.rules.emplace_back(std::move(rule));
10781084
#else
10791085
mLogger.printError("Option --rule cannot be used as Cppcheck has not been built with rules support.");
@@ -1134,6 +1140,11 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
11341140
return Result::Fail;
11351141
}
11361142

1143+
if (rule.id.empty()) {
1144+
mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking an id.");
1145+
return Result::Fail;
1146+
}
1147+
11371148
if (rule.tokenlist.empty()) {
11381149
mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking a tokenlist.");
11391150
return Result::Fail;
@@ -1144,6 +1155,11 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
11441155
return Result::Fail;
11451156
}
11461157

1158+
if (rule.severity == Severity::none) {
1159+
mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule has an invalid severity.");
1160+
return Result::Fail;
1161+
}
1162+
11471163
mSettings.rules.emplace_back(std::move(rule));
11481164
}
11491165
} else {

‎lib/cppcheck.cpp‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1298,13 +1298,14 @@ void CppCheck::executeRules(const std::string &tokenlist, const TokenList &list)
12981298
return;
12991299

13001300
// Write all tokens in a string that can be parsed by pcre
1301-
std::ostringstream ostr;
1302-
for (const Token *tok = list.front(); tok; tok = tok->next())
1303-
ostr << " " << tok->str();
1304-
const std::string str(ostr.str());
1301+
std::string str;
1302+
for (const Token *tok = list.front(); tok; tok = tok->next()) {
1303+
str += " ";
1304+
str += tok->str();
1305+
}
13051306

13061307
for (const Settings::Rule &rule : mSettings.rules) {
1307-
if (rule.pattern.empty() || rule.id.empty() || rule.severity == Severity::none || rule.tokenlist != tokenlist)
1308+
if (rule.tokenlist != tokenlist)
13081309
continue;
13091310

13101311
if (!mSettings.quiet) {
@@ -1379,28 +1380,30 @@ void CppCheck::executeRules(const std::string &tokenlist, const TokenList &list)
13791380
pos = (int)pos2;
13801381

13811382
// determine location..
1382-
std::string file = list.getSourceFilePath();
1383+
int fileIndex = 0;
13831384
int line = 0;
13841385

13851386
std::size_t len = 0;
13861387
for (const Token *tok = list.front(); tok; tok = tok->next()) {
13871388
len = len + 1U + tok->str().size();
13881389
if (len > pos1) {
1389-
file = list.getFiles().at(tok->fileIndex());
1390+
fileIndex = tok->fileIndex();
13901391
line = tok->linenr();
13911392
break;
13921393
}
13931394
}
13941395

1396+
const std::string& file = list.getFiles()[fileIndex];
1397+
13951398
ErrorMessage::FileLocation loc(file, line, 0);
13961399

13971400
// Create error message
1398-
std::string summary;
1399-
if (rule.summary.empty())
1400-
summary = "found '" + str.substr(pos1, pos2 - pos1) + "'";
1401-
else
1402-
summary = rule.summary;
1403-
const ErrorMessage errmsg({std::move(loc)}, list.getSourceFilePath(), rule.severity, summary, rule.id, Certainty::normal);
1401+
const ErrorMessage errmsg({std::move(loc)},
1402+
list.getSourceFilePath(),
1403+
rule.severity,
1404+
!rule.summary.empty() ? rule.summary : "found '" + str.substr(pos1, pos2 - pos1) + "'",
1405+
rule.id,
1406+
Certainty::normal);
14041407

14051408
// Report error
14061409
reportErr(errmsg);

‎lib/errortypes.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ std::string severityToString(Severity severity)
7272
throw InternalError(nullptr, "Unknown severity");
7373
}
7474

75+
// TODO: bail out on invalid severity
7576
Severity severityFromString(const std::string& severity)
7677
{
7778
if (severity.empty())

‎test/cli/other_test.py‎

Lines changed: 124 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,7 +1182,7 @@ def test_unknown_extension(tmpdir):
11821182
assert stderr == ''
11831183

11841184

1185-
def test_multiple_define_rules(tmpdir):
1185+
def test_rule_file_define_multiple(tmpdir):
11861186
rule_file = os.path.join(tmpdir, 'rule_file.xml')
11871187
with open(rule_file, 'wt') as f:
11881188
f.write("""
@@ -1201,6 +1201,7 @@ def test_multiple_define_rules(tmpdir):
12011201
<message>
12021202
<severity>error</severity>
12031203
<id>ruleId2</id>
1204+
<summary>define2</summary>
12041205
</message>
12051206
</rule>
12061207
</rules>""")
@@ -1213,18 +1214,136 @@ def test_multiple_define_rules(tmpdir):
12131214
void f() { }
12141215
''')
12151216

1216-
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
1217+
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), '-DDEF_3', test_file])
12171218
assert exitcode == 0, stderr
12181219
lines = stdout.splitlines()
12191220
assert lines == [
12201221
'Checking {} ...'.format(test_file),
12211222
'Processing rule: DEF_1',
1222-
'Processing rule: DEF_2'
1223+
'Processing rule: DEF_2',
1224+
'Checking {}: DEF_3=1...'.format(test_file)
12231225
]
12241226
lines = stderr.splitlines()
12251227
assert lines == [
12261228
"{}:2:0: error: found 'DEF_1' [ruleId1]".format(test_file),
1227-
"{}:3:0: error: found 'DEF_2' [ruleId2]".format(test_file)
1229+
"{}:3:0: error: define2 [ruleId2]".format(test_file)
1230+
]
1231+
1232+
1233+
def test_rule_file_define(tmpdir):
1234+
rule_file = os.path.join(tmpdir, 'rule_file.xml')
1235+
with open(rule_file, 'wt') as f:
1236+
f.write("""
1237+
<rule>
1238+
<tokenlist>define</tokenlist>
1239+
<pattern>DEF_.</pattern>
1240+
</rule>
1241+
""")
1242+
1243+
test_file = os.path.join(tmpdir, 'test.c')
1244+
with open(test_file, 'wt') as f:
1245+
f.write('''
1246+
#define DEF_1
1247+
#define DEF_2
1248+
void f() { }
1249+
''')
1250+
1251+
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), '-DDEF_3', test_file])
1252+
assert exitcode == 0, stdout
1253+
lines = stdout.splitlines()
1254+
assert lines == [
1255+
'Checking {} ...'.format(test_file),
1256+
'Processing rule: DEF_.',
1257+
'Checking {}: DEF_3=1...'.format(test_file)
1258+
]
1259+
lines = stderr.splitlines()
1260+
assert lines == [
1261+
"{}:2:0: style: found 'DEF_1' [rule]".format(test_file),
1262+
"{}:3:0: style: found 'DEF_2' [rule]".format(test_file)
1263+
]
1264+
1265+
1266+
def test_rule_file_normal(tmpdir):
1267+
rule_file = os.path.join(tmpdir, 'rule_file.xml')
1268+
with open(rule_file, 'wt') as f:
1269+
f.write("""
1270+
<rule>
1271+
<pattern>int</pattern>
1272+
</rule>
1273+
""")
1274+
1275+
test_file = os.path.join(tmpdir, 'test.c')
1276+
with open(test_file, 'wt') as f:
1277+
f.write('''
1278+
#define DEF_1
1279+
#define DEF_2
1280+
typedef int i32;
1281+
void f(i32) { }
1282+
''')
1283+
1284+
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
1285+
assert exitcode == 0, stdout
1286+
lines = stdout.splitlines()
1287+
assert lines == [
1288+
'Checking {} ...'.format(test_file),
1289+
'Processing rule: int',
1290+
]
1291+
lines = stderr.splitlines()
1292+
assert lines == [
1293+
"{}:5:0: style: found 'int' [rule]".format(test_file)
12281294
]
12291295

1230-
# TODO: test "raw" and "normal" rules
1296+
1297+
def test_rule_file_raw(tmpdir):
1298+
rule_file = os.path.join(tmpdir, 'rule_file.xml')
1299+
with open(rule_file, 'wt') as f:
1300+
f.write("""
1301+
<rule>
1302+
<tokenlist>raw</tokenlist>
1303+
<pattern>i32</pattern>
1304+
</rule>
1305+
""")
1306+
1307+
test_file = os.path.join(tmpdir, 'test.c')
1308+
with open(test_file, 'wt') as f:
1309+
f.write('''
1310+
#define DEF_1
1311+
#define DEF_2
1312+
typedef int i32;
1313+
void f(i32) { }
1314+
''')
1315+
1316+
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule-file={}'.format(rule_file), test_file])
1317+
assert exitcode == 0, stdout
1318+
lines = stdout.splitlines()
1319+
assert lines == [
1320+
'Checking {} ...'.format(test_file),
1321+
'Processing rule: i32',
1322+
]
1323+
lines = stderr.splitlines()
1324+
assert lines == [
1325+
"{}:4:0: style: found 'i32' [rule]".format(test_file),
1326+
"{}:5:0: style: found 'i32' [rule]".format(test_file)
1327+
]
1328+
1329+
1330+
def test_rule(tmpdir):
1331+
test_file = os.path.join(tmpdir, 'test.c')
1332+
with open(test_file, 'wt') as f:
1333+
f.write('''
1334+
#define DEF_1
1335+
#define DEF_2
1336+
void f() { }
1337+
''')
1338+
1339+
exitcode, stdout, stderr = cppcheck(['--template=simple', '--rule=f', test_file])
1340+
assert exitcode == 0, stdout
1341+
lines = stdout.splitlines()
1342+
assert lines == [
1343+
'Checking {} ...'.format(test_file),
1344+
'Processing rule: f',
1345+
]
1346+
lines = stderr.splitlines()
1347+
assert lines == [
1348+
"{}:4:0: style: found 'f' [rule]".format(test_file)
1349+
]

‎test/testcmdlineparser.cpp‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@ class TestCmdlineParser : public TestFixture {
333333
TEST_CASE(addonMissing);
334334
#ifdef HAVE_RULES
335335
TEST_CASE(rule);
336+
TEST_CASE(ruleMissingPattern);
336337
#else
337338
TEST_CASE(ruleNotSupported);
338339
#endif
@@ -349,6 +350,9 @@ class TestCmdlineParser : public TestFixture {
349350
TEST_CASE(ruleFileUnknownElement2);
350351
TEST_CASE(ruleFileMissingTokenList);
351352
TEST_CASE(ruleFileUnknownTokenList);
353+
TEST_CASE(ruleFileMissingId);
354+
TEST_CASE(ruleFileInvalidSeverity1);
355+
TEST_CASE(ruleFileInvalidSeverity2);
352356
#else
353357
TEST_CASE(ruleFileNotSupported);
354358
#endif
@@ -2177,6 +2181,13 @@ class TestCmdlineParser : public TestFixture {
21772181
auto it = settings->rules.cbegin();
21782182
ASSERT_EQUALS(".+", it->pattern);
21792183
}
2184+
2185+
void ruleMissingPattern() {
2186+
REDIRECT;
2187+
const char * const argv[] = {"cppcheck", "--rule=", "file.cpp"};
2188+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2189+
ASSERT_EQUALS("cppcheck: error: no rule pattern provided.\n", logger->str());
2190+
}
21802191
#else
21812192
void ruleNotSupported() {
21822193
REDIRECT;
@@ -2359,6 +2370,48 @@ class TestCmdlineParser : public TestFixture {
23592370
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
23602371
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is using the unsupported tokenlist 'simple'.\n", logger->str());
23612372
}
2373+
2374+
void ruleFileMissingId() {
2375+
REDIRECT;
2376+
ScopedFile file("rule.xml",
2377+
"<rule>\n"
2378+
"<pattern>.+</pattern>\n"
2379+
"<message>\n"
2380+
"<id/>"
2381+
"</message>\n"
2382+
"</rule>\n");
2383+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2384+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2385+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking an id.\n", logger->str());
2386+
}
2387+
2388+
void ruleFileInvalidSeverity1() {
2389+
REDIRECT;
2390+
ScopedFile file("rule.xml",
2391+
"<rule>\n"
2392+
"<pattern>.+</pattern>\n"
2393+
"<message>\n"
2394+
"<severity/>"
2395+
"</message>\n"
2396+
"</rule>\n");
2397+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2398+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2399+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule has an invalid severity.\n", logger->str());
2400+
}
2401+
2402+
void ruleFileInvalidSeverity2() {
2403+
REDIRECT;
2404+
ScopedFile file("rule.xml",
2405+
"<rule>\n"
2406+
"<pattern>.+</pattern>\n"
2407+
"<message>\n"
2408+
"<severity>none</severity>"
2409+
"</message>\n"
2410+
"</rule>\n");
2411+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2412+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2413+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule has an invalid severity.\n", logger->str());
2414+
}
23622415
#else
23632416
void ruleFileNotSupported() {
23642417
REDIRECT;

0 commit comments

Comments
 (0)