Skip to content

Commit d6215b7

Browse files
DanTupCommit Bot
DanTup
authored and
Commit Bot
committed
[analysis_server] [LSP] Prevent Getter<->Method refactors showing in invalid places
Fixes Dart-Code/Dart-Code#4000. Change-Id: I3b0b39b871b2d7176a1cbbe73064fce2586813ec Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249482 Commit-Queue: Brian Wilkerson <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]>
1 parent 77bf07a commit d6215b7

File tree

6 files changed

+135
-28
lines changed

6 files changed

+135
-28
lines changed

‎pkg/analysis_server/lib/src/handler/legacy/edit_get_available_refactorings.dart

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,9 @@ class EditGetAvailableRefactoringsHandler extends LegacyHandler {
6161
if (element != null) {
6262
// try CONVERT_METHOD_TO_GETTER
6363
if (element is ExecutableElement) {
64-
Refactoring refactoring = ConvertMethodToGetterRefactoring(
65-
searchEngine, resolvedUnit.session, element);
66-
var status = await refactoring.checkInitialConditions();
67-
if (!status.hasFatalError) {
64+
if (ConvertMethodToGetterRefactoring(
65+
searchEngine, resolvedUnit.session, element)
66+
.isAvailable()) {
6867
kinds.add(RefactoringKind.CONVERT_METHOD_TO_GETTER);
6968
}
7069
}

‎pkg/analysis_server/lib/src/lsp/handlers/handler_code_actions.dart

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,10 @@ class CodeActionHandler
612612
final node = NodeLocator(offset).searchWithin(unit.unit);
613613
final element = server.getElementOfNode(node);
614614
// Getter to Method
615-
if (element is PropertyAccessorElement) {
615+
if (element is PropertyAccessorElement &&
616+
ConvertGetterToMethodRefactoring(
617+
server.searchEngine, unit.session, element)
618+
.isAvailable()) {
616619
refactorActions.add(createRefactor(
617620
CodeActionKind.RefactorRewrite,
618621
'Convert Getter to Method',
@@ -621,7 +624,9 @@ class CodeActionHandler
621624

622625
// Method to Getter
623626
if (element is ExecutableElement &&
624-
element is! PropertyAccessorElement) {
627+
ConvertMethodToGetterRefactoring(
628+
server.searchEngine, unit.session, element)
629+
.isAvailable()) {
625630
refactorActions.add(createRefactor(
626631
CodeActionKind.RefactorRewrite,
627632
'Convert Method to Getter',

‎pkg/analysis_server/lib/src/services/refactoring/convert_getter_to_method.dart

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,24 @@ class ConvertGetterToMethodRefactoringImpl extends RefactoringImpl
7070
return change;
7171
}
7272

73-
RefactoringStatus _checkInitialConditions() {
73+
@override
74+
bool isAvailable() {
75+
return !_checkElement().hasFatalError;
76+
}
77+
78+
/// Checks if [element] is valid to perform this refactor.
79+
RefactoringStatus _checkElement() {
7480
if (!element.isGetter || element.isSynthetic) {
7581
return RefactoringStatus.fatal(
7682
'Only explicit getters can be converted to methods.');
7783
}
7884
return RefactoringStatus();
7985
}
8086

87+
RefactoringStatus _checkInitialConditions() {
88+
return _checkElement();
89+
}
90+
8191
Future<void> _updateElementDeclaration(
8292
PropertyAccessorElement element) async {
8393
// prepare "get" keyword

‎pkg/analysis_server/lib/src/services/refactoring/convert_method_to_getter.dart

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,37 @@ class ConvertMethodToGetterRefactoringImpl extends RefactoringImpl
3939

4040
@override
4141
Future<RefactoringStatus> checkInitialConditions() async {
42+
return _checkElement();
43+
}
44+
45+
@override
46+
Future<SourceChange> createChange() async {
47+
change = SourceChange(refactoringName);
48+
// FunctionElement
49+
final element = this.element;
50+
if (element is FunctionElement) {
51+
await _updateElementDeclaration(element);
52+
await _updateElementReferences(element);
53+
}
54+
// MethodElement
55+
if (element is MethodElement) {
56+
var elements = await getHierarchyMembers(searchEngine, element);
57+
await Future.forEach(elements, (Element element) async {
58+
await _updateElementDeclaration(element);
59+
return _updateElementReferences(element);
60+
});
61+
}
62+
// done
63+
return change;
64+
}
65+
66+
@override
67+
bool isAvailable() {
68+
return !_checkElement().hasFatalError;
69+
}
70+
71+
/// Checks if [element] is valid to perform this refactor.
72+
RefactoringStatus _checkElement() {
4273
// check Element type
4374
if (element is FunctionElement) {
4475
if (element.enclosingElement is! CompilationUnitElement) {
@@ -63,27 +94,6 @@ class ConvertMethodToGetterRefactoringImpl extends RefactoringImpl
6394
return RefactoringStatus();
6495
}
6596

66-
@override
67-
Future<SourceChange> createChange() async {
68-
change = SourceChange(refactoringName);
69-
// FunctionElement
70-
final element = this.element;
71-
if (element is FunctionElement) {
72-
await _updateElementDeclaration(element);
73-
await _updateElementReferences(element);
74-
}
75-
// MethodElement
76-
if (element is MethodElement) {
77-
var elements = await getHierarchyMembers(searchEngine, element);
78-
await Future.forEach(elements, (Element element) async {
79-
await _updateElementDeclaration(element);
80-
return _updateElementReferences(element);
81-
});
82-
}
83-
// done
84-
return change;
85-
}
86-
8797
Future<void> _updateElementDeclaration(Element element) async {
8898
// prepare parameters
8999
FormalParameterList? parameters;

‎pkg/analysis_server/lib/src/services/refactoring/refactoring.dart

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,16 @@ abstract class ConvertGetterToMethodRefactoring implements Refactoring {
3939
AnalysisSession session, PropertyAccessorElement element) {
4040
return ConvertGetterToMethodRefactoringImpl(searchEngine, session, element);
4141
}
42+
43+
/// Return `true` if refactoring is available, possibly without checking all
44+
/// initial conditions.
45+
///
46+
/// This value may be used to control the visibility of the refactor in the UI
47+
/// so that it doesn't show up in locations that are obviously not
48+
/// appropriate. Initial conditions may perform additional checks (and provide
49+
/// user-friendly messages) for locations where a user might reasonably expect
50+
/// to see the refactor but it's not valid for a less obvious reason.
51+
bool isAvailable();
4252
}
4353

4454
/// [Refactoring] to convert normal [MethodDeclaration]s into getters.
@@ -49,6 +59,16 @@ abstract class ConvertMethodToGetterRefactoring implements Refactoring {
4959
AnalysisSession session, ExecutableElement element) {
5060
return ConvertMethodToGetterRefactoringImpl(searchEngine, session, element);
5161
}
62+
63+
/// Return `true` if refactoring is available, possibly without checking all
64+
/// initial conditions.
65+
///
66+
/// This value may be used to control the visibility of the refactor in the UI
67+
/// so that it doesn't show up in locations that are obviously not
68+
/// appropriate. Initial conditions may perform additional checks (and provide
69+
/// user-friendly messages) for locations where a user might reasonably expect
70+
/// to see the refactor but it's not valid for a less obvious reason.
71+
bool isAvailable();
5272
}
5373

5474
/// [Refactoring] to extract an expression into a local variable declaration.
@@ -100,6 +120,12 @@ abstract class ExtractLocalRefactoring implements Refactoring {
100120

101121
/// Return `true` if refactoring is available, possibly without checking all
102122
/// initial conditions.
123+
///
124+
/// This value may be used to control the visibility of the refactor in the UI
125+
/// so that it doesn't show up in locations that are obviously not
126+
/// appropriate. Initial conditions may perform additional checks (and provide
127+
/// user-friendly messages) for locations where a user might reasonably expect
128+
/// to see the refactor but it's not valid for a less obvious reason.
103129
bool isAvailable();
104130
}
105131

@@ -168,6 +194,12 @@ abstract class ExtractMethodRefactoring implements Refactoring {
168194

169195
/// Return `true` if refactoring is available, possibly without checking all
170196
/// initial conditions.
197+
///
198+
/// This value may be used to control the visibility of the refactor in the UI
199+
/// so that it doesn't show up in locations that are obviously not
200+
/// appropriate. Initial conditions may perform additional checks (and provide
201+
/// user-friendly messages) for locations where a user might reasonably expect
202+
/// to see the refactor but it's not valid for a less obvious reason.
171203
bool isAvailable();
172204
}
173205

@@ -195,6 +227,12 @@ abstract class ExtractWidgetRefactoring implements Refactoring {
195227

196228
/// Return `true` if refactoring is available, possibly without checking all
197229
/// initial conditions.
230+
///
231+
/// This value may be used to control the visibility of the refactor in the UI
232+
/// so that it doesn't show up in locations that are obviously not
233+
/// appropriate. Initial conditions may perform additional checks (and provide
234+
/// user-friendly messages) for locations where a user might reasonably expect
235+
/// to see the refactor but it's not valid for a less obvious reason.
198236
bool isAvailable();
199237
}
200238

@@ -214,6 +252,12 @@ abstract class InlineLocalRefactoring implements Refactoring {
214252

215253
/// Return `true` if refactoring is available, possibly without checking all
216254
/// initial conditions.
255+
///
256+
/// This value may be used to control the visibility of the refactor in the UI
257+
/// so that it doesn't show up in locations that are obviously not
258+
/// appropriate. Initial conditions may perform additional checks (and provide
259+
/// user-friendly messages) for locations where a user might reasonably expect
260+
/// to see the refactor but it's not valid for a less obvious reason.
217261
bool isAvailable();
218262
}
219263

@@ -246,6 +290,12 @@ abstract class InlineMethodRefactoring implements Refactoring {
246290

247291
/// Return `true` if refactoring is available, possibly without checking all
248292
/// initial conditions.
293+
///
294+
/// This value may be used to control the visibility of the refactor in the UI
295+
/// so that it doesn't show up in locations that are obviously not
296+
/// appropriate. Initial conditions may perform additional checks (and provide
297+
/// user-friendly messages) for locations where a user might reasonably expect
298+
/// to see the refactor but it's not valid for a less obvious reason.
249299
bool isAvailable();
250300
}
251301

‎pkg/analysis_server/test/lsp/code_actions_refactor_test.dart

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,45 @@ void f() {
5757
await verifyCodeActionEdits(
5858
codeAction, withoutMarkers(content), expectedContent);
5959
}
60+
61+
Future<void> test_setter_notAvailable() async {
62+
const content = '''
63+
set ^a(String value) {}
64+
65+
''';
66+
newFile(mainFilePath, withoutMarkers(content));
67+
await initialize();
68+
69+
final codeActions = await getCodeActions(mainFileUri.toString(),
70+
position: positionFromMarker(content));
71+
final codeAction =
72+
findCommand(codeActions, Commands.performRefactor, refactorTitle);
73+
74+
expect(codeAction, isNull);
75+
}
6076
}
6177

6278
@reflectiveTest
6379
class ConvertMethodToGetterCodeActionsTest extends AbstractCodeActionsTest {
6480
final refactorTitle = 'Convert Method to Getter';
6581

82+
Future<void> test_constructor_notAvailable() async {
83+
const content = '''
84+
class A {
85+
^A();
86+
}
87+
''';
88+
newFile(mainFilePath, withoutMarkers(content));
89+
await initialize();
90+
91+
final codeActions = await getCodeActions(mainFileUri.toString(),
92+
position: positionFromMarker(content));
93+
final codeAction =
94+
findCommand(codeActions, Commands.performRefactor, refactorTitle);
95+
96+
expect(codeAction, isNull);
97+
}
98+
6699
Future<void> test_refactor() async {
67100
const content = '''
68101
int ^test() => 42;

0 commit comments

Comments
 (0)