Merge branch 'topic/enhance_lkql_error' into 'master'

Enhance error messages when parsing an LKQL rule file

Closes #337

See merge request eng/libadalang/langkit-query-language!309

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));

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

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;
 

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<String, RuleInstance> 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<String> 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<String, RuleInstance> parseLKQLRuleFile(final String lkqlRuleFileName) {
+    private static Map<String, RuleInstance> parseLKQLRuleFile(final String lkqlRuleFileName)
+            throws LKQLRuleFileError {
         final File lkqlFile = new File(lkqlRuleFileName);
         final String lkqlFileBasename = lkqlFile.getName();
         final Map<String, RuleInstance> res = new HashMap<>();
@@ -313,9 +336,10 @@ private static Map<String, RuleInstance> 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<String, RuleInstance> toPopulate) {
+            final Map<String, RuleInstance> 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<String, RuleInstance> toPopulate) {
+            final Map<String, RuleInstance> 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 =

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(

testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/invalid_syntax.lkql

@@ -0,0 +1,3 @@
+val rules = @{
+    Goto_Statements,
+}

testsuite/tests/gnatcheck_errors/invalid_lkql_rules_config/test.out

@@ -1,15 +1,26 @@
+Invalid syntax
+==============
+
+gnatcheck: invalid_syntax.lkql:1:01: Cannot parse <val_decl>
+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
 
 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,15 +43,15 @@ 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
 
 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

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