From 60fb5b411db8938981b49590c2572e0986c388b6 Mon Sep 17 00:00:00 2001 From: Hugo Guerrier Date: Fri, 18 Oct 2024 10:14:56 +0200 Subject: [PATCH] Enhance error messages when parsing an LKQL rule file --- lkql_checker/src/gnatcheck-compiler.adb | 18 +++-- lkql_checker/src/gnatcheck-compiler.ads | 3 +- .../src/gnatcheck-rules-rule_table.adb | 34 ++++----- .../com/adacore/lkql_jit/GNATCheckWorker.java | 73 ++++++++++++++----- .../lkql_jit/checker/utils/CheckerUtils.java | 45 +++++++++--- .../invalid_syntax.lkql | 3 + .../invalid_lkql_rules_config/test.out | 24 ++++-- .../invalid_lkql_rules_config/test.yaml | 2 + 8 files changed, 139 insertions(+), 63 deletions(-) create mode 100644 testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/invalid_syntax.lkql diff --git a/lkql_checker/src/gnatcheck-compiler.adb b/lkql_checker/src/gnatcheck-compiler.adb index fd9092304..8513d2c9d 100644 --- a/lkql_checker/src/gnatcheck-compiler.adb +++ b/lkql_checker/src/gnatcheck-compiler.adb @@ -442,7 +442,7 @@ package body Gnatcheck.Compiler is else Get_Rule_Id (Message_Kind))); end Analyze_Line; - -- Start of processing for Analyze_Builder_Output + -- Start of processing for Analyze_Output begin Errors := False; @@ -512,12 +512,15 @@ package body Gnatcheck.Compiler is end if; end; end if; - elsif Line_Len >= 20 - and then Line (1 .. 20) = "WORKER_FATAL_ERROR: " + elsif Line_Len >= 16 + and then Line (1 .. 16) = "WORKER_WARNING: " then - Error ("error raised by the worker: " & Line (21 .. Line_Len)); + Warning (Line (17 .. Line_Len)); + elsif Line_Len >= 14 + and then Line (1 .. 14) = "WORKER_ERROR: " + then + Error (Line (15 .. Line_Len)); Errors := True; - return; else Analyze_Line (Line (1 .. Line_Len)); end if; @@ -1692,8 +1695,7 @@ package body Gnatcheck.Compiler is function Spawn_LKQL_Rule_File_Parser (LKQL_RF_Name : String; - Result_File : String; - Error_File : String) return Process_Id + Result_File : String) return Process_Id is use GNAT.String_Split; @@ -1737,7 +1739,7 @@ package body Gnatcheck.Compiler is -- Spawn the process and return the associated process ID Pid := Non_Blocking_Spawn - (Worker.all, Args (1 .. Num_Args), Result_File, Error_File); + (Worker.all, Args (1 .. Num_Args), Result_File); for J in Args'Range loop Free (Args (J)); diff --git a/lkql_checker/src/gnatcheck-compiler.ads b/lkql_checker/src/gnatcheck-compiler.ads index 3403dba74..7140cc0e8 100644 --- a/lkql_checker/src/gnatcheck-compiler.ads +++ b/lkql_checker/src/gnatcheck-compiler.ads @@ -170,8 +170,7 @@ package Gnatcheck.Compiler is function Spawn_LKQL_Rule_File_Parser (LKQL_RF_Name : String; - Result_File : String; - Error_File : String) return Process_Id; + Result_File : String) return Process_Id; -- Spawn the executable which handles the LKQL rule config file parsing -- with the provided `LKQL_RF_Name` then return the process identifier -- associated to it. Redirects all output made by the process in the diff --git a/lkql_checker/src/gnatcheck-rules-rule_table.adb b/lkql_checker/src/gnatcheck-rules-rule_table.adb index dec6d191a..4bc48c88b 100644 --- a/lkql_checker/src/gnatcheck-rules-rule_table.adb +++ b/lkql_checker/src/gnatcheck-rules-rule_table.adb @@ -805,8 +805,6 @@ package body Gnatcheck.Rules.Rule_Table is Get_Rule_File_Name (LKQL_RF_Name); JSON_Config_File_Name : constant String := Global_Report_Dir.all & "gnatcheck-rules.json.out"; - Error_File_Name : constant String := - Global_Report_Dir.all & "gnatcheck-rules.json.err"; Parser_Pid : Process_Id; Waited_Pid : Process_Id; Success : Boolean; @@ -836,30 +834,30 @@ package body Gnatcheck.Rules.Rule_Table is -- Call the LKQL rule config file parser and parse its result Parser_Pid := Spawn_LKQL_Rule_File_Parser - (Rule_File_Absolute_Path, JSON_Config_File_Name, Error_File_Name); + (Rule_File_Absolute_Path, JSON_Config_File_Name); Wait_Process (Waited_Pid, Success); if Parser_Pid /= Waited_Pid or else not Success then Error ("can not call the LKQL rule file parser"); Rule_Option_Problem_Detected := True; - return; - end if; - Analyze_Output (Error_File_Name, Success); + else + Config_JSON := Read (Read_File (JSON_Config_File_Name).all); - Config_JSON := Read (Read_File (JSON_Config_File_Name).all); - if not Config_JSON.Success then - Error ("can not parse the rule config JSON file"); - Rule_Option_Problem_Detected := True; - return; - end if; + -- If the JSON parsing failed, it means that LKQL rule file + -- processing failed and diagnostics are in the output file. + if not Config_JSON.Success then + Analyze_Output (JSON_Config_File_Name, Success); + Rule_Option_Problem_Detected := True; - -- Populate the global rule table with the rule config - Map_JSON_Object (Config_JSON.Value, Rule_Object_Mapper'Access); + -- Else, populate the global rule table with the rule config + else + Map_JSON_Object (Config_JSON.Value, Rule_Object_Mapper'Access); + end if; - -- Delete the temporary JSON files if not it debug mode - if not Arg.Debug_Mode.Get then - Delete_File (JSON_Config_File_Name, Success); - Delete_File (Error_File_Name, Success); + -- Delete the temporary JSON files if not it debug mode + if not Arg.Debug_Mode.Get then + Delete_File (JSON_Config_File_Name, Success); + end if; end if; end Process_LKQL_Rule_File; diff --git a/lkql_jit/cli/src/main/java/com/adacore/lkql_jit/GNATCheckWorker.java b/lkql_jit/cli/src/main/java/com/adacore/lkql_jit/GNATCheckWorker.java index a85de5fad..db700699f 100644 --- a/lkql_jit/cli/src/main/java/com/adacore/lkql_jit/GNATCheckWorker.java +++ b/lkql_jit/cli/src/main/java/com/adacore/lkql_jit/GNATCheckWorker.java @@ -185,13 +185,19 @@ protected int executeScript(Context.Builder contextBuilder) { // If a LKQL rule config file has been provided, parse it and display the result if (this.args.lkqlConfigFile != null) { - System.out.println( - new JSONObject( - parseLKQLRuleFile(this.args.lkqlConfigFile).entrySet().stream() - .map(e -> Map.entry(e.getKey(), e.getValue().toJson())) - .collect( - Collectors.toMap( - Map.Entry::getKey, Map.Entry::getValue)))); + try { + final var instances = parseLKQLRuleFile(this.args.lkqlConfigFile); + final var jsonInstances = + new JSONObject( + instances.entrySet().stream() + .map(e -> Map.entry(e.getKey(), e.getValue().toJson())) + .collect( + Collectors.toMap( + Map.Entry::getKey, Map.Entry::getValue))); + System.out.println(jsonInstances); + } catch (LKQLRuleFileError e) { + System.out.println(e.getMessage()); + } return 0; } @@ -222,7 +228,12 @@ protected int executeScript(Context.Builder contextBuilder) { final Map instances = new HashMap<>(); for (var rulesFrom : this.args.rulesFroms) { if (!rulesFrom.isEmpty()) { - instances.putAll(parseLKQLRuleFile(rulesFrom)); + try { + instances.putAll(parseLKQLRuleFile(rulesFrom)); + } catch (LKQLRuleFileError e) { + System.out.println(e.getMessage()); + return 0; + } } } optionsBuilder.ruleInstances(instances); @@ -237,7 +248,7 @@ protected int executeScript(Context.Builder contextBuilder) { executable.executeVoid(true); return 0; } catch (Exception e) { - System.out.println("WORKER_FATAL_ERROR: " + e.getMessage()); + System.out.println(e.getMessage()); return 0; } } @@ -253,16 +264,28 @@ protected List preprocessArguments( // ----- Option parsing helpers ----- - /** Emit a formatted error when there is an invalid LKQL rule file. */ - private static void errorInLKQLRuleFile(final String lkqlRuleFile, final String message) { - System.err.println("WORKER_FATAL_ERROR: " + message + " (" + lkqlRuleFile + ")"); + /** Throws an exception with the given message, related ot the provided LKQL file name. */ + private static void errorInLKQLRuleFile(final String lkqlRuleFile, final String message) + throws LKQLRuleFileError { + errorInLKQLRuleFile(lkqlRuleFile, message, true); + } + + private static void errorInLKQLRuleFile( + final String lkqlRuleFile, final String message, final boolean addTag) + throws LKQLRuleFileError { + throw new LKQLRuleFileError( + (addTag ? "WORKER_ERROR: " : "") + message + " (" + lkqlRuleFile + ")"); } /** - * Read the given LKQL file and parse it as a rule configuration file to return the extracted - * instances. + * Read the given LKQL file and parse it as a rule configuration file to return the list of + * instances defined in it. + * + * @throws LKQLRuleFileError If there is any error in the provided LKQL rule file, preventing + * the analysis to go further. */ - private static Map parseLKQLRuleFile(final String lkqlRuleFileName) { + private static Map parseLKQLRuleFile(final String lkqlRuleFileName) + throws LKQLRuleFileError { final File lkqlFile = new File(lkqlRuleFileName); final String lkqlFileBasename = lkqlFile.getName(); final Map res = new HashMap<>(); @@ -313,9 +336,10 @@ private static Map parseLKQLRuleFile(final String lkqlRule } } catch (IOException e) { errorInLKQLRuleFile(lkqlFileBasename, "Could not read file"); + } catch (LKQLRuleFileError e) { + throw e; } catch (Exception e) { - errorInLKQLRuleFile( - lkqlFileBasename, "Error during file processing: " + e.getMessage()); + errorInLKQLRuleFile(lkqlFileBasename, e.getMessage(), false); } return res; } @@ -328,7 +352,8 @@ private static void processInstancesObject( final String lkqlRuleFile, final Value instancesObject, final RuleInstance.SourceMode sourceMode, - final Map toPopulate) { + final Map toPopulate) + throws LKQLRuleFileError { // Iterate on all instance object keys for (String ruleName : instancesObject.getMemberKeys()) { final String lowerRuleName = ruleName.toLowerCase(); @@ -385,7 +410,8 @@ private static void processArgsObject( final Value argsObject, final RuleInstance.SourceMode sourceMode, final String ruleName, - final Map toPopulate) { + final Map toPopulate) + throws LKQLRuleFileError { // Ensure that the given value has members (is an object) if (!argsObject.hasMembers()) { errorInLKQLRuleFile(lkqlRuleFile, "Arguments should be in an object value"); @@ -439,6 +465,15 @@ private static boolean acceptSoleArgs(final String ruleName) { return List.of("style_checks", "warnings").contains(ruleName); } + // ----- Inner classes ----- + + /** An exception to throw while analysing an LKQL rule file. */ + static final class LKQLRuleFileError extends Exception { + public LKQLRuleFileError(String message) { + super(message); + } + } + // ----- The LKQL checker ----- public static final String checkerSource = diff --git a/lkql_jit/language/src/main/java/com/adacore/lkql_jit/checker/utils/CheckerUtils.java b/lkql_jit/language/src/main/java/com/adacore/lkql_jit/checker/utils/CheckerUtils.java index 8b1ac10ef..72516fd34 100644 --- a/lkql_jit/language/src/main/java/com/adacore/lkql_jit/checker/utils/CheckerUtils.java +++ b/lkql_jit/language/src/main/java/com/adacore/lkql_jit/checker/utils/CheckerUtils.java @@ -280,19 +280,34 @@ public String diagnostic( SourceLocation adaErrorLocation, SourceLocation lkqlErrorLocation, String ruleName) { + // If there is an Ada location for this diagnostic, emit it as a GNU formatted one + if (adaErrorLocation != null) { + var adaLoc = adaErrorLocation.display(true) + ": "; + var lkqlLoc = + lkqlErrorLocation != null + ? "internal issue at " + lkqlErrorLocation.display(true) + ": " + : ""; + var rulePart = + ruleName == null || ruleName.isBlank() + ? "" + : " [" + ruleName.toLowerCase() + "]"; + + return adaLoc + kindtoString(messageKind) + ": " + lkqlLoc + message + rulePart; + } - var adaLoc = - adaErrorLocation != null ? adaErrorLocation.display(true) + ": " : "null:0:0: "; - var lkqlLoc = - lkqlErrorLocation != null - ? "internal issue at " + lkqlErrorLocation.display(true) + ": " - : ""; - var rulePart = - ruleName == null || ruleName.isBlank() - ? "" - : " [" + ruleName.toLowerCase() + "]"; + // Else, emit a "worker error / warning" one + else { + var prefix = kindToWorkerPrefix(messageKind); + var lkqlLoc = + lkqlErrorLocation != null ? lkqlErrorLocation.display(true) + ": " : ""; - return adaLoc + kindtoString(messageKind) + ": " + lkqlLoc + message + rulePart; + if (messageKind == MessageKind.RULE_VIOLATION) { + prefix = kindToWorkerPrefix(MessageKind.ERROR); + message = "rule violation without Ada location: " + message; + } + + return prefix + ": " + lkqlLoc + message; + } } @Override @@ -304,6 +319,14 @@ public String kindtoString(MessageKind messageKind) { }; } + public static String kindToWorkerPrefix(MessageKind messageKind) { + return switch (messageKind) { + case WARNING -> "WORKER_WARNING"; + case ERROR -> "WORKER_ERROR"; + case RULE_VIOLATION -> ""; + }; + } + @Override public void emitFileNotFound(SourceLocation from, String fileName, boolean isError) { this.emitDiagnostic( diff --git a/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/invalid_syntax.lkql b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/invalid_syntax.lkql new file mode 100644 index 000000000..02b61f2e9 --- /dev/null +++ b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/invalid_syntax.lkql @@ -0,0 +1,3 @@ +val rules = @{ + Goto_Statements, +} diff --git a/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.out b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.out index ca138ab17..5fe41d9d6 100644 --- a/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.out +++ b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.out @@ -1,7 +1,18 @@ +Invalid syntax +============== + +gnatcheck: invalid_syntax.lkql:1:01: Cannot parse +gnatcheck: invalid_syntax.lkql:3:01: Expected Upper_Identifier, got '}' +gnatcheck: invalid_syntax.lkql:3:01: End of input expected, got "R_Curl" +gnatcheck: Syntax errors in invalid_syntax.lkql: stopping interpreter (invalid_syntax.lkql) +gnatcheck: No rule to check specified +try "gnatcheck --help" for more information. +>>>program returned status code 2 + Invalid LKQL semantic ===================== -gnatcheck: error raised by the worker: Error during file processing: null:0:0: error: internal issue at invalid_semantic.lkql:1:25: Type error: expected Int but got Bool (invalid_semantic.lkql) +gnatcheck: invalid_semantic.lkql:1:25: Type error: expected Int but got Bool (invalid_semantic.lkql) gnatcheck: No rule to check specified try "gnatcheck --help" for more information. >>>program returned status code 2 @@ -9,7 +20,7 @@ try "gnatcheck --help" for more information. No 'rules' object ================= -gnatcheck: error raised by the worker: LKQL config file must define a 'rules' top level object value (no_rules_object.lkql) +gnatcheck: LKQL config file must define a 'rules' top level object value (no_rules_object.lkql) gnatcheck: No rule to check specified try "gnatcheck --help" for more information. >>>program returned status code 2 @@ -32,7 +43,7 @@ try "gnatcheck --help" for more information. Not valid arguments =================== -gnatcheck: error raised by the worker: Rule arguments must be an object or indexable value (not_valid_args.lkql) +gnatcheck: Rule arguments must be an object or indexable value (not_valid_args.lkql) gnatcheck: No rule to check specified try "gnatcheck --help" for more information. >>>program returned status code 2 @@ -40,7 +51,7 @@ try "gnatcheck --help" for more information. Not valid arguments element =========================== -gnatcheck: error raised by the worker: Arguments should be in an object value (not_valid_args_elem.lkql) +gnatcheck: Arguments should be in an object value (not_valid_args_elem.lkql) gnatcheck: No rule to check specified try "gnatcheck --help" for more information. >>>program returned status code 2 @@ -70,4 +81,7 @@ gnatcheck: extra argument for instance goto_statements: 'extra' (extra_param.lkq Multiple instance with the same name ==================================== -gnatcheck: error raised by the worker: Multiple instances with the same name: an_instance (instance_names.lkql) +gnatcheck: Multiple instances with the same name: an_instance (instance_names.lkql) +gnatcheck: No rule to check specified +try "gnatcheck --help" for more information. +>>>program returned status code 2 diff --git a/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.yaml b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.yaml index 246880c9e..938e1e0d9 100644 --- a/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.yaml +++ b/testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.yaml @@ -4,6 +4,8 @@ input_sources: format: brief tests: +- label: Invalid syntax + lkql_rule_file: invalid_syntax.lkql - label: Invalid LKQL semantic lkql_rule_file: invalid_semantic.lkql - label: No 'rules' object