From 3487a50faa6b94efeafae95193b52297df6d26ff Mon Sep 17 00:00:00 2001 From: Maarten van der Schrieck Date: Mon, 1 Jan 2024 12:35:43 +0100 Subject: [PATCH] addons/namingng.py: Add config sanity checks and unit test. The config file is parsed and (superficially) validated, before starting validation. Various errors are checked and reported, along with a non-zero exit status. After parsing and type validation, the config values are stored in an object, so they can be referenced as object members instead of dict keys (`conf.variable` instead of `conf["RE_VARNAME"]`). This separates config syntax from application code. A unit test is added to test config file validation. The example config JSON in namingng.py is fixed. --- addons/namingng.py | 177 ++++++++++++++++++++++++++++++----------- test/cli/test-other.py | 81 +++++++++++++++++++ 2 files changed, 210 insertions(+), 48 deletions(-) diff --git a/addons/namingng.py b/addons/namingng.py index e0bf0085122..fb852c1740f 100755 --- a/addons/namingng.py +++ b/addons/namingng.py @@ -12,9 +12,10 @@ # 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_", @@ -49,10 +50,89 @@ def __init__(self, file, linenr, string): 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): @@ -82,7 +162,7 @@ def evalExpr(conf, exp, mockToken, msgType): msg = msgType + ' ' + mockToken.str + ' violates naming convention' checkFalseRegex(mockToken, exp, msg) -def check_include_guard_name(conf_ig,cfg,directive): +def check_include_guard_name(conf_include_guard,cfg,directive): parts = directive.str.split() if len(parts) != 2: msg = 'syntax error' @@ -91,9 +171,9 @@ def check_include_guard_name(conf_ig,cfg,directive): guard_name = parts[1] filename = directive.file - if conf_ig.get('input','path') == 'basename': + if conf_include_guard.get('input','path') == 'basename': filename = os.path.basename(filename) - use_case = conf_ig.get('case','upper') + use_case = conf_include_guard.get('case','upper') if use_case == 'upper': filename = filename.upper() elif use_case == 'lower': @@ -105,20 +185,20 @@ def check_include_guard_name(conf_ig,cfg,directive): sys.exit(1) barename = re.sub('[^A-Za-z0-9]','_',filename).strip('_') - expect_guard_name = conf_ig.get('prefix','') + barename + conf_ig.get('suffix','') + 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_ig,cfg,unguarded_include_files): +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_ig.get('max_linenr', 5) + max_linenr = conf_include_guard.get('max_linenr', 5) def report(directive,msg,errorId): reportNamingError(directive,msg,errorId) @@ -144,7 +224,7 @@ def report_pending_ifndef(directive): continue if directive.linenr > max_linenr: - if phase == 0 and conf_ig.get('required',1): + 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 @@ -153,11 +233,11 @@ def report_pending_ifndef(directive): if phase == 0: # looking for '#ifndef FILE_H' if not directive.str.startswith('#ifndef'): - if conf_ig.get('required',1): + 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_ig,cfg,directive) + guard_name = check_include_guard_name(conf_include_guard,cfg,directive) if guard_name == None: phase = -1 continue @@ -187,9 +267,9 @@ def report_pending_ifndef(directive): def process(dumpfiles, configfile, debugprint=False): conf = loadConfig(configfile) - if "include_guard" in conf and conf["include_guard"]: + if conf.include_guard: global include_guard_header_re - include_guard_header_re = conf["include_guard"].get('RE_HEADERFILE',"[^/].*\\.h\\Z") + include_guard_header_re = conf.include_guard.get('RE_HEADERFILE',"[^/].*\\.h\\Z") for afile in dumpfiles: if not afile[-5:] == '.dump': @@ -199,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: @@ -211,23 +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 "include_guard" in conf and conf["include_guard"]: - if conf["include_guard"].get('required',1): + 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 @@ -245,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 + @@ -256,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'): @@ -304,27 +385,27 @@ 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 "include_guard" in conf and conf["include_guard"]: - check_include_guard(conf["include_guard"],cfg,unguarded_include_files) + 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)) diff --git a/test/cli/test-other.py b/test/cli/test-other.py index d2f42dff52b..7a2e8177235 100644 --- a/test/cli/test-other.py +++ b/test/cli/test-other.py @@ -456,6 +456,87 @@ def test_addon_namingng(tmpdir): 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: