Merge branch 'topic/vscode-config' into 'master'

Update VS Code settings and project loading policies

Closes #1621 and #1631

See merge request eng/ide/ada_language_server!1973

Author: eliericha

integration/vscode/ada/package.json

@@ -25,12 +25,12 @@
         "projectDiagnostics": {
             "type": "boolean",
             "default": true,
-            "description": "Controls whether or not the Ada Language Server should emit project diagnostics into the VS Code Problems view."
+            "description": "Controls whether or not the Ada Language Server should emit diagnostics about project loading."
         },
         "alireDiagnostics": {
             "type": "boolean",
             "default": true,
-            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to alire into the VS Code Problems view."
+            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to Alire."
         },
         "defaultCharset": {
             "type": "string",
@@ -88,12 +88,12 @@
         "adaFileDiagnostics": {
             "type": "boolean",
             "default": true,
-            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to the edition of Ada Files into the VS Code Problems view."
+            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to the edition of Ada files."
         },
         "gprFileDiagnostics": {
             "type": "boolean",
             "default": true,
-            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to the edition of GPR Files into the VS Code Problems view."
+            "description": "Controls whether or not the Ada Language Server should emit diagnostics related to the edition of GPR files."
         },
         "foldComments": {
             "type": "boolean",

integration/vscode/ada/schemas/als-settings-schema.json

@@ -1,10 +1,12 @@
 import { existsSync } from 'fs';
 import * as vscode from 'vscode';
 import {
+    DidChangeConfigurationNotification,
     LanguageClient,
     LanguageClientOptions,
     MessageSignature,
     ServerOptions,
+    State,
 } from 'vscode-languageclient/node';
 import { logger } from './extension';
 import { logErrorAndThrow, setTerminalEnvironment } from './helpers';
@@ -124,16 +126,69 @@ export function createClient(
         // Register the server for ada sources documents
         documentSelector: [{ scheme: 'file', language: id }],
         synchronize: {
-            // Synchronize the setting section 'ada' to the server
-            configurationSection: 'ada',
             // Notify the server about file changes to Ada files contain in the workspace
             fileEvents: vscode.workspace.createFileSystemWatcher(pattern),
         },
-        // Include the ada.* settings in the initialize request sent to the server
-        initializationOptions: () => ({ ada: vscode.workspace.getConfiguration('ada') }),
+        initializationOptions: () =>
+            /**
+             * Only include the settings that are explicitly set in one of the
+             * VS Code scopes to avoid unintentionally overriding ALS settings
+             * from .als.json or other applicable configuration files.
+             */
+            ({ ada: getExplicitlySetConfiguration() }),
     };
+
     // Create the language client
     const client = new AdaLanguageClient(id, name, serverOptions, clientOptions);
+
+    context.subscriptions.push(
+        vscode.workspace.onDidChangeConfiguration(async (e) => {
+            if (e.affectsConfiguration('ada') && client.state === State.Running) {
+                await client.sendNotification(DidChangeConfigurationNotification.type, {
+                    settings: {
+                        ada: getExplicitlySetConfiguration(),
+                    },
+                });
+            }
+        }),
+    );
+
     client.serverEnv = serverEnv;
     return client;
 }
+
+/**
+ * ALS settings can come from configuration files (e.g. .als.json) and from VS
+ * Code.
+ *
+ * Settings from config files are loaded first. To avoid overwriting them
+ * unintentionally, settings from VS Code should only be sent if they were
+ * explicitly set in one of the VS Code configuration scopes.
+ *
+ * This function returns that set of settings.
+ *
+ * @returns a dictionary of ada.* settings that have been explicitly set in one
+ * of the VS Code configuration scopes.
+ */
+function getExplicitlySetConfiguration(): { [k: string]: unknown } {
+    // Get all ada.* settings
+    const adaConfig = vscode.workspace.getConfiguration('ada');
+    const explicitSettings = Object.fromEntries(
+        Object.entries(adaConfig).filter(([key]) => {
+            // Filter to settings that are explicitly set
+            const info = adaConfig.inspect(key);
+            return (
+                info &&
+                [
+                    info.globalValue,
+                    info.workspaceValue,
+                    info.workspaceFolderValue,
+                    info.globalLanguageValue,
+                    info.workspaceLanguageValue,
+                    info.workspaceFolderLanguageValue,
+                ].some((v) => v !== undefined)
+            );
+        }),
+    );
+    return explicitSettings;
+}

integration/vscode/ada/src/clients.ts

@@ -50,6 +50,7 @@ with URIs;
 
 with VSS.Characters;
 with VSS.Characters.Latin;
+with VSS.String_Vectors;
 with VSS.Strings;
 with VSS.Strings.Conversions;
 
@@ -118,8 +119,9 @@ package body LSP.Ada_Handlers.Project_Loading is
 
    procedure Ensure_Project_Loaded (Self : in out Message_Handler'Class) is
       use type VSS.Strings.Virtual_String;
+      use VSS.Strings.Conversions;
 
-      GPRs_Found             : Natural := 0;
+      Candidates             : VSS.String_Vectors.Virtual_String_Vector;
       Project_File           : VSS.Strings.Virtual_String :=
         Self.Configuration.Project_File;
       GPR_Configuration_File : VSS.Strings.Virtual_String :=
@@ -188,29 +190,30 @@ package body LSP.Ada_Handlers.Project_Loading is
          declare
             Files : GNATCOLL.VFS.File_Array_Access :=
               Self.Client.Root_Directory.Read_Dir (GNATCOLL.VFS.Files_Only);
-            Found : GNATCOLL.VFS.Virtual_File;
          begin
             for X of Files.all loop
                if X.Has_Suffix (".gpr") then
-                  GPRs_Found := GPRs_Found + 1;
-                  exit when GPRs_Found > 1;
-                  Found := X;
+                  Candidates.Append (LSP.Utils.To_Virtual_String (X));
                end if;
             end loop;
 
             GNATCOLL.VFS.Unchecked_Free (Files);
 
-            if GPRs_Found = 1 then
-               Project_File := LSP.Utils.To_Virtual_String (Found);
+            if Candidates.Length = 1 then
+               Project_File := Candidates.First_Element;
 
                --  Report how we found the project
                Self.Project_Status.Set_Project_Type
                  (LSP.Ada_Project_Loading.Single_Project_Found);
 
                Tracer.Trace_Text ("Found unique project: " & Project_File);
             else
-               Tracer.Trace
-                 ("Found " & GPRs_Found'Image & " projects at the root");
+               Tracer.Trace_Text
+                 ("Found "
+                  & To_Virtual_String (Candidates.Length'Image)
+                  & " projects at the root:"
+                  & VSS.Characters.Latin.Line_Feed
+                  & Candidates.Join (VSS.Characters.Latin.Line_Feed));
             end if;
          end;
       end if;
@@ -276,12 +279,12 @@ package body LSP.Ada_Handlers.Project_Loading is
          --  We didn't find a project file. Let's load an implicit project. We
          --  reach this point either because there are no GPR projects at the
          --  root, or there are more than one.
-         pragma Assert (GPRs_Found = 0 or GPRs_Found > 1);
+         pragma Assert (Candidates.Length = 0 or Candidates.Length > 1);
 
          Load_Implicit_Project
            (Self,
-            (if GPRs_Found = 0 then LSP.Ada_Project_Loading.No_Project
-             elsif GPRs_Found > 1
+            (if Candidates.Length = 0 then LSP.Ada_Project_Loading.No_Project
+             elsif Candidates.Length > 1
              then LSP.Ada_Project_Loading.Multiple_Projects
              else LSP.Ada_Project_Loading.Project_Not_Found));
       end if;

source/ada/lsp-ada_handlers-project_loading.adb

@@ -94,6 +94,9 @@ with LSP.Utils;
 
 package body LSP.Ada_Handlers is
 
+   Tracer_Config : constant LSP.GNATCOLL_Tracers.Tracer :=
+     LSP.GNATCOLL_Tracers.Create ("ALS.CONFIG", GNATCOLL.Traces.On);
+
    pragma Style_Checks ("o");  --  check subprogram bodies in alphabetical ordr
 
    subtype AlsReferenceKind_Array is LSP.Structures.AlsReferenceKind_Set;
@@ -2486,6 +2489,8 @@ package body LSP.Ada_Handlers is
       if not Value.initializationOptions.Is_Empty then
          Self.Tracer.Trace
            ("Processing initializationOptions from initialize request");
+         Tracer_Config.Trace
+           ("initializationOptions = " & Value.initializationOptions'Image);
          --  The expected structure is this:
          --     "initializationOptions": {
          --        "ada": {
@@ -2495,7 +2500,12 @@ package body LSP.Ada_Handlers is
          --        }
          --     }
          declare
-            New_Configuration : LSP.Ada_Configurations.Configuration;
+            New_Configuration : LSP.Ada_Configurations.Configuration :=
+              Self.Configuration;
+            --  Start from the existing configuration so that settings parsed
+            --  from configuration files are preserved, and the settings from
+            --  initialize request are applied on top.
+
             Messages : VSS.String_Vectors.Virtual_String_Vector;
          begin
             --  Parse the configuration.
@@ -2537,30 +2547,27 @@ package body LSP.Ada_Handlers is
      (Self  : in out Message_Handler;
       Value : LSP.Structures.InitializedParams) is
    begin
-      --  The client is notifying us that it has initialized. If a project was
-      --  provided with the initialize request, we can load it and start
-      --  communicating with the client.
-      --
-      --  We only perform loading in case ada.projectFile was provided with the
-      --  initialize request. That's because some clients don't provide
-      --  settings in the initialize request, and instead send a
-      --  onDidChangeConfiguration notification immediately after
-      --  initialization. In this scenario, loading a project unconditionally
-      --  here would result in automatically searching for a project (unique
-      --  project at the root, or implicit fallback) and loading it, only to
-      --  load another project shortly after, upon receiving
-      --  onDidChangeConfiguration. To avoid the first useless load, we only
-      --  load a project here if it was specified in the initialize request.
+      --  The client is notifying us that it has initialized. It is good at
+      --  this stage to load a project so that subsequent requests, e.g.
+      --  project attribute requests to support VS Code integration, work on a
+      --  valid project.
       --
-      --  Moreover, there is an impact on legacy tests. The prior project
+      --  However, there is an impact on legacy tests. The prior project
       --  loading policy was to wait for the first onDidChangeConfiguration
       --  notification to obtain settings and load a project. Many existing
       --  tests do not set the project in the initialize request and don't
       --  expect messages pertaining to project loading after the initialize
-      --  request.  If we were to always load the project here, tests would
-      --  receive different messages and potentially fail. So the conditional
-      --  project loading here is backwards compatible with existing tests.
-      if not Self.Configuration.Project_File.Is_Empty then
+      --  request. If we were to always load the project here, tests would
+      --  receive different messages and potentially fail.
+      --
+      --  To mitigate that, we perform project loading conditionally to
+      --  Base_Configuration_Received. If Base_Configuration_Received is True,
+      --  it means that we received non-empty initializationOptions which is
+      --  the typical case in IDEs like GNAT Studio and VS Code.
+      --
+      --  This conditional load allows us to load a project early while
+      --  remaining backwards compatible with legacy tests.
+      if Self.Base_Configuration_Received then
          LSP.Ada_Handlers.Project_Loading.Ensure_Project_Loaded (Self);
       end if;
    end On_Initialized_Notification;
@@ -3935,6 +3942,8 @@ package body LSP.Ada_Handlers is
      (Self  : in out Message_Handler;
       Value : LSP.Ada_Configurations.Configuration'Class) is
    begin
+      Tracer_Config.Trace ("Setting configuration: " & Value'Image);
+
       Self.Configuration := LSP.Ada_Configurations.Configuration (Value);
 
       --  The base configuration is still the default one, meaning that

source/ada/lsp-ada_handlers.adb

@@ -19,3 +19,16 @@ async def func(lsp: ALSLanguageClient) -> None:
     response = await lsp.getCurrentProject()
     assert response
     assertEqual(response, URI("non-root/p2.gpr"))
+
+
+@test(als_settings={})
+async def test_init_override(lsp: ALSLanguageClient) -> None:
+    # In this test we want to check that settings obtained in the initializationOptions
+    # of the 'initialize' request are interpreted on top of the file-based
+    # configuration.
+    #
+    # To check that we send an empty dictionary of settings in the initialize request
+    # (the 'als_settings' argument above), and we check that the applicable project is
+    # still the one specified in .als.json, i.e. the initialize request did not
+    # overwrite the file settings.
+    assertEqual(await lsp.getCurrentProject(), URI("non-root/p2.gpr"))

testsuite/ada_lsp/config_local/test.py

@@ -1,6 +1,6 @@
 """
-The goal of this test is to check that when --config CONFIG_FILE is given, that
-configuration is loaded.
+The goal of the tests below is to check the priority with which the ALS considers
+configuration files and the initialize request.
 
 To test that we create a unique p1.gpr at the root. If there was no configuration,
 p1.gpr would be loaded automatically. It's a failure sentinel.

testsuite/ada_lsp/config_priority/test.py

@@ -0,0 +1,5 @@
+project Prj is
+   package Gnattest is
+      for Harness_Dir use "myharness";
+   end Gnattest;
+end Prj;

testsuite/ada_lsp/early_project_load/prj.gpr

@@ -0,0 +1,21 @@
+"""
+The goal of this test is to check that the ALS loads the project early in the session,
+such that immediately after the initialization sequence, it is possible to inspect
+project attributes without opening any Ada sources or sending a configuration change to
+force a project load.
+"""
+
+from drivers.pylsp import ALSLanguageClient, test
+
+
+@test(
+    # Sending initializationOptions should cause the ALS to load a project early,
+    # when handling the `initialized` notification. So we give an empty settings
+    # dictionary to force the sending of initializationOptions.
+    als_settings={}
+)
+async def t1(lsp: ALSLanguageClient):
+    # Query a project attribute
+    result = await lsp.getProjectAttributeValue("harness_dir", "gnattest")
+
+    lsp.assertEqual(result, "myharness")

testsuite/ada_lsp/early_project_load/test.py

@@ -0,0 +1 @@
+driver: pylsp