Skip to content

Commit b0566a5

Browse files
DanTupCommit Queue
DanTup
authored and
Commit Queue
committed
[analysis_server] Remove isDefault and don't set value when there's no argument for EditableArguments API
Now that we have `defaultValue` against each argument, there was some redundancy here. `isDefault` is redundant because if `hasArgument=false` it's always default, and if `hasArgument=true`, then `value` and `defaultValue` can be compared When `hasArgument=false`, there's no value in duplicating `defaultValue` into `value` Change-Id: I5cba055e56175565842efef815e08dd46e9ed36a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406621 Reviewed-by: Elliott Brooks <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Brian Wilkerson <[email protected]>
1 parent a84fe54 commit b0566a5

File tree

4 files changed

+95
-102
lines changed

4 files changed

+95
-102
lines changed

‎pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_editable_arguments.dart‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,7 @@ import 'package:analyzer/src/dart/ast/ast.dart';
1919
import 'package:analyzer/src/lint/constants.dart';
2020

2121
/// Information about the values for a parameter/argument.
22-
typedef _Values =
23-
({bool isDefault, DartObject? parameterValue, DartObject? argumentValue});
22+
typedef _Values = ({DartObject? parameterValue, DartObject? argumentValue});
2423

2524
class EditableArgumentsHandler
2625
extends SharedMessageHandler<TextDocumentPositionParams, EditableArguments?>
@@ -143,17 +142,7 @@ class EditableArgumentsHandler
143142
var parameterValue = parameter.computeConstantValue();
144143
var argumentValue = argumentExpression?.computeConstantValue().value;
145144

146-
var isDefault =
147-
argumentValue == null ||
148-
((parameterValue?.hasKnownValue ?? false) &&
149-
(argumentValue.hasKnownValue) &&
150-
parameterValue == argumentValue);
151-
152-
return (
153-
isDefault: isDefault,
154-
parameterValue: parameterValue,
155-
argumentValue: argumentValue,
156-
);
145+
return (parameterValue: parameterValue, argumentValue: argumentValue);
157146
}
158147

159148
/// Converts a [parameter]/[argument] pair into an [EditableArgument] if it
@@ -167,7 +156,6 @@ class EditableArgumentsHandler
167156
}) {
168157
var valueExpression =
169158
argument is NamedExpression ? argument.expression : argument;
170-
var hasArgument = valueExpression != null;
171159

172160
// Lazily compute the values if we will use this parameter/argument.
173161
late var values = _getValues(parameter, valueExpression);
@@ -216,11 +204,6 @@ class EditableArgumentsHandler
216204
return null;
217205
}
218206

219-
// If no argument is present, we always populate "value" with the default.
220-
if (!hasArgument) {
221-
value = defaultValue;
222-
}
223-
224207
var isEditable = notEditableReason == null;
225208

226209
// Compute a displayValue.
@@ -246,7 +229,6 @@ class EditableArgumentsHandler
246229
value: value,
247230
displayValue: displayValue,
248231
options: options,
249-
isDefault: values.isDefault,
250232
defaultValue: defaultValue,
251233
hasArgument: valueExpression != null,
252234
isRequired: parameter.isRequired,

‎pkg/analysis_server/test/shared/shared_editable_arguments_tests.dart‎

Lines changed: 42 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ $content
7474
Object? value = anything,
7575
Object? displayValue = anything,
7676
Object? hasArgument = anything,
77-
Object? isDefault = anything,
7877
Object? defaultValue = anything,
7978
Object? isRequired = anything,
8079
Object? isNullable = anything,
@@ -88,7 +87,6 @@ $content
8887
.having((arg) => arg.value, 'value', value)
8988
.having((arg) => arg.displayValue, 'displayValue', displayValue)
9089
.having((arg) => arg.hasArgument, 'hasArgument', hasArgument)
91-
.having((arg) => arg.isDefault, 'isDefault', isDefault)
9290
.having((arg) => arg.defaultValue, 'defaultValue', defaultValue)
9391
.having((arg) => arg.isRequired, 'isRequired', isRequired)
9492
.having((arg) => arg.isNullable, 'isNullable', isNullable)
@@ -413,7 +411,7 @@ class MyWidget extends StatelessWidget {
413411
type: 'string',
414412
value: isNull,
415413
displayValue: 'ab',
416-
isDefault: false,
414+
hasArgument: true,
417415
isEditable: false,
418416
notEditableReason: "Adjacent strings can't be edited",
419417
),
@@ -1081,21 +1079,21 @@ class MyWidget extends StatelessWidget {
10811079
'supplied',
10821080
type: 'bool',
10831081
value: false,
1084-
isDefault: false,
1082+
hasArgument: true,
10851083
defaultValue: true,
10861084
),
10871085
isArg(
10881086
'suppliedAsDefault',
10891087
type: 'bool',
10901088
value: true,
1091-
isDefault: true,
1089+
hasArgument: true,
10921090
defaultValue: true,
10931091
),
10941092
isArg(
10951093
'notSupplied',
10961094
type: 'bool',
1097-
value: true,
1098-
isDefault: true,
1095+
value: isNull,
1096+
hasArgument: false,
10991097
defaultValue: true,
11001098
),
11011099
]),
@@ -1171,21 +1169,21 @@ class MyWidget extends StatelessWidget {
11711169
'supplied',
11721170
type: 'double',
11731171
value: 2.0,
1174-
isDefault: false,
1172+
hasArgument: true,
11751173
defaultValue: 1.0,
11761174
),
11771175
isArg(
11781176
'suppliedAsDefault',
11791177
type: 'double',
11801178
value: 1.0,
1181-
isDefault: true,
1179+
hasArgument: true,
11821180
defaultValue: 1.0,
11831181
),
11841182
isArg(
11851183
'notSupplied',
11861184
type: 'double',
1187-
value: 1.0,
1188-
isDefault: true,
1185+
value: isNull,
1186+
hasArgument: false,
11891187
defaultValue: 1.0,
11901188
),
11911189
]),
@@ -1213,9 +1211,27 @@ class MyWidget extends StatelessWidget {
12131211
result,
12141212
hasArgs(
12151213
orderedEquals([
1216-
isArg('supplied', type: 'double', value: 2, isDefault: false),
1217-
isArg('suppliedAsDefault', type: 'double', value: 1, isDefault: true),
1218-
isArg('notSupplied', type: 'double', value: 1, isDefault: true),
1214+
isArg(
1215+
'supplied',
1216+
type: 'double',
1217+
value: 2,
1218+
hasArgument: true,
1219+
defaultValue: 1,
1220+
),
1221+
isArg(
1222+
'suppliedAsDefault',
1223+
type: 'double',
1224+
value: 1,
1225+
hasArgument: true,
1226+
defaultValue: 1,
1227+
),
1228+
isArg(
1229+
'notSupplied',
1230+
type: 'double',
1231+
value: isNull,
1232+
hasArgument: false,
1233+
defaultValue: 1,
1234+
),
12191235
]),
12201236
),
12211237
);
@@ -1292,23 +1308,23 @@ class MyWidget extends StatelessWidget {
12921308
'supplied',
12931309
type: 'enum',
12941310
value: 'E.two',
1295-
isDefault: false,
1311+
hasArgument: true,
12961312
defaultValue: 'E.one',
12971313
options: optionsMatcher,
12981314
),
12991315
isArg(
13001316
'suppliedAsDefault',
13011317
type: 'enum',
13021318
value: 'E.one',
1303-
isDefault: true,
1319+
hasArgument: true,
13041320
defaultValue: 'E.one',
13051321
options: optionsMatcher,
13061322
),
13071323
isArg(
13081324
'notSupplied',
13091325
type: 'enum',
1310-
value: 'E.one',
1311-
isDefault: true,
1326+
value: isNull,
1327+
hasArgument: false,
13121328
defaultValue: 'E.one',
13131329
options: optionsMatcher,
13141330
),
@@ -1393,21 +1409,21 @@ class MyWidget extends StatelessWidget {
13931409
'supplied',
13941410
type: 'int',
13951411
value: 2,
1396-
isDefault: false,
1412+
hasArgument: true,
13971413
defaultValue: 1,
13981414
),
13991415
isArg(
14001416
'suppliedAsDefault',
14011417
type: 'int',
14021418
value: 1,
1403-
isDefault: true,
1419+
hasArgument: true,
14041420
defaultValue: 1,
14051421
),
14061422
isArg(
14071423
'notSupplied',
14081424
type: 'int',
1409-
value: 1,
1410-
isDefault: true,
1425+
value: isNull,
1426+
hasArgument: false,
14111427
defaultValue: 1,
14121428
),
14131429
]),
@@ -1483,21 +1499,21 @@ class MyWidget extends StatelessWidget {
14831499
'supplied',
14841500
type: 'string',
14851501
value: 'b',
1486-
isDefault: false,
1502+
hasArgument: true,
14871503
defaultValue: 'a',
14881504
),
14891505
isArg(
14901506
'suppliedAsDefault',
14911507
type: 'string',
14921508
value: 'a',
1493-
isDefault: true,
1509+
hasArgument: true,
14941510
defaultValue: 'a',
14951511
),
14961512
isArg(
14971513
'notSupplied',
14981514
type: 'string',
1499-
value: 'a',
1500-
isDefault: true,
1515+
value: isNull,
1516+
hasArgument: false,
15011517
defaultValue: 'a',
15021518
),
15031519
]),

