diff --git a/addons/namingng.config.json b/addons/namingng.config.json index 26cb4bbce..0abba4f15 100644 --- a/addons/namingng.config.json +++ b/addons/namingng.config.json @@ -2,6 +2,15 @@ "RE_VARNAME": ["[a-z]*[a-zA-Z0-9_]*\\Z"], "RE_PRIVATE_MEMBER_VARIABLE": null, "RE_FUNCTIONNAME": ["[a-z0-9A-Z]*\\Z"], + "include_guard": { + "input": "path", + "prefix": "", + "suffix": "", + "case": "upper", + "max_linenr": 5, + "RE_HEADERFILE": "[^/].*\\.h\\Z", + "required": true + }, "var_prefixes": {"uint32_t": "ui32", "int*": "intp"}, "function_prefixes": {"uint16_t": "ui16", diff --git a/addons/namingng.py b/addons/namingng.py index d6732494c..fb852c174 100755 --- a/addons/namingng.py +++ b/addons/namingng.py @@ -3,6 +3,7 @@ # cppcheck addon for naming conventions # An enhanced version. Configuration is taken from a json file # It supports to check for type-based prefixes in function or variable names. +# Aside from include guard naming, include guard presence can also be tested. # # Example usage (variable name must start with lowercase, function name must start with uppercase): # $ cppcheck --dump path-to-src/ @@ -11,9 +12,18 @@ # JSON format: # # { -# "RE_VARNAME": "[a-z]*[a-zA-Z0-9_]*\\Z", +# "RE_VARNAME": ["[a-z]*[a-zA-Z0-9_]*\\Z"], # "RE_PRIVATE_MEMBER_VARIABLE": null, -# "RE_FUNCTIONNAME": "[a-z0-9A-Z]*\\Z", +# "RE_FUNCTIONNAME": ["[a-z0-9A-Z]*\\Z"], +# "_comment": "comments can be added to the config with underscore-prefixed keys", +# "include_guard": { +# "input": "path", +# "prefix": "GUARD_", +# "case": "upper", +# "max_linenr": 5, +# "RE_HEADERFILE": "[^/].*\\.h\\Z", +# "required": true +# }, # "var_prefixes": {"uint32_t": "ui32"}, # "function_prefixes": {"uint16_t": "ui16", # "uint32_t": "ui32"} @@ -40,10 +50,89 @@ class DataStruct: def reportNamingError(location,message,errorId='namingConvention',severity='style',extra=''): cppcheckdata.reportError(location,severity,message,'namingng',errorId,extra) +def configError(error,fatal=True): + print('config error: %s'%error) + if fatal: + sys.exit(1) + def loadConfig(configfile): - with open(configfile) as fh: - data = json.load(fh) - return data + if not os.path.exists(configfile): + configError("cannot find config file '%s'"%configfile) + + try: + with open(configfile) as fh: + try: + data = json.load(fh) + except json.JSONDecodeError as e: + configError("error parsing config file as JSON at line %d: %s"%(e.lineno,e.msg)) + except Exception as e: + configError("error opening config file '%s': %s"%(configfile,e)) + + if not isinstance(data, dict): + configError('config file must contain a JSON object at the top level') + + # All errors are emitted before bailing out, to make the unit test more + # effective. + have_error = False + + # Put config items in a class, so that settings can be accessed using + # config.feature + class Config: + pass + config = Config() + + mapping = { + 'file': ('RE_FILE', list), + 'namespace': ('RE_NAMESPACE', list), + 'include_guard': ('include_guard', dict), + 'variable': ('RE_VARNAME', list), + 'variable_prefixes': ('var_prefixes', dict, {}), + 'private_member': ('RE_PRIVATE_MEMBER_VARIABLE', list), + 'public_member': ('RE_PUBLIC_MEMBER_VARIABLE', list), + 'global_variable': ('RE_GLOBAL_VARNAME', list), + 'function_name': ('RE_FUNCTIONNAME', list), + 'function_prefixes': ('function_prefixes', dict, {}), + 'class_name': ('RE_CLASS_NAME', list), + 'skip_one_char_variables': ('skip_one_char_variables', bool), + } + + # parse defined keys and store as members of config object + for key,opts in mapping.items(): + json_key = opts[0] + req_type = opts[1] + default = None if len(opts)<3 else opts[2] + + value = data.pop(json_key,default) + if value is not None and not isinstance(value, req_type): + req_typename = req_type.__name__ + got_typename = type(value).__name__ + configError('%s must be %s (not %s), or not set'%(json_key,req_typename,got_typename),fatal=False) + have_error = True + continue + + if req_type == list and value is not None: + # type 'list' implies a list of regular expressions + for item in value: + try: + re.compile(item) + except re.error as err: + configError("item '%s' of '%s' is not a valid regular expression: %s"%(item,json_key,err),fatal=False) + have_error = True + if have_error: + continue + + setattr(config,key,value) + + # check remaining keys, only accept underscore-prefixed comments + for key,value in data.items(): + if key == '' or key[0] != '_': + configError("unknown config key '%s'"%key,fatal=False) + have_error = True + + if have_error: + sys.exit(1) + + return config def checkTrueRegex(data, expr, msg): @@ -73,10 +162,115 @@ def evalExpr(conf, exp, mockToken, msgType): msg = msgType + ' ' + mockToken.str + ' violates naming convention' checkFalseRegex(mockToken, exp, msg) +def check_include_guard_name(conf_include_guard,cfg,directive): + parts = directive.str.split() + if len(parts) != 2: + msg = 'syntax error' + reportNamingError(directive,msg,'syntax') + return None + guard_name = parts[1] + + filename = directive.file + if conf_include_guard.get('input','path') == 'basename': + filename = os.path.basename(filename) + use_case = conf_include_guard.get('case','upper') + if use_case == 'upper': + filename = filename.upper() + elif use_case == 'lower': + filename = filename.lower() + elif use_case == 'keep': + pass # keep filename case as-is + else: + print("invalid config value for 'case': '%s'"%use_case,file=sys.stderr) + sys.exit(1) + + barename = re.sub('[^A-Za-z0-9]','_',filename).strip('_') + expect_guard_name = conf_include_guard.get('prefix','') + barename + conf_include_guard.get('suffix','') + if expect_guard_name != guard_name: + msg = 'include guard naming violation; %s != %s'%(guard_name,expect_guard_name) + reportNamingError(directive,msg,'includeGuardName') + + return guard_name + +def check_include_guard(conf_include_guard,cfg,unguarded_include_files): + # Scan for '#ifndef FILE_H' as the first directive, in the first N lines. + # Then test whether the next directive #defines the found name. + # Various tests are done: + # - check include guards for their naming and consistency + # - test whether include guards are in place + max_linenr = conf_include_guard.get('max_linenr', 5) + + def report(directive,msg,errorId): + reportNamingError(directive,msg,errorId) + + def report_pending_ifndef(directive): + report(directive,'include guard #ifndef is not followed by #define','includeGuardIncomplete') + + last_fn = None + pending_ifndef = None + phase = 0 + for directive in cfg.directives: + if last_fn != directive.file: + if pending_ifndef: + report_pending_ifndef(pending_ifndef) + pending_ifndef = None + last_fn = directive.file + phase = 0 + if phase == -1: + # ignore (the remainder of) this file + continue + if not re.match(include_guard_header_re,directive.file): + phase = -1 + continue + + if directive.linenr > max_linenr: + if phase == 0 and conf_include_guard.get('required',1): + report(directive,'include guard not found before line %d'%max_linenr,'includeGuardMissing') + pass + phase = -1 + continue + + if phase == 0: + # looking for '#ifndef FILE_H' + if not directive.str.startswith('#ifndef'): + if conf_include_guard.get('required',1): + report(directive,'first preprocessor directive should be include guard #ifndef','includeGuardMissing') + phase = -1 + continue + guard_name = check_include_guard_name(conf_include_guard,cfg,directive) + if guard_name == None: + phase = -1 + continue + pending_ifndef = directive + phase = 1 + elif phase == 1: + pending_ifndef = None + # looking for '#define FILE_H' + if not directive.str.startswith('#define'): + report(directive,'second preprocessor directive should be include guard #define','includeGuardIncomplete') + phase = -1 + continue + parts = directive.str.split() + if len(parts) == 1: + report(directive,'syntax error','syntax') + phase = -1 + continue + if guard_name != parts[1]: + report(directive,'include guard does not guard; %s != %s'%(guard_name,parts[1]),'includeGuardAwayFromDuty',severity='warning') + + unguarded_include_files.remove(directive.file) + + phase = -1 + if pending_ifndef: + report_pending_ifndef(pending_ifndef) def process(dumpfiles, configfile, debugprint=False): conf = loadConfig(configfile) + if conf.include_guard: + global include_guard_header_re + include_guard_header_re = conf.include_guard.get('RE_HEADERFILE',"[^/].*\\.h\\Z") + for afile in dumpfiles: if not afile[-5:] == '.dump': continue @@ -85,11 +279,11 @@ def process(dumpfiles, configfile, debugprint=False): data = cppcheckdata.CppcheckData(afile) # Check File naming - if "RE_FILE" in conf and conf["RE_FILE"]: + if conf.file: for source_file in data.files: basename = os.path.basename(source_file) good = False - for exp in conf["RE_FILE"]: + for exp in conf.file: good |= bool(re.match(exp, source_file)) good |= bool(re.match(exp, basename)) if not good: @@ -97,18 +291,23 @@ def process(dumpfiles, configfile, debugprint=False): reportNamingError(mockToken, 'File name ' + basename + ' violates naming convention') # Check Namespace naming - if "RE_NAMESPACE" in conf and conf["RE_NAMESPACE"]: + if conf.namespace: for tk in data.rawTokens: if tk.str == 'namespace': mockToken = DataStruct(tk.next.file, tk.next.linenr, tk.next.str) msgType = 'Namespace' - for exp in conf["RE_NAMESPACE"]: - evalExpr(conf["RE_NAMESPACE"], exp, mockToken, msgType) + for exp in conf.namespace: + evalExpr(conf.namespace, exp, mockToken, msgType) + + unguarded_include_files = [] + if conf.include_guard: + if conf.include_guard.get('required',1): + unguarded_include_files = [fn for fn in data.files if re.match(include_guard_header_re,fn)] for cfg in data.configurations: if not args.cli: print('Checking %s, config %s...' % (afile, cfg.name)) - if "RE_VARNAME" in conf and conf["RE_VARNAME"]: + if conf.variable: for var in cfg.variables: if var.nameToken and var.access != 'Global' and var.access != 'Public' and var.access != 'Private': prev = var.nameToken.previous @@ -126,10 +325,11 @@ def process(dumpfiles, configfile, debugprint=False): print("\n") print("\t-- {} {}".format(varType, str(var.nameToken.str))) - if conf["skip_one_char_variables"] and len(var.nameToken.str) == 1: + if conf.skip_one_char_variables and len(var.nameToken.str) == 1: continue - if varType in conf.get("var_prefixes",{}): - if not var.nameToken.str.startswith(conf["var_prefixes"][varType]): + if varType in conf.variable_prefixes: + prefix = conf.variable_prefixes[varType] + if not var.nameToken.str.startswith(prefix): reportNamingError(var.typeStartToken, 'Variable ' + var.nameToken.str + @@ -137,42 +337,42 @@ def process(dumpfiles, configfile, debugprint=False): mockToken = DataStruct(var.typeStartToken.file, var.typeStartToken.linenr, var.nameToken.str) msgType = 'Variable' - for exp in conf["RE_VARNAME"]: - evalExpr(conf["RE_VARNAME"], exp, mockToken, msgType) + for exp in conf.variable: + evalExpr(conf.variable, exp, mockToken, msgType) # Check Private Variable naming - if "RE_PRIVATE_MEMBER_VARIABLE" in conf and conf["RE_PRIVATE_MEMBER_VARIABLE"]: + if conf.private_member: # TODO: Not converted yet for var in cfg.variables: if (var.access is None) or var.access != 'Private': continue mockToken = DataStruct(var.typeStartToken.file, var.typeStartToken.linenr, var.nameToken.str) msgType = 'Private member variable' - for exp in conf["RE_PRIVATE_MEMBER_VARIABLE"]: - evalExpr(conf["RE_PRIVATE_MEMBER_VARIABLE"], exp, mockToken, msgType) + for exp in conf.private_member: + evalExpr(conf.private_member, exp, mockToken, msgType) # Check Public Member Variable naming - if "RE_PUBLIC_MEMBER_VARIABLE" in conf and conf["RE_PUBLIC_MEMBER_VARIABLE"]: + if conf.public_member: for var in cfg.variables: if (var.access is None) or var.access != 'Public': continue mockToken = DataStruct(var.typeStartToken.file, var.typeStartToken.linenr, var.nameToken.str) msgType = 'Public member variable' - for exp in conf["RE_PUBLIC_MEMBER_VARIABLE"]: - evalExpr(conf["RE_PUBLIC_MEMBER_VARIABLE"], exp, mockToken, msgType) + for exp in conf.public_member: + evalExpr(conf.public_member, exp, mockToken, msgType) # Check Global Variable naming - if "RE_GLOBAL_VARNAME" in conf and conf["RE_GLOBAL_VARNAME"]: + if conf.global_variable: for var in cfg.variables: if (var.access is None) or var.access != 'Global': continue mockToken = DataStruct(var.typeStartToken.file, var.typeStartToken.linenr, var.nameToken.str) msgType = 'Public member variable' - for exp in conf["RE_GLOBAL_VARNAME"]: - evalExpr(conf["RE_GLOBAL_VARNAME"], exp, mockToken, msgType) + for exp in conf.global_variable: + evalExpr(conf.global_variable, exp, mockToken, msgType) # Check Functions naming - if "RE_FUNCTIONNAME" in conf and conf["RE_FUNCTIONNAME"]: + if conf.function_name: for token in cfg.tokenlist: if token.function: if token.function.type in ('Constructor', 'Destructor', 'CopyConstructor', 'MoveConstructor'): @@ -185,23 +385,31 @@ def process(dumpfiles, configfile, debugprint=False): if debugprint: print("\t:: {} {}".format(retval, token.function.name)) - if retval and retval in conf.get("function_prefixes",{}): - if not token.function.name.startswith(conf["function_prefixes"][retval]): + if retval and retval in conf.function_prefixes: + if not token.function.name.startswith(conf.function_prefixes[retval]): reportNamingError(token, 'Function ' + token.function.name + ' violates naming convention') mockToken = DataStruct(token.file, token.linenr, token.function.name) msgType = 'Function' - for exp in conf["RE_FUNCTIONNAME"]: - evalExpr(conf["RE_FUNCTIONNAME"], exp, mockToken, msgType) + for exp in conf.function_name: + evalExpr(conf.function_name, exp, mockToken, msgType) # Check Class naming - if "RE_CLASS_NAME" in conf and conf["RE_CLASS_NAME"]: + if conf.class_name: for fnc in cfg.functions: # Check if it is Constructor/Destructor if fnc.type == 'Constructor' or fnc.type == 'Destructor': mockToken = DataStruct(fnc.tokenDef.file, fnc.tokenDef.linenr, fnc.name) msgType = 'Class ' + fnc.type - for exp in conf["RE_CLASS_NAME"]: - evalExpr(conf["RE_CLASS_NAME"], exp, mockToken, msgType) + for exp in conf.class_name: + evalExpr(conf.class_name, exp, mockToken, msgType) + + # Check include guard naming + if conf.include_guard: + check_include_guard(conf.include_guard,cfg,unguarded_include_files) + + for fn in unguarded_include_files: + mockToken = DataStruct(fn,0,os.path.basename(fn)) + reportNamingError(mockToken,'Missing include guard','includeGuardMissing') if __name__ == "__main__": parser = cppcheckdata.ArgumentParser() diff --git a/test/cli/test-other.py b/test/cli/test-other.py index e68e5ffd8..7a2e81772 100644 --- a/test/cli/test-other.py +++ b/test/cli/test-other.py @@ -346,6 +346,15 @@ def test_addon_namingng(tmpdir): "RE_VARNAME": ["[a-z][a-z0-9_]*[a-z0-9]\\Z"], "RE_GLOBAL_VARNAME": ["[a-z][a-z0-9_]*[a-z0-9]\\Z"], "RE_FUNCTIONNAME": ["[a-z][a-z0-9_]*[a-z0-9]\\Z"], + "include_guard": { + "input": "basename", + "prefix": "_", + "suffix": "", + "case": "upper", + "max_linenr": 5, + "RE_HEADERFILE": ".*\\.h\\Z", + "required": true + }, "var_prefixes": {"uint32_t": "ui32"}, "function_prefixes": {"uint16_t": "ui16", "uint32_t": "ui32"}, @@ -353,6 +362,12 @@ def test_addon_namingng(tmpdir): } """.replace('\\','\\\\')) + test_unguarded_include_file_basename = 'test_unguarded.h' + test_unguarded_include_file = os.path.join(tmpdir, test_unguarded_include_file_basename) + with open(test_unguarded_include_file, 'wt') as f: + f.write(""" +void InvalidFunctionUnguarded(); +""") test_include_file_basename = '_test.h' test_include_file = os.path.join(tmpdir, test_include_file_basename) @@ -364,8 +379,10 @@ def test_addon_namingng(tmpdir): void InvalidFunction(); extern int _invalid_extern_global; +#include "{}" + #endif -""") +""".format(test_unguarded_include_file)) test_file_basename = 'test_.c' test_file = os.path.join(tmpdir, test_file_basename) @@ -403,11 +420,18 @@ static int _invalid_static_global; lines = [line for line in stderr.splitlines() if line.strip() != '^' and line != ''] expect = [ '{}:0:0: style: File name {} violates naming convention [namingng-namingConvention]'.format(test_include_file,test_include_file_basename), + '{}:2:0: style: include guard naming violation; TEST_H != _TEST_H [namingng-includeGuardName]'.format(test_include_file), + '#ifndef TEST_H', '{}:5:0: style: Function InvalidFunction violates naming convention [namingng-namingConvention]'.format(test_include_file), 'void InvalidFunction();', '{}:6:0: style: Public member variable _invalid_extern_global violates naming convention [namingng-namingConvention]'.format(test_include_file), 'extern int _invalid_extern_global;', + '{}:0:0: style: File name {} violates naming convention [namingng-namingConvention]'.format(test_unguarded_include_file,test_unguarded_include_file_basename), + '{}:0:0: style: Missing include guard [namingng-includeGuardMissing]'.format(test_unguarded_include_file), + '{}:2:0: style: Function InvalidFunctionUnguarded violates naming convention [namingng-namingConvention]'.format(test_unguarded_include_file), + 'void InvalidFunctionUnguarded();', + '{}:0:0: style: File name {} violates naming convention [namingng-namingConvention]'.format(test_file,test_file_basename), '{}:7:0: style: Variable _invalid_arg violates naming convention [namingng-namingConvention]'.format(test_file), 'void valid_function2(int _invalid_arg);', @@ -432,6 +456,87 @@ static int _invalid_static_global; assert lines == expect +def test_addon_namingng_config(tmpdir): + addon_file = os.path.join(tmpdir, 'namingng.json') + addon_config_file = os.path.join(tmpdir, 'namingng.config.json') + with open(addon_file, 'wt') as f: + f.write(""" +{ + "script": "addons/namingng.py", + "args": [ + "--configfile=%s" + ] +} + """%(addon_config_file).replace('\\','\\\\')) + + with open(addon_config_file, 'wt') as f: + f.write(""" +{ + "RE_FILE": "[^/]*[a-z][a-z0-9_]*[a-z0-9]\\.c\\Z", + "RE_NAMESPACE": false, + "RE_VARNAME": ["+bad pattern","[a-z]_good_pattern\\Z","(parentheses?"], + "RE_PRIVATE_MEMBER_VARIABLE": "[a-z][a-z0-9_]*[a-z0-9]\\Z", + "RE_PUBLIC_MEMBER_VARIABLE": "[a-z][a-z0-9_]*[a-z0-9]\\Z", + "RE_GLOBAL_VARNAME": "[a-z][a-z0-9_]*[a-z0-9]\\Z", + "RE_FUNCTIONNAME": "[a-z][a-z0-9_]*[a-z0-9]\\Z", + "RE_CLASS_NAME": "[a-z][a-z0-9_]*[a-z0-9]\\Z", + "_comment1": "these should all be arrays, or null, or not set", + + "include_guard": true, + "var_prefixes": ["bad"], + "function_prefixes": false, + "_comment2": "these should all be dict", + + "skip_one_char_variables": "false", + "_comment3": "this should be bool", + + "RE_VAR_NAME": "typo" +} + """.replace('\\','\\\\')) + + test_file_basename = 'test.c' + test_file = os.path.join(tmpdir, test_file_basename) + with open(test_file, 'a') as f: + # only create the file + pass + + args = ['--addon='+addon_file, '--verbose', '--enable=all', test_file] + + exitcode, stdout, stderr = cppcheck(args) + assert exitcode == 0 + + lines = stdout.splitlines() + assert lines == [ + 'Checking {} ...'.format(test_file), + 'Defines:', + 'Undefines:', + 'Includes:', + 'Platform:native' + ] + lines = stderr.splitlines() + # ignore the first line, stating that the addon failed to run properly + lines.pop(0) + assert lines == [ + "Output:", + "config error: RE_FILE must be list (not str), or not set", + "config error: RE_NAMESPACE must be list (not bool), or not set", + "config error: include_guard must be dict (not bool), or not set", + "config error: item '+bad pattern' of 'RE_VARNAME' is not a valid regular expression: nothing to repeat at position 0", + "config error: item '(parentheses?' of 'RE_VARNAME' is not a valid regular expression: missing ), unterminated subpattern at position 0", + "config error: var_prefixes must be dict (not list), or not set", + "config error: RE_PRIVATE_MEMBER_VARIABLE must be list (not str), or not set", + "config error: RE_PUBLIC_MEMBER_VARIABLE must be list (not str), or not set", + "config error: RE_GLOBAL_VARNAME must be list (not str), or not set", + "config error: RE_FUNCTIONNAME must be list (not str), or not set", + "config error: function_prefixes must be dict (not bool), or not set", + "config error: RE_CLASS_NAME must be list (not str), or not set", + "config error: skip_one_char_variables must be bool (not str), or not set", + "config error: unknown config key 'RE_VAR_NAME' [internalError]", + "", + "^", + ] + + def test_addon_findcasts(tmpdir): test_file = os.path.join(tmpdir, 'test.cpp') with open(test_file, 'wt') as f: