[dds/dap] Normalise Windows drive letters to avoid missing breakpoints

See https://github.com/dart-lang/sdk/issues/32222.
See https://github.com/Dart-Code/Dart-Code/issues/4149.

Change-Id: I6f975734839ff7cad4d086d5363c0ab03390b966
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/258900
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>

pkg/dds/CHANGELOG.md

@@ -1,6 +1,7 @@
-# 2.3.0-dev
+# 2.3.0
 - [DAP] Removed an unused parameter `resumeIfStarting` from `DartDebugAdapter.connectDebugger`.
 - [DAP] Fixed some issues where removing breakpoints could fail if an isolate exited during an update or multiple client breakpoints mapped to the same VM breakpoint.
+- [DAP] Paths provided to DAP now always have Windows drive letters normalized to uppercase to avoid some issues where paths may be treated case sensitively.
 
 # 2.2.6
 - Fixed an issue where debug adapters would not automatically close after terminating/disconnecting from the debugee.

pkg/dds/lib/src/dap/adapters/dart.dart

@@ -22,6 +22,7 @@ import '../protocol_converter.dart';
 import '../protocol_generated.dart';
 import '../protocol_stream.dart';
 import '../utils.dart';
+import 'mixins.dart';
 
 /// The mime type to send with source responses to the client.
 ///