‎pkg/analysis_server/tool/lsp_spec/generate_all.dart‎

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ List<LspEntity> getCustomClasses() {
436436
'type',
437437
type: 'string',
438438
comment:
439-
'The kind of parameter. This is not necessarily the Dart type, '
439+
'The kind of parameter.\n\nThis is not necessarily the Dart type, '
440440
'it is from a defined set of values that clients may understand '
441441
'how to edit.',
442442
),
@@ -446,46 +446,41 @@ List<LspEntity> getCustomClasses() {
446446
allowsNull: false,
447447
allowsUndefined: true,
448448
comment:
449-
'The current value for this argument. This is only included if '
450-
'an explicit value is given in the code and is a valid literal for '
451-
'the kind of parameter. For expressions or named constants, this '
452-
'will not be included and displayValue can be shown as the current '
453-
'value instead.',
449+
'The current value for this argument (provided only if '
450+
'hasArgument=true).\n\nThis is only included if an explicit value '
451+
'is given in the code and is a valid literal for the kind of '
452+
'parameter. For expressions or named constants, this will not be '
453+
'included and displayValue can be shown as the current value '
454+
'instead.\n\nA value of `null` when hasArgument=true means the '
455+
'argument has an explicit null value and not that defaultValue is '
456+
'being used.',
454457
),
455458
field(
456459
'hasArgument',
457460
type: 'boolean',
458461
comment:
459462
'Whether an explicit argument exists for this parameter in the '
460-
'code. This will be true even if the explicit argument is the same '
461-
'value as the parameter default.',
462-
),
463-
field(
464-
'isDefault',
465-
type: 'boolean',
466-
comment:
467-
'Whether the value is the default for this parameter, either '
468-
'because there is no argument or because it is explicitly provided '
469-
'as the same value.',
463+
'code.\n\nThis will be true even if the explicit argument is the '
464+
'same value as the parameter default or null.',
470465
),
471466
Field(
472467
name: 'defaultValue',
473468
type: TypeReference.LspAny,
474469
allowsNull: false,
475470
allowsUndefined: true,
476471
comment:
477-
'The default value for this parameter if no argument is supplied. '
478-
'Setting the argument to this value does not remove it from the '
479-
'argument list.',
472+
'The default value for this parameter if no argument is supplied.'
473+
'\n\nSetting the argument to this value does not remove it from '
474+
'the argument list.',
480475
),
481476
field(
482477
'displayValue',
483478
type: 'string',
484479
canBeUndefined: true,
485480
comment:
486481
'A string that can be displayed to indicate the value for this '
487-
'argument. This will be populated in cases where the source code '
488-
'is not literally the same as the value field, for example an '
482+
'argument.\n\nThis will be populated in cases where the source '
483+
'code is not literally the same as the value field, for example an '
489484
'expression or named constant.',
490485
),
491486
field(
@@ -497,14 +492,14 @@ List<LspEntity> getCustomClasses() {
497492
'isNullable',
498493
type: 'boolean',
499494
comment:
500-
'Whether this argument can be `null`. It is possible for an '
495+
'Whether this argument can be `null`.\n\nIt is possible for an '
501496
'argument to be required, but still allow an explicit `null`.',
502497
),
503498
field(
504499
'isEditable',
505500
type: 'boolean',
506501
comment:
507-
'Whether this argument can be add/edited. If not, '
502+
'Whether this argument can be add/edited.\n\nIf not, '
508503
'notEditableReason will contain an explanation for why.',
509504
),
510505
field(
@@ -521,7 +516,7 @@ List<LspEntity> getCustomClasses() {
521516
array: true,
522517
canBeUndefined: true,
523518
comment:
524-
'The set of values allowed for this argument if it is an enum. '
519+
'The set of values allowed for this argument if it is an enum.\n\n'
525520
'Values are qualified in the form `EnumName.valueName`.',
526521
),
527522
// TODO(dantup): field('properties', ...),

0 commit comments

Comments
 (0)