[analysis_server] Improve LSP server error messages for invalid paths/parse failures
Change-Id: I1a72ee62e282991b0071d0135c9a658bc5f7e14e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/232861
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/constants.dart b/pkg/analysis_server/lib/src/lsp/constants.dart
index a782782..55257c6 100644
--- a/pkg/analysis_server/lib/src/lsp/constants.dart
+++ b/pkg/analysis_server/lib/src/lsp/constants.dart
@@ -208,6 +208,8 @@
static const InvalidFileLineCol = ErrorCodes(-32004);
static const UnknownCommand = ErrorCodes(-32005);
static const InvalidCommandArguments = ErrorCodes(-32006);
+
+ /// A file that is not part of the analyzed set.
static const FileNotAnalyzed = ErrorCodes(-32007);
static const FileHasErrors = ErrorCodes(-32008);
static const ClientFailedToApplyEdit = ErrorCodes(-32009);
@@ -215,6 +217,9 @@
static const RefactorFailed = ErrorCodes(-32011);
static const FeatureDisabled = ErrorCodes(-32012);
+ /// A file that is expected to be analyzed, but failed.
+ static const FileAnalysisFailed = ErrorCodes(-32013);
+
/// An error raised when the server detects that the server and client are out
/// of sync and cannot recover. For example if a textDocument/didChange notification
/// has invalid offsets, suggesting the client and server have become out of sync
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_format_on_type.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_format_on_type.dart
index 81b9b85..84da8f2 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_format_on_type.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_format_on_type.dart
@@ -23,7 +23,8 @@
ErrorOr<List<TextEdit>?> formatFile(String path) {
final file = server.resourceProvider.getFile(path);
if (!file.exists) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ return error(
+ ServerErrorCodes.InvalidFilePath, 'File does not exist', path);
}
final result = server.getParsedUnit(path);
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_format_range.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_format_range.dart
index a6bbf90..f28ef15 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_format_range.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_format_range.dart
@@ -23,7 +23,8 @@
ErrorOr<List<TextEdit>?> formatRange(String path, Range range) {
final file = server.resourceProvider.getFile(path);
if (!file.exists) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ return error(
+ ServerErrorCodes.InvalidFilePath, 'File does not exist', path);
}
final result = server.getParsedUnit(path);
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_formatting.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_formatting.dart
index be75ccb..6ff4313 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_formatting.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_formatting.dart
@@ -23,7 +23,8 @@
ErrorOr<List<TextEdit>?> formatFile(String path) {
final file = server.resourceProvider.getFile(path);
if (!file.exists) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ return error(
+ ServerErrorCodes.InvalidFilePath, 'File does not exist', path);
}
final result = server.getParsedUnit(path);
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
index e0f497c..ddd82dc 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
@@ -56,7 +56,8 @@
final lineInfo = server.getLineInfo(path);
if (lineInfo == null) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ return error(ServerErrorCodes.InvalidFilePath,
+ 'Unable to obtain line information for file', path);
} else {
return success(lineInfo);
}
@@ -65,7 +66,19 @@
Future<ErrorOr<ResolvedUnitResult>> requireResolvedUnit(String path) async {
final result = await server.getResolvedUnit(path);
if (result == null) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ if (server.isAnalyzed(path)) {
+ // If the file was being analyzed and we got a null result, that usually
+ // indicators a parser or analysis failure, so provide a more specific
+ // message.
+ return error(ServerErrorCodes.FileAnalysisFailed,
+ 'Analysis failed for file', path);
+ } else {
+ return error(ServerErrorCodes.FileNotAnalyzed,
+ 'File is not being analyzed', path);
+ }
+ } else if (!result.exists) {
+ return error(
+ ServerErrorCodes.InvalidFilePath, 'File does not exist', path);
}
return success(result);
}
@@ -73,7 +86,16 @@
ErrorOr<ParsedUnitResult> requireUnresolvedUnit(String path) {
final result = server.getParsedUnit(path);
if (result == null) {
- return error(ServerErrorCodes.InvalidFilePath, 'Invalid file path', path);
+ if (server.isAnalyzed(path)) {
+ // If the file was being analyzed and we got a null result, that usually
+ // indicators a parser or analysis failure, so provide a more specific
+ // message.
+ return error(ServerErrorCodes.FileAnalysisFailed,
+ 'Analysis failed for file', path);
+ } else {
+ return error(ServerErrorCodes.FileNotAnalyzed,
+ 'File is not being analyzed', path);
+ }
}
return success(result);
}
diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart
index 853fa09..ba6a1d6 100644
--- a/pkg/analysis_server/lib/src/lsp/mapping.dart
+++ b/pkg/analysis_server/lib/src/lsp/mapping.dart
@@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+import 'dart:io';
import 'dart:math';
import 'package:analysis_server/lsp_protocol/protocol_custom_generated.dart'
@@ -798,7 +799,18 @@
));
}
try {
- return ErrorOr<String>.success(uri.toFilePath());
+ final filePath = uri.toFilePath();
+ // On Windows, paths that start with \ and not a drive letter are not
+ // supported but will return `true` from `path.isAbsolute` so check for them
+ // specifically.
+ if (Platform.isWindows && filePath.startsWith(r'\')) {
+ return ErrorOr<String>.error(ResponseError(
+ code: lsp.ServerErrorCodes.InvalidFilePath,
+ message: 'URI was not an absolute file path (missing drive letter)',
+ data: uri.toString(),
+ ));
+ }
+ return ErrorOr<String>.success(filePath);
} catch (e) {
// Even if tryParse() works and file == scheme, toFilePath() can throw on
// Windows if there are invalid characters.
diff --git a/pkg/analysis_server/test/lsp/document_symbols_test.dart b/pkg/analysis_server/test/lsp/document_symbols_test.dart
index a37882a..48ddc1b 100644
--- a/pkg/analysis_server/test/lsp/document_symbols_test.dart
+++ b/pkg/analysis_server/test/lsp/document_symbols_test.dart
@@ -206,8 +206,10 @@
newFile(mainFilePath, content: content);
await initialize(allowEmptyRootUri: true);
- await expectLater(getDocumentSymbols(mainFileUri.toString()),
- throwsA(isResponseError(ServerErrorCodes.InvalidFilePath)));
+ await expectLater(
+ getDocumentSymbols(mainFileUri.toString()),
+ throwsA(isResponseError(ServerErrorCodes.FileNotAnalyzed,
+ message: 'File is not being analyzed')));
}
Future<void> test_nonDartFile() async {
diff --git a/pkg/analysis_server/test/lsp/format_test.dart b/pkg/analysis_server/test/lsp/format_test.dart
index 7ffff8a..f3d24e8 100644
--- a/pkg/analysis_server/test/lsp/format_test.dart
+++ b/pkg/analysis_server/test/lsp/format_test.dart
@@ -640,7 +640,8 @@
await expectLater(
formatDocument(
Uri.file(join(projectFolderPath, 'missing.dart')).toString()),
- throwsA(isResponseError(ServerErrorCodes.InvalidFilePath)),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'File does not exist')),
);
}
@@ -649,8 +650,9 @@
await expectLater(
// Add some invalid path characters to the end of a valid file:// URI.
- formatDocument(mainFileUri.toString() + '***.dart'),
- throwsA(isResponseError(ServerErrorCodes.InvalidFilePath)),
+ formatDocument(mainFileUri.toString() + r'***###\\\///:::.dart'),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'File URI did not contain a valid file path')),
);
}
@@ -659,7 +661,8 @@
await expectLater(
formatDocument('a:/a.dart'),
- throwsA(isResponseError(ServerErrorCodes.InvalidFilePath)),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'URI was not a valid file:// URI')),
);
}
diff --git a/pkg/analysis_server/test/lsp/server_test.dart b/pkg/analysis_server/test/lsp/server_test.dart
index 22608c0..48d0109 100644
--- a/pkg/analysis_server/test/lsp/server_test.dart
+++ b/pkg/analysis_server/test/lsp/server_test.dart
@@ -2,11 +2,15 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+import 'dart:io';
+
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
+import 'package:analysis_server/src/lsp/constants.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
+import '../tool/lsp_spec/matchers.dart';
import 'server_abstract.dart';
void main() {
@@ -44,6 +48,67 @@
await server.exited.timeout(const Duration(seconds: 10));
}
+ Future<void> test_path_doesNotExist() async {
+ final missingFileUri = Uri.file(join(projectFolderPath, 'missing.dart'));
+ await initialize();
+ await expectLater(
+ getHover(missingFileUri, startOfDocPos),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'File does not exist')),
+ );
+ }
+
+ Future<void> test_path_invalidFormat() async {
+ await initialize();
+ await expectLater(
+ // Add some invalid path characters to the end of a valid file:// URI.
+ formatDocument(mainFileUri.toString() + r'***###\\\///:::.dart'),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'File URI did not contain a valid file path')),
+ );
+ }
+
+ Future<void> test_path_missingDriveLetterWindows() async {
+ // This test is only valid on Windows, as a URI in the format:
+ // file:///foo/bar.dart
+ // is valid for non-Windows platforms, but not valid on Windows as it does
+ // not have a drive letter.
+ if (!Platform.isWindows) {
+ return;
+ }
+ final missingDriveLetterFileUri = Uri.file('/foo/bar.dart');
+ await initialize();
+ await expectLater(
+ getHover(missingDriveLetterFileUri, startOfDocPos),
+ // The Uri.file() above translates to a non-file:// URI of just 'a/b.dart'
+ // so will get the not-file-scheme error message.
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'URI was not an absolute file path (missing drive letter)')),
+ );
+ }
+
+ Future<void> test_path_notFileScheme() async {
+ final relativeFileUri = Uri(scheme: 'foo', path: '/a/b.dart');
+ await initialize();
+ await expectLater(
+ getHover(relativeFileUri, startOfDocPos),
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'URI was not a valid file:// URI')),
+ );
+ }
+
+ Future<void> test_path_relative() async {
+ final relativeFileUri = Uri.file('a/b.dart');
+ await initialize();
+ await expectLater(
+ getHover(relativeFileUri, startOfDocPos),
+ // The Uri.file() above translates to a non-file:// URI of just 'a/b.dart'
+ // so will get the not-file-scheme error message.
+ throwsA(isResponseError(ServerErrorCodes.InvalidFilePath,
+ message: 'URI was not a valid file:// URI')),
+ );
+ }
+
Future<void> test_shutdown_initialized() async {
await initialize();
final response = await sendShutdown();
diff --git a/pkg/analysis_server/test/tool/lsp_spec/matchers.dart b/pkg/analysis_server/test/tool/lsp_spec/matchers.dart
index a424ae1..65bb7ae 100644
--- a/pkg/analysis_server/test/tool/lsp_spec/matchers.dart
+++ b/pkg/analysis_server/test/tool/lsp_spec/matchers.dart
@@ -15,8 +15,13 @@
Matcher isMapOf(Matcher indexMatcher, Matcher valueMatcher) =>
MapTypeMatcher(wrapMatcher(indexMatcher), wrapMatcher(valueMatcher));
-Matcher isResponseError(ErrorCodes code) => const TypeMatcher<ResponseError>()
- .having((e) => e.code, 'code', equals(code));
+Matcher isResponseError(ErrorCodes code, {String? message}) {
+ final matcher = const TypeMatcher<ResponseError>()
+ .having((e) => e.code, 'code', equals(code));
+ return message != null
+ ? matcher.having((e) => e.message, 'message', equals(message))
+ : matcher;
+}
Matcher isSimpleType(String name) => SimpleTypeMatcher(name);
