Refactor creating set of files with warnings to a dedicated function

python · hugovk · Aug 6, 2024 · Jul 13, 2024 · Jul 13, 2024 · Jul 13, 2024
commit bd1634e3c3504b4b79423c9f65a50bbf30205241
diff --git a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst
@@ -1 +1,2 @@
 Add tooling to check for changes in compiler warnings.
+Patch by Nate Ohlson
diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py
@@ -3,11 +3,11 @@
 Parses compiler output with -fdiagnostics-format=json and checks that warnings exist
 only in files that are expected to have warnings.
 """
-import sys
-from pathlib import Path
 import argparse
 import json
 import re
+import sys
+from pathlib import Path
 
 def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]:
     """
@@ -24,63 +24,67 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]:
         try:
             json_data = json.loads(array)
             json_objects_in_array = [entry for entry in json_data]
-            compiler_warnings.extend([entry for entry in json_objects_in_array if entry.get('kind') == 'warning'])
+            compiler_warnings.extend([entry for entry in json_objects_in_array
+                                       if entry.get('kind') == 'warning'])
         except json.JSONDecodeError:
             continue  # Skip malformed JSON
 
     return compiler_warnings
 
+# Create a function that returns a dictionary where the key is the file and the data is the warnings in that file
+def get_warnings_by_file(warnings: list[dict]) -> dict[str, list[dict]]:
+    """
+    Returns a dictionary where the key is the file and the data is the warnings in that file
+    """
+    warnings_by_file = {}
+    for warning in warnings:
+        locations = warning['locations']
+        for location in locations:
+            for key in ['caret', 'start', 'end']:
+                if key in location:
+                    file = location[key]['file']
+                    file = file.lstrip('./') # Remove leading current directory if present
+                    if file not in warnings_by_file:
+                        warnings_by_file[file] = []
+                    warnings_by_file[file].append(warning)
+
+    return warnings_by_file
+
 def get_unexpected_warnings(
     warnings: list[dict],
     files_with_expected_warnings: set[str],
+    files_with_warnings: set[str],
 ) -> int:
     """
     Returns failure status if warnings discovered in list of warnings are associated with a file
     that is not found in the list of files with expected warnings
     """
     unexpected_warnings = []
-    for warning in warnings:
-        locations = warning['locations']
-        for location in locations:
-            for key in ['caret', 'start', 'end']:
-                if key in location:
-                    file = location[key]['file']
-                    file = file.lstrip('./') # Remove leading curdir if present
-                    if file not in files_with_expected_warnings:
-                        unexpected_warnings.append(warning)
+    for file in files_with_warnings.keys():
+        if file not in files_with_expected_warnings:
+            unexpected_warnings.extend(files_with_warnings[file])
 
     if unexpected_warnings:
         print("Unexpected warnings:")
         for warning in unexpected_warnings:
             print(warning)
         return 1
-
+    
     return 0
 
 
 def get_unexpected_improvements(
     warnings: list[dict],
     files_with_expected_warnings: set[str],
+    files_with_warnings: set[str],
 ) -> int:
     """
     Returns failure status if there are no warnings in the list of warnings for a file
     that is in the list of files with expected warnings
     """
-
-    # Create set of files with warnings
-    files_with_warnings = set()
-    for warning in warnings:
-        locations = warning['locations']
-        for location in locations:
-            for key in ['caret', 'start', 'end']:
-                if key in location:
-                    file = location[key]['file']
-                    file = file.lstrip('./') # Remove leading curdir if present
-                    files_with_warnings.add(file)
-
     unexpected_improvements = []
     for file in files_with_expected_warnings:
-        if file not in files_with_warnings:
+        if file not in files_with_warnings.keys():
             unexpected_improvements.append(file)
 
     if unexpected_improvements:
@@ -143,11 +147,12 @@ def main(argv: list[str] | None = None) -> int:
         }
 
     warnings = extract_warnings_from_compiler_output(compiler_output_file_contents)
+    files_with_warnings = get_warnings_by_file(warnings)
 
-    status = get_unexpected_warnings(warnings, files_with_expected_warnings)
+    status = get_unexpected_warnings(warnings, files_with_expected_warnings, files_with_warnings)
     if args.fail_on_regression: exit_code |= status
 
-    status = get_unexpected_improvements(warnings, files_with_expected_warnings)
+    status = get_unexpected_improvements(warnings, files_with_expected_warnings, files_with_warnings)
     if args.fail_on_improvement: exit_code |= status
 
     return exit_code
Original file line number	Diff line number	Diff line change
		@@ -1 +1,2 @@
		Add tooling to check for changes in compiler warnings.
		Patch by Nate Ohlson