diff options
Diffstat (limited to 'python/tidy/servo_tidy/tidy.py')
| -rw-r--r-- | python/tidy/servo_tidy/tidy.py | 166 |
1 files changed, 85 insertions, 81 deletions
diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py index 5b66d2911e4..093e8f3aeee 100644 --- a/python/tidy/servo_tidy/tidy.py +++ b/python/tidy/servo_tidy/tidy.py @@ -19,19 +19,21 @@ import subprocess import sys from licenseck import licenses +# License and header checks +EMACS_HEADER = "/* -*- Mode:" +VIM_HEADER = "/* vim:" +MAX_LICENSE_LINESPAN = max(len(license.splitlines()) for license in licenses) + # File patterns to include in the non-WPT tidy check. -file_patterns_to_check = ["*.rs", "*.rc", "*.cpp", "*.c", +FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c", "*.h", "Cargo.lock", "*.py", "*.toml", "*.webidl", "*.json"] # File patterns that are ignored for all tidy and lint checks. -file_patterns_to_ignore = [ - "*.#*", - "*.pyc", -] +FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc"] # Files that are ignored for all tidy and lint checks. -ignored_files = [ +IGNORED_FILES = [ # Generated and upstream code combined with our own. Could use cleanup os.path.join(".", "ports", "geckolib", "gecko_bindings", "bindings.rs"), os.path.join(".", "ports", "geckolib", "gecko_bindings", "structs_debug.rs"), @@ -41,12 +43,15 @@ ignored_files = [ os.path.join(".", "tests", "wpt", "metadata", "MANIFEST.json"), os.path.join(".", "tests", "wpt", "metadata-css", "MANIFEST.json"), os.path.join(".", "components", "script", "dom", "webidls", "ForceTouchEvent.webidl"), + # FIXME(pcwalton, #11679): This is a workaround for a tidy error on the quoted string + # `"__TEXT,_info_plist"` inside an attribute. + os.path.join(".", "components", "servo", "platform", "macos", "mod.rs"), # Hidden files os.path.join(".", "."), ] # Directories that are ignored for the non-WPT tidy check. -ignored_dirs = [ +IGNORED_DIRS = [ # Upstream os.path.join(".", "support", "android", "apk"), os.path.join(".", "support", "rust-task_info"), @@ -65,13 +70,35 @@ ignored_dirs = [ # Generated and upstream code combined with our own. Could use cleanup os.path.join(".", "target"), os.path.join(".", "ports", "cef"), - # Tooling, generated locally from external repos. - os.path.join(".", "ports", "geckolib", "gecko_bindings", "tools"), # Hidden directories os.path.join(".", "."), ] -spec_base_path = "components/script/dom/" +SPEC_BASE_PATH = "components/script/dom/" + +WEBIDL_STANDARDS = [ + "//www.khronos.org/registry/webgl/specs", + "//developer.mozilla.org/en-US/docs/Web/API", + "//dev.w3.org/2006/webapi", + "//dev.w3.org/csswg", + "//dev.w3.org/fxtf", + "//dvcs.w3.org/hg", + "//dom.spec.whatwg.org", + "//domparsing.spec.whatwg.org", + "//drafts.csswg.org/cssom", + "//drafts.fxtf.org", + "//encoding.spec.whatwg.org", + "//html.spec.whatwg.org", + "//url.spec.whatwg.org", + "//xhr.spec.whatwg.org", + "//w3c.github.io", + "//heycam.github.io/webidl", + "//webbluetoothcg.github.io/web-bluetooth/", + "//slightlyoff.github.io/ServiceWorker/spec/service_worker/", + # Not a URL + "// This interface is entirely internal to Servo, and should not be" + + " accessible to\n// web pages." +] def is_iter_empty(iterator): @@ -94,16 +121,16 @@ def progress_wrapper(iterator): def filter_file(file_name): - if any(file_name.startswith(ignored_file) for ignored_file in ignored_files): + if any(file_name.startswith(ignored_file) for ignored_file in IGNORED_FILES): return False base_name = os.path.basename(file_name) - if any(fnmatch.fnmatch(base_name, pattern) for pattern in file_patterns_to_ignore): + if any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_IGNORE): return False return True def filter_files(start_dir, only_changed_files, progress): - file_iter = get_file_list(start_dir, only_changed_files, ignored_dirs) + file_iter = get_file_list(start_dir, only_changed_files, IGNORED_DIRS) (has_element, file_iter) = is_iter_empty(file_iter) if not has_element: raise StopIteration @@ -111,18 +138,13 @@ def filter_files(start_dir, only_changed_files, progress): file_iter = progress_wrapper(file_iter) for file_name in file_iter: base_name = os.path.basename(file_name) - if not any(fnmatch.fnmatch(base_name, pattern) for pattern in file_patterns_to_check): + if not any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_CHECK): continue if not filter_file(file_name): continue yield file_name -EMACS_HEADER = "/* -*- Mode:" -VIM_HEADER = "/* vim:" -MAX_LICENSE_LINESPAN = max(len(license.splitlines()) for license in licenses) - - def check_license(file_name, lines): if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json")): raise StopIteration @@ -270,13 +292,6 @@ duplicate versions for package "{package}" yield (1, message) -def maybe_int(value): - try: - return int(value) - except ValueError: - return value - - def check_toml(file_name, lines): if not file_name.endswith(".toml"): raise StopIteration @@ -292,9 +307,9 @@ def check_rust(file_name, lines): file_name.endswith(os.path.join("geckolib", "build.rs")) or \ file_name.endswith(os.path.join("unit", "style", "stylesheets.rs")): raise StopIteration + comment_depth = 0 merged_lines = '' - import_block = False whitespace = False @@ -311,6 +326,8 @@ def check_rust(file_name, lines): for idx, original_line in enumerate(lines): # simplify the analysis line = original_line.strip() + is_attribute = re.search(r"#\[.*\]", line) + is_comment = re.search(r"^//|^/\*|^\*", line) # Simple heuristic to avoid common case of no comments. if '/' in line: @@ -328,17 +345,17 @@ def check_rust(file_name, lines): merged_lines = '' # Keep track of whitespace to enable checking for a merged import block - # + # Ignore attributes, comments, and imports if import_block: - if not (line_is_comment(line) or line_is_attribute(line) or line.startswith("use ")): + if not (is_comment or is_attribute or line.startswith("use ")): whitespace = line == "" if not whitespace: import_block = False # get rid of strings and chars because cases like regex expression, keep attributes - if not line_is_attribute(line): + if not is_attribute: line = re.sub(r'"(\\.|[^\\"])*?"', '""', line) line = re.sub(r"'(\\.|[^\\'])*?'", "''", line) @@ -352,21 +369,22 @@ def check_rust(file_name, lines): # tuple format: (pattern, format_message, filter_function(match, line)) no_filter = lambda match, line: True regex_rules = [ - (r",[^\s]", "missing space after ,", lambda match, line: '$' not in line), + (r",[^\s]", "missing space after ,", + lambda match, line: '$' not in line and not is_attribute), (r"[A-Za-z0-9\"]=", "missing space before =", - lambda match, line: line_is_attribute(line)), + lambda match, line: is_attribute), (r"=[A-Za-z0-9\"]", "missing space after =", - lambda match, line: line_is_attribute(line)), + lambda match, line: is_attribute), # ignore scientific notation patterns like 1e-6 (r"[A-DF-Za-df-z0-9]-", "missing space before -", - lambda match, line: not line_is_attribute(line)), + lambda match, line: not is_attribute), (r"[A-Za-z0-9]([\+/\*%=])", "missing space before {0}", - lambda match, line: (not line_is_attribute(line) and + lambda match, line: (not is_attribute and not is_associated_type(match, line))), # * not included because of dereferencing and casting # - not included because of unary negation (r'([\+/\%=])[A-Za-z0-9"]', "missing space after {0}", - lambda match, line: (not line_is_attribute(line) and + lambda match, line: (not is_attribute and not is_associated_type(match, line))), (r"\)->", "missing space before ->", no_filter), (r"->[A-Za-z]", "missing space after ->", no_filter), @@ -390,17 +408,18 @@ def check_rust(file_name, lines): (r": &Vec<", "use &[T] instead of &Vec<T>", no_filter), # No benefit over using &str (r": &String", "use &str instead of &String", no_filter), + # No benefit to using &Root<T> + (r": &Root<", "use &T instead of &Root<T>", no_filter), (r"^&&", "operators should go at the end of the first line", no_filter), (r"\{[A-Za-z0-9_]+\};", "use statement contains braces for single import", lambda match, line: line.startswith('use ')), + (r"^\s*else {", "else braces should be on the same line", no_filter), ] for pattern, message, filter_func in regex_rules: for match in re.finditer(pattern, line): - if not filter_func(match, line): - continue - - yield (idx + 1, message.format(*match.groups(), **match.groupdict())) + if filter_func(match, line): + yield (idx + 1, message.format(*match.groups(), **match.groupdict())) if prev_open_brace and not line: yield (idx + 1, "found an empty line following a {") @@ -476,14 +495,6 @@ def is_associated_type(match, line): return generic_open and generic_close -def line_is_attribute(line): - return re.search(r"#\[.*\]", line) - - -def line_is_comment(line): - return re.search(r"^//|^/\*|^\*", line) - - def check_webidl_spec(file_name, contents): # Sorted by this function (in pseudo-Rust). The idea is to group the same # organization together. @@ -503,35 +514,24 @@ def check_webidl_spec(file_name, contents): # } # a_domain.path().cmp(b_domain.path()) # } + if not file_name.endswith(".webidl"): raise StopIteration - standards = [ - "//www.khronos.org/registry/webgl/specs", - "//developer.mozilla.org/en-US/docs/Web/API", - "//dev.w3.org/2006/webapi", - "//dev.w3.org/csswg", - "//dev.w3.org/fxtf", - "//dvcs.w3.org/hg", - "//dom.spec.whatwg.org", - "//domparsing.spec.whatwg.org", - "//drafts.csswg.org/cssom", - "//drafts.fxtf.org", - "//encoding.spec.whatwg.org", - "//html.spec.whatwg.org", - "//url.spec.whatwg.org", - "//xhr.spec.whatwg.org", - "//w3c.github.io", - "//heycam.github.io/webidl", - "//webbluetoothcg.github.io/web-bluetooth/", - "//slightlyoff.github.io/ServiceWorker/spec/service_worker/", - # Not a URL - "// This interface is entirely internal to Servo, and should not be" + - " accessible to\n// web pages." - ] - for i in standards: + + for i in WEBIDL_STANDARDS: if contents.find(i) != -1: raise StopIteration - yield 0, "No specification link found." + yield (0, "No specification link found.") + + +def check_for_possible_duplicate_json_keys(key_value_pairs): + keys = [x[0] for x in key_value_pairs] + seen_keys = set() + for key in keys: + if key in seen_keys: + raise KeyError(key) + + seen_keys.add(key) def check_json(filename, contents): @@ -539,17 +539,19 @@ def check_json(filename, contents): raise StopIteration try: - json.loads(contents) + json.loads(contents, object_pairs_hook=check_for_possible_duplicate_json_keys) except ValueError as e: match = re.search(r"line (\d+) ", e.message) line_no = match and match.group(1) yield (line_no, e.message) + except KeyError as e: + yield (None, "Duplicated Key (%s)" % e.message) def check_spec(file_name, lines): - if spec_base_path not in file_name: + if SPEC_BASE_PATH not in file_name: raise StopIteration - file_name = os.path.relpath(os.path.splitext(file_name)[0], spec_base_path) + file_name = os.path.relpath(os.path.splitext(file_name)[0], SPEC_BASE_PATH) patt = re.compile("^\s*\/\/.+") # Pattern representing a line with a macro @@ -561,9 +563,10 @@ def check_spec(file_name, lines): # Pattern representing a line with comment comment_patt = re.compile("^\s*///?.+$") - pattern = "impl {}Methods for {} {{".format(file_name, file_name) brace_count = 0 in_impl = False + pattern = "impl {}Methods for {} {{".format(file_name, file_name) + for idx, line in enumerate(lines): if "// check-tidy: no specs after this line" in line: break @@ -594,10 +597,10 @@ def collect_errors_for_files(files_to_check, checking_functions, line_checking_f raise StopIteration if print_text: print '\rChecking files for tidiness...' + for filename in files_to_check: if not os.path.exists(filename): continue - with open(filename, "r") as f: contents = f.read() for check in checking_functions: @@ -629,7 +632,7 @@ def check_wpt_lint_errors(files): if os.path.isdir(wpt_working_dir): site.addsitedir(wpt_working_dir) from tools.lint import lint - returncode = lint.lint(files) + returncode = lint.lint(wpt_working_dir, files, output_json=False) if returncode: yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status {0}".format(returncode)) @@ -645,11 +648,12 @@ def get_file_list(directory, only_changed_files=False, exclude_dirs=[]): args = ["git", "ls-files", "--others", "--exclude-standard", directory] file_list += subprocess.check_output(args) for f in file_list.splitlines(): - yield os.path.join('.', f) + if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in exclude_dirs): + yield os.path.join('.', f) elif exclude_dirs: for root, dirs, files in os.walk(directory, topdown=True): # modify 'dirs' in-place so that we don't do unwanted traversals in excluded directories - dirs[:] = [d for d in dirs if not any(os.path.join(root, d).startswith(name) for name in ignored_dirs)] + dirs[:] = [d for d in dirs if not any(os.path.join(root, d).startswith(name) for name in exclude_dirs)] for rel_path in files: yield os.path.join(root, rel_path) else: |