Improve the error messages reported in LSP when params are not valid

Change-Id: I305955a22d687e8338138db48dff4e32589bbc6a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105520
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This commit is contained in:
Danny Tuppeny 2019-06-11 19:07:29 +00:00 committed by commit-bot@chromium.org
parent 2cc0261f39
commit 9fb2842fce
9 changed files with 6042 additions and 1215 deletions

View file

@ -17,6 +17,7 @@ import 'dart:core' as core show deprecated;
import 'dart:convert' show JsonEncoder;
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/json_parsing.dart';
import 'package:analysis_server/src/protocol/protocol_internal.dart'
show listEqual, mapEqual;
import 'package:analyzer/src/generated/utilities_general.dart';
@ -46,10 +47,31 @@ class AnalyzerStatusParams implements ToJsonable {
return __result;
}
static bool canParse(Object obj) {
return obj is Map<String, dynamic> &&
obj.containsKey('isAnalyzing') &&
obj['isAnalyzing'] is bool;
static bool canParse(Object obj, [LspJsonReporter reporter]) {
reporter?.push();
try {
if (obj is Map<String, dynamic>) {
reporter?.field = 'isAnalyzing';
if (!obj.containsKey('isAnalyzing')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['isAnalyzing'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['isAnalyzing'] != null && !(obj['isAnalyzing'] is bool)) {
reporter?.reportError("must be of type bool");
return false;
}
return true;
} else {
reporter?.reportError("must be a JavaScript object");
return false;
}
} finally {
reporter?.pop();
}
}
@override
@ -99,12 +121,44 @@ class ClosingLabel implements ToJsonable {
return __result;
}
static bool canParse(Object obj) {
return obj is Map<String, dynamic> &&
obj.containsKey('range') &&
Range.canParse(obj['range']) &&
obj.containsKey('label') &&
obj['label'] is String;
static bool canParse(Object obj, [LspJsonReporter reporter]) {
reporter?.push();
try {
if (obj is Map<String, dynamic>) {
reporter?.field = 'range';
if (!obj.containsKey('range')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['range'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['range'] != null && !(Range.canParse(obj['range'], reporter))) {
reporter?.reportError("must be of type Range");
return false;
}
reporter?.field = 'label';
if (!obj.containsKey('label')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['label'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['label'] != null && !(obj['label'] is String)) {
reporter?.reportError("must be of type String");
return false;
}
return true;
} else {
reporter?.reportError("must be a JavaScript object");
return false;
}
} finally {
reporter?.pop();
}
}
@override
@ -185,20 +239,96 @@ class CompletionItemResolutionInfo implements ToJsonable {
return __result;
}
static bool canParse(Object obj) {
return obj is Map<String, dynamic> &&
obj.containsKey('file') &&
obj['file'] is String &&
obj.containsKey('offset') &&
obj['offset'] is num &&
obj.containsKey('libId') &&
obj['libId'] is num &&
obj.containsKey('displayUri') &&
obj['displayUri'] is String &&
obj.containsKey('rOffset') &&
obj['rOffset'] is num &&
obj.containsKey('rLength') &&
obj['rLength'] is num;
static bool canParse(Object obj, [LspJsonReporter reporter]) {
reporter?.push();
try {
if (obj is Map<String, dynamic>) {
reporter?.field = 'file';
if (!obj.containsKey('file')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['file'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['file'] != null && !(obj['file'] is String)) {
reporter?.reportError("must be of type String");
return false;
}
reporter?.field = 'offset';
if (!obj.containsKey('offset')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['offset'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['offset'] != null && !(obj['offset'] is num)) {
reporter?.reportError("must be of type num");
return false;
}
reporter?.field = 'libId';
if (!obj.containsKey('libId')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['libId'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['libId'] != null && !(obj['libId'] is num)) {
reporter?.reportError("must be of type num");
return false;
}
reporter?.field = 'displayUri';
if (!obj.containsKey('displayUri')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['displayUri'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['displayUri'] != null && !(obj['displayUri'] is String)) {
reporter?.reportError("must be of type String");
return false;
}
reporter?.field = 'rOffset';
if (!obj.containsKey('rOffset')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['rOffset'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['rOffset'] != null && !(obj['rOffset'] is num)) {
reporter?.reportError("must be of type num");
return false;
}
reporter?.field = 'rLength';
if (!obj.containsKey('rLength')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['rLength'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['rLength'] != null && !(obj['rLength'] is num)) {
reporter?.reportError("must be of type num");
return false;
}
return true;
} else {
reporter?.reportError("must be a JavaScript object");
return false;
}
} finally {
reporter?.pop();
}
}
@override
@ -253,10 +383,31 @@ class DartDiagnosticServer implements ToJsonable {
return __result;
}
static bool canParse(Object obj) {
return obj is Map<String, dynamic> &&
obj.containsKey('port') &&
obj['port'] is num;
static bool canParse(Object obj, [LspJsonReporter reporter]) {
reporter?.push();
try {
if (obj is Map<String, dynamic>) {
reporter?.field = 'port';
if (!obj.containsKey('port')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['port'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['port'] != null && !(obj['port'] is num)) {
reporter?.reportError("must be of type num");
return false;
}
return true;
} else {
reporter?.reportError("must be a JavaScript object");
return false;
}
} finally {
reporter?.pop();
}
}
@override
@ -309,13 +460,47 @@ class PublishClosingLabelsParams implements ToJsonable {
return __result;
}
static bool canParse(Object obj) {
return obj is Map<String, dynamic> &&
obj.containsKey('uri') &&
obj['uri'] is String &&
obj.containsKey('labels') &&
(obj['labels'] is List &&
(obj['labels'].every((item) => ClosingLabel.canParse(item))));
static bool canParse(Object obj, [LspJsonReporter reporter]) {
reporter?.push();
try {
if (obj is Map<String, dynamic>) {
reporter?.field = 'uri';
if (!obj.containsKey('uri')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['uri'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['uri'] != null && !(obj['uri'] is String)) {
reporter?.reportError("must be of type String");
return false;
}
reporter?.field = 'labels';
if (!obj.containsKey('labels')) {
reporter?.reportError("may not be undefined");
return false;
}
if (obj['labels'] == null) {
reporter?.reportError("may not be null");
return false;
}
if (obj['labels'] != null &&
!((obj['labels'] is List &&
(obj['labels'].every(
(item) => ClosingLabel.canParse(item, reporter)))))) {
reporter?.reportError("must be of type List<ClosingLabel>");
return false;
}
return true;
} else {
reporter?.reportError("must be a JavaScript object");
return false;
}
} finally {
reporter?.pop();
}
}
@override

File diff suppressed because it is too large Load diff

View file

@ -5,6 +5,7 @@
import 'dart:async';
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/src/lsp/json_parsing.dart';
const jsonRpcVersion = '2.0';
@ -29,9 +30,9 @@ Object specToJson(Object obj) {
ErrorOr<R> success<R>([R t]) => new ErrorOr<R>.success(t);
Null _alwaysNull(_) => null;
Null _alwaysNull(_, [__]) => null;
bool _alwaysTrue(_) => true;
bool _alwaysTrue(_, [__]) => true;
class Either2<T1, T2> {
final int _which;
@ -227,7 +228,8 @@ abstract class IncomingMessage {
/// A helper to allow handlers to declare both a JSON validation function and
/// parse function.
class LspJsonHandler<T> {
final bool Function(Map<String, Object>) validateParams;
final bool Function(Map<String, Object>, LspJsonReporter reporter)
validateParams;
final T Function(Map<String, Object>) convertParams;
const LspJsonHandler(this.validateParams, this.convertParams);

View file

@ -9,6 +9,7 @@ import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/handler_cancel_request.dart';
import 'package:analysis_server/src/lsp/handlers/handler_reject.dart';
import 'package:analysis_server/src/lsp/json_parsing.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analyzer/dart/analysis/results.dart';
@ -85,9 +86,15 @@ abstract class MessageHandler<P, R> with Handler<P, R> {
/// [NotificationMessage]s are not expected to return results.
FutureOr<ErrorOr<R>> handleMessage(
IncomingMessage message, CancellationToken token) {
if (!jsonHandler.validateParams(message.params)) {
return error(ErrorCodes.InvalidParams,
'Invalid params for ${message.method}', null);
final reporter = LspJsonReporter('params');
if (!jsonHandler.validateParams(message.params, reporter)) {
return error(
ErrorCodes.InvalidParams,
'Invalid params for ${message.method}:\n'
'${reporter.errors.isNotEmpty ? reporter.errors.first : ''}'
.trim(),
null,
);
}
final params = jsonHandler.convertParams(message.params);

View file

@ -0,0 +1,44 @@
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
// 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:collection';
/// Tracks a path through a JSON object during validation to allow reporting
/// validation errors with user-friendly paths to the invalid fields.
class LspJsonReporter {
/// The current field name being validated.
String field;
/// A list of errors collected so far.
final List<String> errors = [];
/// The path from the root object (usually `params`) to the current object
/// being validated.
final ListQueue<String> path = new ListQueue<String>();
LspJsonReporter([this.field]);
/// Pops the last field off the stack to become the current gield.
void pop() {
field = path.removeLast();
}
/// Pushes the current field onto a stack to allow reporting errors in child
/// properties.
void push() {
path.add(field);
field = null;
}
/// Reports an error message for the field represented by [field] at [path].
void reportError(String message) {
if (field != null) {
path.add(field);
}
errors.add('${path.join(".")} $message');
if (field != null) {
path.removeLast();
}
}
}

View file

@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analyzer/src/dart/analysis/driver.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
@ -70,6 +71,27 @@ class WorkspaceSymbolsTest extends AbstractLspAnalysisServerTest {
expect(symbols.any((s) => s.name == 'myMethod'), isFalse);
}
test_invalidParams() async {
await initialize();
// Create a request that doesn't supply the query param.
final request = new RequestMessage(
Either2<num, String>.t1(1),
Method.workspace_symbol,
<String, dynamic>{},
jsonRpcVersion,
);
final response = await sendRequestToServer(request);
expect(response.error.code, equals(ErrorCodes.InvalidParams));
// Ensure the error is useful to the client.
expect(
response.error.message,
equals('Invalid params for workspace/symbol:\n'
'params.query may not be undefined'),
);
}
test_partialMatch() async {
const content = '''
String topLevel = '';

View file

@ -6,6 +6,7 @@ import 'dart:convert';
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/json_parsing.dart';
import 'package:test/test.dart';
main() {
@ -137,6 +138,79 @@ main() {
isFalse);
});
test('canParse records undefined fields', () {
final reporter = LspJsonReporter('params');
expect(CreateFile.canParse(<String, dynamic>{}, reporter), isFalse);
expect(reporter.errors, hasLength(1));
expect(reporter.errors.first, equals("params.kind may not be undefined"));
});
test('canParse records null fields', () {
final reporter = LspJsonReporter('params');
expect(CreateFile.canParse({'kind': null}, reporter), isFalse);
expect(reporter.errors, hasLength(1));
expect(reporter.errors.first, equals("params.kind may not be null"));
});
test('canParse records fields of the wrong type', () {
final reporter = LspJsonReporter('params');
expect(RenameFileOptions.canParse({'overwrite': 1}, reporter), isFalse);
expect(reporter.errors, hasLength(1));
expect(reporter.errors.first,
equals("params.overwrite must be of type bool"));
});
test('canParse records nested undefined fields', () {
final reporter = LspJsonReporter('params');
expect(
CompletionParams.canParse(
{'textDocument': <String, dynamic>{}}, reporter),
isFalse);
expect(reporter.errors, hasLength(greaterThanOrEqualTo(1)));
expect(reporter.errors.first,
equals("params.textDocument.uri may not be undefined"));
});
test('canParse records nested null fields', () {
final reporter = LspJsonReporter('params');
expect(
CompletionParams.canParse({
'textDocument': {'uri': null}
}, reporter),
isFalse);
expect(reporter.errors, hasLength(greaterThanOrEqualTo(1)));
expect(reporter.errors.first,
equals("params.textDocument.uri may not be null"));
});
test('canParse records nested fields of the wrong type', () {
final reporter = LspJsonReporter('params');
expect(
CompletionParams.canParse({
'textDocument': {'uri': 1}
}, reporter),
isFalse);
expect(reporter.errors, hasLength(greaterThanOrEqualTo(1)));
expect(reporter.errors.first,
equals("params.textDocument.uri must be of type String"));
});
test(
'canParse records errors when the type is not in the set of allowed types',
() {
final reporter = LspJsonReporter('params');
expect(
WorkspaceEdit.canParse({
'documentChanges': {'uri': 1}
}, reporter),
isFalse);
expect(reporter.errors, hasLength(greaterThanOrEqualTo(1)));
expect(
reporter.errors.first,
equals(
"params.documentChanges must be of type Either2<List<TextDocumentEdit>, List<Either4<TextDocumentEdit, CreateFile, RenameFile, DeleteFile>>>"));
});
test('ResponseMessage can include a null result', () {
final id = new Either2<num, String>.t1(1);
final resp = new ResponseMessage(id, null, null, jsonRpcVersion);

View file

@ -161,29 +161,64 @@ Iterable<String> _wrapLines(List<String> lines, int maxLength) sync* {
void _writeCanParseMethod(IndentableStringBuffer buffer, Interface interface) {
buffer
..writeIndentedln('static bool canParse(Object obj) {')
..writeIndentedln(
'static bool canParse(Object obj, [LspJsonReporter reporter]) {')
..indent()
..writeIndented('return obj is Map<String, dynamic>');
..writeIndentedln('reporter?.push();')
..writeIndentedln('try {')
..indent()
..writeIndentedln('if (obj is Map<String, dynamic>) {')
..indent();
// In order to consider this valid for parsing, all fields that may not be
// undefined must be present and also type check for the correct type.
// Any fields that are optional but present, must still type check.
final fields = _getAllFields(interface);
for (var field in fields) {
buffer.writeIndentedln("reporter?.field = '${field.name}';");
if (!field.allowsUndefined) {
buffer.write(" && obj.containsKey('${field.name}') && ");
} else {
buffer.write(" && ");
buffer
..writeIndentedln("if (!obj.containsKey('${field.name}')) {")
..indent()
..writeIndentedln('reporter?.reportError("may not be undefined");')
..writeIndentedln('return false;')
..outdent()
..writeIndentedln('}');
}
if (field.allowsNull || field.allowsUndefined) {
buffer.write("(obj['${field.name}'] == null || ");
}
_writeTypeCheckCondition(buffer, "obj['${field.name}']", field.type);
if (field.allowsNull || field.allowsUndefined) {
buffer.write(")");
if (!field.allowsNull && !field.allowsUndefined) {
buffer
..writeIndentedln("if (obj['${field.name}'] == null) {")
..indent()
..writeIndentedln('reporter?.reportError("may not be null");')
..writeIndentedln('return false;')
..outdent()
..writeIndentedln('}');
}
buffer..writeIndented("if (obj['${field.name}'] != null && !(");
_writeTypeCheckCondition(buffer, "obj['${field.name}']", field.type, true);
buffer
..write(')) {')
..indent()
..writeIndentedln(
'reporter?.reportError("must be of type ${field.type.dartTypeWithTypeArgs}");')
..writeIndentedln('return false;')
..outdent()
..writeIndentedln('}');
}
buffer
..writeln(';')
..writeIndentedln('return true;')
..outdent()
..writeIndentedln('} else {')
..indent()
..writeIndentedln('reporter?.reportError("must be a JavaScript object");')
..writeIndentedln('return false;')
..outdent()
..writeIndentedln('}')
..outdent()
..writeIndentedln('} finally {')
..indent()
..writeIndentedln('reporter?.pop();')
..outdent()
..writeIndentedln('}')
..outdent()
..writeIndentedln('}');
}
@ -265,11 +300,12 @@ void _writeEnumClass(IndentableStringBuffer buffer, Namespace namespace) {
..writeln()
..writeIndentedln('final ${typeOfValues.dartTypeWithTypeArgs} _value;')
..writeln()
..writeIndentedln('static bool canParse(Object obj) {')
..writeIndentedln(
'static bool canParse(Object obj, [LspJsonReporter reporter]) {')
..indent();
if (allowsAnyValue) {
buffer.writeIndentedln('return ');
_writeTypeCheckCondition(buffer, 'obj', typeOfValues);
_writeTypeCheckCondition(buffer, 'obj', typeOfValues, true);
buffer.writeln(';');
} else {
buffer
@ -399,7 +435,7 @@ void _writeFromJsonCodeForUnion(
// Dynamic matches all type checks, so only emit it if required.
if (!isDynamic) {
_writeTypeCheckCondition(buffer, valueCode, type);
_writeTypeCheckCondition(buffer, valueCode, type, false);
buffer.write(' ? ');
}
@ -623,8 +659,8 @@ void _writeType(IndentableStringBuffer buffer, AstNode type) {
}
}
void _writeTypeCheckCondition(
IndentableStringBuffer buffer, String valueCode, TypeBase type) {
void _writeTypeCheckCondition(IndentableStringBuffer buffer, String valueCode,
TypeBase type, bool hasErrorReporter) {
type = resolveTypeAlias(type);
final dartType = type.dartType;
@ -634,14 +670,19 @@ void _writeTypeCheckCondition(
} else if (_isSimpleType(type)) {
buffer.write('$valueCode is $fullDartType');
} else if (_isSpecType(type)) {
buffer.write('$dartType.canParse($valueCode)');
if (hasErrorReporter) {
buffer.write('$dartType.canParse($valueCode, reporter)');
} else {
buffer.write('$dartType.canParse($valueCode)');
}
} else if (type is ArrayType) {
buffer.write('($valueCode is List');
if (fullDartType != 'dynamic') {
// TODO(dantup): If we're happy to assume we never have two lists in a union
// we could skip this bit.
buffer.write(' && ($valueCode.every((item) => ');
_writeTypeCheckCondition(buffer, 'item', type.elementType);
_writeTypeCheckCondition(
buffer, 'item', type.elementType, hasErrorReporter);
buffer.write('))');
}
buffer.write(')');
@ -649,9 +690,11 @@ void _writeTypeCheckCondition(
buffer.write('($valueCode is Map');
if (fullDartType != 'dynamic') {
buffer..write(' && (')..write('$valueCode.keys.every((item) => ');
_writeTypeCheckCondition(buffer, 'item', type.indexType);
_writeTypeCheckCondition(
buffer, 'item', type.indexType, hasErrorReporter);
buffer..write('&& $valueCode.values.every((item) => ');
_writeTypeCheckCondition(buffer, 'item', type.valueType);
_writeTypeCheckCondition(
buffer, 'item', type.valueType, hasErrorReporter);
buffer.write(')))');
}
buffer.write(')');
@ -662,7 +705,8 @@ void _writeTypeCheckCondition(
if (i != 0) {
buffer.write(' || ');
}
_writeTypeCheckCondition(buffer, valueCode, type.types[i]);
_writeTypeCheckCondition(
buffer, valueCode, type.types[i], hasErrorReporter);
}
buffer.write(')');
} else {

View file

@ -165,6 +165,7 @@ import 'dart:core' as core show deprecated;
import 'dart:convert' show JsonEncoder;
import 'package:analysis_server/lsp_protocol/protocol${importCustom ? '_custom' : ''}_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/json_parsing.dart';
import 'package:analysis_server/src/protocol/protocol_internal.dart'
show listEqual, mapEqual;
import 'package:analyzer/src/generated/utilities_general.dart';