@@ -285,7 +286,8 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments {
 /// (for example when the server sends a `StoppedEvent` it may cause the client
 /// to then send a `stackTraceRequest` or `scopesRequest` to get variables).
 abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
-    TA extends AttachRequestArguments> extends BaseDebugAdapter<TL, TA> {
+        TA extends AttachRequestArguments> extends BaseDebugAdapter<TL, TA>
+    with FileUtils {
   late final DartCommonLaunchAttachRequestArguments args;
   final _debuggerInitializedCompleter = Completer<void>();
   final _configurationDoneCompleter = Completer<void>();
@@ -706,7 +708,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     void Function(Object?) sendResponse,
   ) async {
     switch (request.command) {
-
       // Used by tests to validate available protocols (e.g. DDS). There may be
       // value in making this available to clients in future, but for now it's
       // internal.
@@ -1186,7 +1187,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
 
     final path = args.source.path;
     final name = args.source.name;
-    final uri = path != null ? Uri.file(path).toString() : name!;
+    final uri = path != null ? Uri.file(normalizePath(path)).toString() : name!;
 
     await _isolateManager.setBreakpoints(uri, breakpoints);
 

pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart

@@ -124,7 +124,9 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     }
 
     // Handle customTool and deletion of any arguments for it.
-    final executable = args.customTool ?? Platform.resolvedExecutable;
+    final executable = normalizePath(
+      args.customTool ?? Platform.resolvedExecutable,
+    );
     final removeArgs = args.customToolReplacesArgs;
     if (args.customTool != null && removeArgs != null) {
       vmArgs.removeRange(0, math.min(removeArgs, vmArgs.length));
@@ -133,7 +135,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     final processArgs = [
       ...vmArgs,
       ...toolArgs,
-      args.program,
+      normalizePath(args.program),
       ...?args.args,
     ];
 
@@ -153,20 +155,25 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
                 : null
         : null;
 
+    var cwd = args.cwd;
+    if (cwd != null) {
+      cwd = normalizePath(cwd);
+    }
+
     if (terminalKind != null) {
       await launchInEditorTerminal(
         debug,
         terminalKind,
         executable,
         processArgs,
-        workingDirectory: args.cwd,
+        workingDirectory: cwd,
         env: args.env,
       );
     } else {
       await launchAsProcess(
         executable,
         processArgs,
-        workingDirectory: args.cwd,
+        workingDirectory: cwd,
         env: args.env,
       );
     }
@@ -216,7 +223,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     // we can detect with the normal watching code.
     final requestArgs = RunInTerminalRequestArguments(
       args: [executable, ...processArgs],
-      cwd: workingDirectory ?? path.dirname(args.program),
+      cwd: workingDirectory ?? normalizePath(path.dirname(args.program)),
       env: env,
       kind: terminalKind,
       title: args.name ?? 'Dart',

pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart

@@ -111,7 +111,9 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     ];
 
     // Handle customTool and deletion of any arguments for it.
-    final executable = args.customTool ?? Platform.resolvedExecutable;
+    final executable = normalizePath(
+      args.customTool ?? Platform.resolvedExecutable,
+    );
     final removeArgs = args.customToolReplacesArgs;
     if (args.customTool != null && removeArgs != null) {
       vmArgs.removeRange(0, math.min(removeArgs, vmArgs.length));
@@ -120,14 +122,19 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     final processArgs = [
       ...vmArgs,
       ...?args.toolArgs,
-      args.program,
+      normalizePath(args.program),
       ...?args.args,
     ];
 
+    var cwd = args.cwd;
+    if (cwd != null) {
+      cwd = normalizePath(cwd);
+    }
+
     await launchAsProcess(
       executable,
       processArgs,
-      workingDirectory: args.cwd,
+      workingDirectory: cwd,
       env: args.env,
     );
   }

pkg/dds/lib/src/dap/adapters/mixins.dart

@@ -134,7 +134,7 @@ mixin TestAdapter {
 /// A mixin providing some utility functions for working with vm-service-info
 /// files such as ensuring a temp folder exists to create them in, and waiting
 /// for the file to become valid parsable JSON.
-mixin VmServiceInfoFileUtils {
+mixin VmServiceInfoFileUtils on FileUtils {
   /// Creates a temp folder for the VM to write the service-info-file into and
   /// returns the [File] to use.
   File generateVmServiceInfoFile() {
@@ -143,7 +143,8 @@ mixin VmServiceInfoFileUtils {
     // watcher. Creating/watching a folder and writing the file into it seems
     // to be reliable.
     final serviceInfoFilePath = path.join(
-      Directory.systemTemp.createTempSync('dart-vm-service').path,
+      normalizePath(
+          Directory.systemTemp.createTempSync('dart-vm-service').path),
       'vm.json',
     );
 
@@ -203,3 +204,24 @@ mixin VmServiceInfoFileUtils {
     }
   }
 }
+
+mixin FileUtils {
+  /// Normalizes [filePath] to avoid issues with different casing of drive
+  /// letters on Windows.
+  ///
+  /// Some clients like VS Code do their own normalization and may provide drive
+  /// letters case different in some requests (such as breakpoints) to drive
+  /// letters computed elsewhere (such in `Platform.resolvedExecutable`). When
+  /// these do not match, breakpoints may not be hit.
+  ///
+  /// This is the case for the whole path, but drive letters are most commonly
+  /// mismatched due to VS Code's explicit normalization.
+  ///
+  /// https://github.com/dart-lang/sdk/issues/32222
+  String normalizePath(String filePath) {
+    if (!Platform.isWindows || filePath.isEmpty || path.isRelative(filePath)) {
+      return filePath;
+    }
+    return filePath.substring(0, 1).toUpperCase() + filePath.substring(1);
+  }
+}

pkg/dds/pubspec.yaml

@@ -1,5 +1,5 @@
 name: dds
-version: 2.2.6
+version: 2.3.0
 description: >-
   A library used to spawn the Dart Developer Service, used to communicate with
   a Dart VM Service instance.

pkg/dds/test/dap/integration/debug_breakpoints_test.dart

@@ -107,7 +107,8 @@ void main(List<String> args) async {
       await client.hitBreakpoint(sdkFile, breakpointLine, entryFile: testFile);
     });
 
-    test('stops at a line breakpoint and can be resumed', () async {
+    /// Tests hitting a simple breakpoint and resuming.
+    Future<void> _testHitBreakpointAndResume() async {
       final client = dap.client;
       final testFile = dap.createTestFile(simpleBreakpointProgram);
       final breakpointLine = lineWith(testFile, breakpointMarker);
@@ -120,8 +121,32 @@ void main(List<String> args) async {
         client.event('terminated'),
         client.continue_(stop.threadId!),
       ], eagerError: true);
+    }
+
+    test('stops at a line breakpoint and can be resumed', () async {
+      await _testHitBreakpointAndResume();
     });
 
+    test(
+        'stops at a line breakpoint and can be resumed '
+        'when breakpoint requests have lowercase drive letters '
+        'and program/VM have uppercase drive letters', () async {
+      final client = dap.client;
+      client.forceDriveLetterCasingUpper = true;
+      client.forceBreakpointDriveLetterCasingLower = true;
+      await _testHitBreakpointAndResume();
+    }, skip: !Platform.isWindows);
+
+    test(
+        'stops at a line breakpoint and can be resumed '
+        'when breakpoint requests have uppercase drive letters '
+        'and program/VM have lowercase drive letters', () async {
+      final client = dap.client;
+      client.forceDriveLetterCasingLower = true;
+      client.forceBreakpointDriveLetterCasingUpper = true;
+      await _testHitBreakpointAndResume();
+    }, skip: !Platform.isWindows);
+
     test('stops at a line breakpoint and can step over (next)', () async {
       final testFile = dap.createTestFile('''
 void main(List<String> args) async {

pkg/dds/test/dap/integration/test_client.dart

@@ -38,6 +38,20 @@ class DapTestClient {
 
   late final Future<Uri?> vmServiceUri;
 
+  /// Used to control drive letter casing on Windows for testing.
+  bool? forceDriveLetterCasingUpper;
+
+  /// Used to control drive letter casing on Windows for testing.
+  bool? forceDriveLetterCasingLower;
+
+  /// Used to control drive letter casing for breakpoint requests on Windows for
+  /// testing.
+  bool? forceBreakpointDriveLetterCasingUpper;
+
+  /// Used to control drive letter casing for breakpoint requests on Windows for
+  /// testing.
+  bool? forceBreakpointDriveLetterCasingLower;
+
   DapTestClient._(
     this._channel,
     this._logger, {
@@ -238,11 +252,12 @@ class DapTestClient {
     return sendRequest(
       DartLaunchRequestArguments(
         noDebug: noDebug,
-        program: program,
+        program: _normalizePath(program),
-        cwd: cwd,
+        cwd: cwd != null ? _normalizePath(cwd) : null,
         args: args,
         toolArgs: toolArgs,
-        additionalProjectPaths: additionalProjectPaths,
+        additionalProjectPaths:
+            additionalProjectPaths?.map(_normalizePath).toList(),
         console: console,
         debugSdkLibraries: debugSdkLibraries,
         debugExternalPackageLibraries: debugExternalPackageLibraries,
@@ -522,7 +537,7 @@ extension DapTestClientExtension on DapTestClient {
   Future<void> setBreakpoint(File file, int line, {String? condition}) async {
     await sendRequest(
       SetBreakpointsArguments(
-        source: Source(path: file.path),
+        source: Source(path: _normalizeBreakpointPath(file.path)),
         breakpoints: [SourceBreakpoint(line: line, condition: condition)],
       ),
     );
@@ -532,12 +547,48 @@ extension DapTestClientExtension on DapTestClient {
   Future<void> setBreakpoints(File file, List<int> lines) async {
     await sendRequest(
       SetBreakpointsArguments(
-        source: Source(path: file.path),
+        source: Source(path: _normalizeBreakpointPath(file.path)),
         breakpoints: lines.map((line) => SourceBreakpoint(line: line)).toList(),
       ),
     );
   }
 
+  /// Normalizes a non-breakpoint path being sent to the debug adapter based on
+  /// the values of [forceDriveLetterCasingUpper] and
+  /// [forceDriveLetterCasingLower].
+  String _normalizePath(String path) {
+    return _forceDriveLetterCasing(
+      path,
+      upper: forceDriveLetterCasingUpper,
+      lower: forceDriveLetterCasingLower,
+    );
+  }
+
+  /// Normalizes a non-breakpoint path being sent to the debug adapter based on
+  /// the values of [forceBreakpointDriveLetterCasingUpper] and
+  /// [forceBreakpointDriveLetterCasingLower].
+  String _normalizeBreakpointPath(String path) {
+    return _forceDriveLetterCasing(
+      path,
+      upper: forceBreakpointDriveLetterCasingUpper,
+      lower: forceBreakpointDriveLetterCasingLower,
+    );
+  }
+
+  String _forceDriveLetterCasing(String path, {bool? upper, bool? lower}) {
+    assert(upper != true || lower != true);
+    if (!Platform.isWindows || path.isEmpty) {
+      return path;
+    }
+    if (upper ?? false) {
+      return path.substring(0, 1).toUpperCase() + path.substring(1);
+    } else if (lower ?? false) {
+      return path.substring(0, 1).toLowerCase() + path.substring(1);
+    } else {
+      return path;
+    }
+  }
+
   /// Sets the exception pause mode to [pauseMode] and expects to pause after
   /// running the script.
   ///
@@ -585,7 +636,7 @@ extension DapTestClientExtension on DapTestClient {
       initialize(),
       sendRequest(
         SetBreakpointsArguments(
-          source: Source(path: file.path),
+          source: Source(path: _normalizeBreakpointPath(file.path)),
           breakpoints: [
             SourceBreakpoint(
               line: line,