From 733965704a702f5de03fbed8b7e52b87be19f375 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 27 Dec 2022 08:43:58 -0800 Subject: [PATCH] Start building some checking of diagnostic use. (#2487) In theory we're doing a central registry so that we can ensure there's at least one test for each. This isn't doing that, but I'm trying to validate that the central registry isn't leading to duplicates or abandoned checks (and catching a couple of each). --- .pre-commit-config.yaml | 10 ++ toolchain/diagnostics/check_diagnostics.py | 117 ++++++++++++++++++ toolchain/diagnostics/diagnostic_registry.def | 3 +- toolchain/parser/parser.cpp | 12 +- 4 files changed, 134 insertions(+), 8 deletions(-) create mode 100755 toolchain/diagnostics/check_diagnostics.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9c5c81ae9c34..445f5b70308e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -95,6 +95,16 @@ repos: entry: scripts/check_sha_filenames.py language: python files: ^.*/fuzzer_corpus/.*$ + - id: check-toolchain-diagnostics + name: Check toolchain diagnostics + entry: toolchain/diagnostics/check_diagnostics.py + language: python + files: | + (?x)^( + toolchain/.*\.cpp| + toolchain/.*\.h| + toolchain/diagnostics/diagnostic_registry\.def + )$ # Run linters last, as formatters and other checks may fix issues. - repo: local diff --git a/toolchain/diagnostics/check_diagnostics.py b/toolchain/diagnostics/check_diagnostics.py new file mode 100755 index 000000000000..b202c61e5241 --- /dev/null +++ b/toolchain/diagnostics/check_diagnostics.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 + +"""Checks diagnostic use. + +Validates that each diagnostic declared with CARBON_DIAGNOSTIC_KIND is +referenced by one (and only one) CARBON_DIAGNOSTIC. +""" + +__copyright__ = """ +Part of the Carbon Language project, under the Apache License v2.0 with LLVM +Exceptions. See /LICENSE for license information. +SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +""" + +import collections +from concurrent import futures +import itertools +from pathlib import Path +import os +import re +import sys +from typing import Dict, List, NamedTuple, Set + + +# Example or test diagnostics, ignored because they're expected to not pass. +IGNORED = set(["MyDiagnostic", "TestDiagnostic"]) + + +class Location(NamedTuple): + """A location for a diagnostic.""" + + def __str__(self) -> str: + return f"{str(self.path)}:{self.line}" + + path: Path + line: int + + +def load_diagnostic_kind() -> Set[str]: + """Returns the set of declared diagnostic kinds. + + This isn't validated for uniqueness because the compiler does that. + """ + path = Path("toolchain/diagnostics/diagnostic_registry.def") + content = path.read_text() + decls = set(re.findall(r"CARBON_DIAGNOSTIC_KIND\((.+)\)", content)) + return decls.difference(IGNORED) + + +def load_diagnostic_uses_in( + path: Path, +) -> Dict[str, List[Location]]: + """Returns the path's CARBON_DIAGNOSTIC uses.""" + content = path.read_text() + + # Keep a line cursor so that we don't keep re-scanning the file. + line = 1 + line_offset = 0 + + found: Dict[str, List[Location]] = collections.defaultdict(lambda: []) + for m in re.finditer(r"CARBON_DIAGNOSTIC\(\s*(\w+),", content): + diag = m.group(1) + if diag in IGNORED: + continue + line += content.count("\n", line_offset, m.start()) + line_offset = m.start() + found[diag].append(Location(path, line)) + return found + + +def load_diagnostic_uses() -> Dict[str, List[Location]]: + """Returns all CARBON_DIAGNOSTIC uses.""" + globs = itertools.chain( + *[Path("toolchain").glob(f"**/*.{ext}") for ext in ("h", "cpp")] + ) + with futures.ThreadPoolExecutor() as exec: + results = exec.map(load_diagnostic_uses_in, globs) + + found: Dict[str, List[Location]] = collections.defaultdict(lambda: []) + for result in results: + for diag, locations in result.items(): + found[diag].extend(locations) + return found + + +def check_uniqueness(uses: Dict[str, List[Location]]) -> bool: + """If any diagnostic is non-unique, prints an error and returns true.""" + has_errors = False + for diag in sorted(uses.keys()): + if len(uses[diag]) > 1: + print(f"Non-unique diagnostic {diag}:", file=sys.stderr) + for loc in uses[diag]: + print(f" - {loc}", file=sys.stderr) + has_errors = True + return has_errors + + +def check_unused(decls: Set[str], uses: Dict[str, List[Location]]) -> bool: + """If any diagnostic is unused, prints an error and returns true.""" + unused = decls.difference(uses.keys()) + for diag in sorted(unused): + print(f"Unused diagnostic: {diag}") + return False + + +def main() -> None: + # Run from the repo root. + os.chdir(Path(__file__).parent.parent.parent) + decls = load_diagnostic_kind() + uses = load_diagnostic_uses() + + if any([check_uniqueness(uses), check_unused(decls, uses)]): + exit(1) + + +if __name__ == "__main__": + main() diff --git a/toolchain/diagnostics/diagnostic_registry.def b/toolchain/diagnostics/diagnostic_registry.def index 75fca2a5947d..f312395b1b51 100644 --- a/toolchain/diagnostics/diagnostic_registry.def +++ b/toolchain/diagnostics/diagnostic_registry.def @@ -61,11 +61,9 @@ CARBON_DIAGNOSTIC_KIND(ExpectedStructLiteralField) CARBON_DIAGNOSTIC_KIND(ExpectedVariableDeclaration) CARBON_DIAGNOSTIC_KIND(ExpectedVariableName) CARBON_DIAGNOSTIC_KIND(OperatorRequiresParentheses) -CARBON_DIAGNOSTIC_KIND(StackLimitExceeded) CARBON_DIAGNOSTIC_KIND(UnaryOperatorHasWhitespace) CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresWhitespace) CARBON_DIAGNOSTIC_KIND(UnexpectedTokenAfterListElement) -CARBON_DIAGNOSTIC_KIND(UnexpectedTokenInCodeBlock) CARBON_DIAGNOSTIC_KIND(UnrecognizedDeclaration) // Package-related diagnostics. @@ -77,6 +75,7 @@ CARBON_DIAGNOSTIC_KIND(ExpectedSemiToEndPackageDirective) // For-specific diagnostics. CARBON_DIAGNOSTIC_KIND(ExpectedIn) +CARBON_DIAGNOSTIC_KIND(ExpectedInNotColon) // Interface-specific diagnostics. CARBON_DIAGNOSTIC_KIND(ExpectedInterfaceName) diff --git a/toolchain/parser/parser.cpp b/toolchain/parser/parser.cpp index 2f4fc62f57e5..65ae76458c92 100644 --- a/toolchain/parser/parser.cpp +++ b/toolchain/parser/parser.cpp @@ -28,6 +28,9 @@ CARBON_DIAGNOSTIC(ExpectedParenAfter, Error, "Expected `(` after `{0}`.", CARBON_DIAGNOSTIC(ExpectedSemiAfterExpression, Error, "Expected `;` after expression."); +CARBON_DIAGNOSTIC(UnrecognizedDeclaration, Error, + "Unrecognized declaration introducer."); + // A relative location for characters in errors. enum class RelativeLocation : int8_t { Around, @@ -738,8 +741,6 @@ auto Parser::HandleDeclarationLoopState() -> void { break; } default: { - CARBON_DIAGNOSTIC(UnrecognizedDeclaration, Error, - "Unrecognized declaration introducer."); emitter_->Emit(*position_, UnrecognizedDeclaration); auto cursor = *position_; auto semi = SkipPastLikelyEnd(cursor); @@ -1236,8 +1237,6 @@ auto Parser::HandleInterfaceDefinitionLoopState() -> void { break; } default: { - CARBON_DIAGNOSTIC(UnrecognizedDeclaration, Error, - "Unrecognized declaration introducer."); emitter_->Emit(*position_, UnrecognizedDeclaration); if (auto semi = SkipPastLikelyEnd(*position_)) { AddLeafNode(ParseNodeKind::EmptyDeclaration(), *semi, @@ -1851,8 +1850,9 @@ auto Parser::HandleVarFinishAsForState() -> void { if (PositionIs(TokenKind::In())) { end_token = Consume(); } else if (PositionIs(TokenKind::Colon())) { - CARBON_DIAGNOSTIC(ExpectedIn, Error, "`:` should be replaced by `in`."); - emitter_->Emit(*position_, ExpectedIn); + CARBON_DIAGNOSTIC(ExpectedInNotColon, Error, + "`:` should be replaced by `in`."); + emitter_->Emit(*position_, ExpectedInNotColon); state.has_error = true; end_token = Consume(); } else {