| # Writing a quick fix |
| |
| This document describes what a quick fix is and outlines the basic steps for |
| writing a quick fix. |
| |
| ## Overview |
| |
| A quick fix is an automated [code edit](code_edits.md) that's associated with a |
| diagnostic. They are required to be [local in scope](code_edits.md#scope). The |
| intent is to automate the work required to fix the issue being reported by the |
| diagnostic. |
| |
| There are three ways for users to apply quick fixes: single-site, fix-in-file, |
| and bulk. We'll discuss how to implement the support for each of these usages in |
| its own section below. The sections build on each other, so we recommend reading |
| them in order. |
| |
| Through most of this document we'll use a simple example. We'll assume you wrote |
| a new lint named `could_be_final` that flags fields that could be marked final |
| but aren't, and that you're adding a fix for it. The fix will simply add the |
| keyword `final` to the field declaration. |
| |
| ## Single-site fixes |
| |
| A single-site fix is one in which a single fix can be applied to a single |
| location at which the associated diagnostic is reported. |
| |
| When the client asks for quick fixes at a given location, the analysis server |
| computes the relevant diagnostics based on that location. Then, for each |
| diagnostic, it computes one or more fixes. It then returns the list of computed |
| fixes to the client. The client typically allows the user to choose a single fix |
| to be applied. |
| |
| ### Design considerations |
| |
| Because quick fixes are computed on demand, they need to be computed quickly. |
| (Some clients request quick fixes every time the user repositions the cursor.) |
| That places a performance requirement on quick fixes, one that requires that the |
| code to compute a quick fix can't perform any potentially lengthy computations |
| such as searching all of the user's code or accessing the network. That, in |
| turn, generally means that fixes can only support localized changes. They can |
| add or remove text in the library in which the diagnostic was reported, but |
| generally can't do more than that. |
| |
| ### Describing the fix |
| |
| Each fix has an instance of the class `FixKind` associated with it. The existing |
| fixes for Dart diagnostics are defined in the class `DartFixKind`, fixes for the |
| analysis options file are defined in the class `AnalysisOptionsFixKind`, and |
| fixes for the pubspec file are in the class `PubspecFixKind`. A fix kind has an |
| identifier, a priority, and a message. |
| |
| The identifier is used by some LSP-based clients to provide user-defined |
| shortcuts. It's a hierarchical dot-separated identifier and should follow the |
| pattern seen in the existing fix kinds. |
| |
| The priority is used to order the list of fixes when presented to the user. The |
| larger the value the closer to the top of the list it will appear. If you're |
| implementing a fix for Dart files, you should use one of the constants defined |
| in `DartFixKindPriority` (typically `DartFixKindPriority.DEFAULT`), or add a new |
| constant if there's a need for it. |
| |
| The message is what will be displayed to the user by the client. This should be |
| a sentence fragment (no terminating period) that would be appropriate as a label |
| in a menu or on a button. It should describe the change that will be made to the |
| user's code. |
| |
| Create a static field, in the appropriate class, whose value is a `FixKind` |
| describing the fix you're implementing. For our example you might define a new |
| constant in `DartFixKind` like this: |
| |
| ```dart |
| static const ADD_FINAL = FixKind( |
| 'dart.fix.add.final', |
| DartFixKindPriority.DEFAULT, |
| "Add 'final' modifier", |
| ); |
| ``` |
| |
| ### Implementing the fix, part 1 |
| |
| To implement the fix you'll create a subclass of `CorrectionProducer`. Most of |
| the existing correction producers are in the directory |
| `analysis_server/lib/src/services/correction/dart`, so we'll start by creating |
| a file named `add_final.dart` in that directory that contains the following |
| (with the year updated appropriately): |
| |
| ```dart |
| // Copyright (c) 2022, 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 'package:analysis_server/src/services/correction/dart/abstract_producer.dart'; |
| import 'package:analysis_server/src/services/correction/fix.dart'; |
| import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart'; |
| import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; |
| |
| class AddFinal extends CorrectionProducer { |
| @override |
| FixKind get fixKind => DartFixKind.ADD_FINAL; |
| |
| @override |
| Future<void> compute(ChangeBuilder builder) async { |
| } |
| } |
| ``` |
| |
| The `compute` method is where the fix will be built. We'll come back to it in |
| "Implementing the fix, part 2". |
| |
| The `fixKind` getter is how you associate the fix kind we created earlier with |
| the fix produced by the `compute` method. |
| |
| There's another getter you might need to override. The message associated with |
| the fix kind is actually a template that can be filled in at runtime. The |
| placeholders in the message are denoted by integers inside curly braces (such as |
| `{0}`). The integers are indexes into a list of replacement values (hence, zero |
| based), and the getter `fixArguments` returns the list of replacement values. |
| The message we used above doesn't have any placeholders, but if we'd written the |
| message as `"Add '{0}' modifier"`, then we could return the replacement values |
| by implementing something like: |
| |
| ```dart |
| @override |
| List<Object> get fixArguments => ['final']; |
| ``` |
| |
| If you don't implement this getter, then the inherited getter will return an |
| empty list. The number of elements in the list must match the largest index used |
| in the message. If it doesn't, then an exception will be thrown at runtime. |
| |
| ### Testing the fix |
| |
| Before we look at implementing the `compute` method, we should probably write |
| some tests. Even if you don't normally use a test-driven approach to coding, we |
| recommend it when writing fixes because writing the tests can help you think of |
| corner cases that the implementation will need to handle. The corresponding |
| tests are in the directory `analysis_server/test/src/services/correction/fix`, |
| so we'll |
| create a file named `add_final_test.dart` that contains the following: |
| |
| ```dart |
| // Copyright (c) 2022, 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 'package:analysis_server/src/services/correction/fix.dart'; |
| import 'package:analysis_server/src/services/linter/lint_names.dart'; |
| import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; |
| import 'package:test_reflective_loader/test_reflective_loader.dart'; |
| |
| import 'fix_processor.dart'; |
| |
| void main() { |
| defineReflectiveSuite(() { |
| defineReflectiveTests(AddFinalTest); |
| }); |
| } |
| |
| @reflectiveTest |
| class AddFinalTest extends FixProcessorLintTest { |
| @override |
| FixKind get kind => DartFixKind.ADD_FINAL; |
| |
| @override |
| String get lintCode => LintNames.could_be_final; |
| } |
| ``` |
| |
| The two getters in the test class tell the test framework to enable the lint |
| being fixed and to expect a fix of the expected kind. Unless there's already a |
| fix associated with the lint you'll likely need to add a new constant in |
| `LintNames`. |
| |
| The test can then be written in a method that looks something like this: |
| |
| ```dart |
| Future<void> test_withType() async { |
| await resolveTestCode(''' |
| class C { |
| String name; |
| |
| C(this.name); |
| } |
| '''); |
| await assertHasFix(''' |
| class C { |
| final String name; |
| |
| C(this.name); |
| } |
| '''); |
| } |
| ``` |
| |
| The test framework will create a file containing the first piece of code, run |
| the lint over the code, use the correction producer you wrote to build a fix, |
| apply the fix to the file, and textually compare the results with the second |
| piece of code. |
| |
| ### Registering the fix |
| |
| Before we can run the test, we need to register the correction producer so that |
| it can be run. |
| |
| The list of fixes is computed by a `FixProcessor`. For each diagnostic passed |
| to it, it will look up the diagnostic in a table to get a list of the correction |
| producers it can use to produce fixes. There are three tables: |
| |
| - The [`lintProducerMap`][lintmap] is used for fixes related to lint rules. The |
| table is keyed by the name of the lint to which the fix applies. |
| |
| - The [`nonLintProducerMap`][nonlintmap] is used for all other fixes. The table |
| is keyed by the `ErrorCode` associated with the diagnostic. |
| |
| - The [`nonLintMultiProducerMap`][multimap] is used for multi-producers, which |
| are described below in "Multi-fix producers". |
| |
| Actually, the tables contain lists of functions used to create the producers. We |
| do that so that producers can't accidentally carry state over from one use to |
| the next. These functions are usually a tear-off of one of the correction |
| producer's constructors. |
| |
| The last step is to add your correction producer to the appropriate map. If |
| you're adding a fix for a lint, then you'd add an entry like |
| |
| ```dart |
| LintNames.could_be_final: [ |
| AddFinal.new, |
| ], |
| ``` |
| |
| At this point you should be able to run the test and see it failing. |
| |
| ### Implementing the fix, part 2 |
| |
| We're now at a point where we can finish the implementation of the fix by |
| implementing the `compute` method. |
| |
| The correction producer has access to most of the information you should need in |
| order to write the fix. The change builder passed to `compute` is how you |
| construct the fix that will be sent back to the client. |
| |
| The first step in the implementation of any fix is to find the location in the |
| AST where the diagnostic was reported and verify that all the conditions on |
| which the fix is predicated are valid. Assuming that the lint has been written |
| and tested correctly, there's no need to test that the conditions that caused |
| the lint rule to generate a diagnostic are still true. However, sometimes there |
| are additional constraints that need to be satisfied before the fix can safely |
| be applied. For example, for our fix, where we're only going to add a keyword, |
| we'll need to ensure that there's only one field that would be impacted by the |
| change, otherwise we might introduce more problems than there were before the |
| fix. |
| |
| Of course, sometimes you'll end up duplicating some of the work done by the lint |
| in the course of getting the information you need from the AST. While that's |
| less than optimal, there isn't any mechanism for passing information from the |
| diagnostic to the fix, so sometimes it just can't be avoided. |
| |
| For this example we're going to assume that the lint highlights the name of the |
| field that could have been final. Finding the AST node is easy because it's done |
| for you by the fix processor before `compute` is invoked. All you have to do is |
| use the getter `node` to find the node at the beginning of the lint's highlight |
| region. We're expecting it to be the name of a field, so that's how we'll name |
| the variable: |
| |
| ```dart |
| @override |
| Future<void> compute(ChangeBuilder builder) async { |
| var fieldName = node; |
| } |
| ``` |
| |
| Then we need to verify that this node really is an identifier as expected: |
| |
| ```dart |
| @override |
| Future<void> compute(ChangeBuilder builder) async { |
| var fieldName = node; |
| if (fieldName is! SimpleIdentifier) { |
| return; |
| } |
| } |
| ``` |
| |
| If it isn't, then we'll return. Because we haven't used the builder to create a |
| fix, returning now means that no fix from this producer will be sent to the |
| client. |
| |
| Simple identifiers appear in lots of places, so to be extra sure we have what |
| we're looking for we'll also make sure that |
| - the identifier is the name being declared by a variable declaration, |
| - the variable declaration is in a list of variables in a field declaration, and |
| - there's only one field being declared. |
| |
| ```dart |
| @override |
| Future<void> compute(ChangeBuilder builder) async { |
| var fieldName = node; |
| if (fieldName is! SimpleIdentifier) { |
| return; |
| } |
| var field = fieldName.parent; |
| if (field is! VariableDeclaration || field.name != fieldName) { |
| return; |
| } |
| var fieldList = field.parent; |
| if (fieldList is! VariableDeclarationList || |
| fieldList.variables.length > 1 || |
| fieldList.parent is! FieldDeclaration) { |
| return; |
| } |
| } |
| ``` |
| |
| After all those checks we now know that the `fieldName` really is the name of a |
| field and that there are no other fields that would be impacted by marking the |
| list of fields with `final`. |
| |
| We're now ready to create the actual fix. To do that we're going to use the |
| `ChangeBuilder` passed to the `compute` method. In the example below we'll |
| introduce a couple of the methods on `ChangeBuilder`, but for more information |
| you can read [Creating `SourceChange`s](https://github.com/dart-lang/sdk/blob/main/pkg/analyzer_plugin/doc/tutorial/creating_edits.md). |
| |
| Fields can be declared with either `final`, `const`, `var`, or a type |
| annotation, and the change that needs to be made depends on how the field was |
| declared. To figure that out we'll make use of the fact that the first three |
| tokens are all accessible from the AST by using the getter `keyword`. If there |
| is no keyword, then we can safely insert `final `, but if `var` was used then it |
| has to be replaced. And, of course, if it's already `final` or `const` then we |
| shouldn't do anything (and presumably the lint rule wouldn't have produced a |
| lint). |
| |
| ```dart |
| @override |
| Future<void> compute(ChangeBuilder builder) async { |
| var fieldName = node; |
| if (fieldName is! SimpleIdentifier) { |
| return; |
| } |
| var field = fieldName.parent; |
| if (field is! VariableDeclaration || field.name != fieldName) { |
| return; |
| } |
| var fieldList = field.parent; |
| if (fieldList is! VariableDeclarationList || |
| fieldList.variables.length > 1 || |
| fieldList.parent is! FieldDeclaration) { |
| return; |
| } |
| var keyword = fieldList.keyword; |
| if (keyword == null) { |
| await builder.addDartFileEdit(file, (builder) { |
| builder.addSimpleInsertion(fieldList.offset, 'final '); |
| }); |
| } else if (keyword.type == Keyword.VAR) { |
| await builder.addDartFileEdit(file, (builder) { |
| builder.addSimpleReplacement(range.token(keyword), 'final'); |
| }); |
| } |
| } |
| ``` |
| |
| In both cases we're using `addDartFileEdit` to create an edit in a `.dart` file. |
| If we only need to insert the keyword, then we'll use `addSimpleInsertion` to do |
| the insertion, but if we need to replace the existing keyword, then we'll use |
| `addSimpleReplacement` to do the replacement. |
| |
| We don't have a test case for the branch where the keyword `var` is used. We'll |
| leave adding such a test as an exercise for the reader. |
| |
| In this example we're just adding a single keyword, so we're avoiding any need |
| to worry about formatting. As a general principle we don't attempt to format the |
| code after it's been modified, but we do make an effort to leave the code in a |
| reasonably readable state. There's a getter (`eol`) that you can use to get the |
| end-of-line marker that should be used in the file, and there's another getter |
| (`utils`) that will return an object with several utility methods that help with |
| things like getting the right indentation for nested code. |
| |
| If we were adding a fix for a non-lint diagnostic, then there would be a couple |
| of minor differences. First, we'd register the correction producer using the |
| diagnostic's error code. Second, the test class would be a subclass of |
| `FixProcessorTest` and wouldn't specify the name of the lint. |
| |
| ### Multi-fix producers |
| |
| We skipped over the map named `nonLintMultiProducerMap` earlier, promising that |
| we'd return to it later. You'll probably never have a need for it, but in case |
| you do this section will hopefully tell you what you need to know. |
| |
| There's a subclass of `CorrectionProducer` named `MultiCorrectionProducer` and |
| this map is how you register one of them. That class exists for rare cases where |
| you need to use a single correction producer to produce multiple fixes. This is |
| generally only needed when you can't know in advance the maximum number of |
| fixes that might need to be produced. For example, if there is an undefined |
| identifier, and it might be possible to add an import to fix the problem, |
| there's no way to know in advance how many libraries might define the name, but |
| we'd want to propose adding an import for each such library. |
| |
| If you are able to enumerate the possible fixes ahead of time, then you're |
| better off to create one subclass of `CorrectionProducer` for each of the fixes. |
| For example, taking the case of an undefined identifier again, another way to |
| fix the problem is to add a declaration of the name. There are a finite number |
| of kinds of declarations a user might want: class, mixin, variable, etc. Even |
| though some kinds of declarations might not make sense because of how the |
| identifier is being used, it's better to have a separate correction producer for |
| each kind of declaration and have each one determine whether to generate a fix. |
| |
| And, we don't currently have support for associating a `MultiCorrectionProducer` |
| with a lint, so if you're writing fixes for a lint then this option isn't |
| available to you. |
| |
| ## Fix-in-file fixes |
| |
| A fix-in-file fix is one in which a single fix can be applied that will fix all |
| the locations within a single file at which a single kind of diagnostic has been |
| reported. |
| |
| When the client asks for quick fixes, in addition to computing the single-site |
| fixes, the analysis server looks to see whether any of the diagnostics at the |
| cursor location are also reported at other places in the same file. If any of |
| them are, and if the fix supports being used as a fix-in-file fix, then the |
| analysis server will attempt to apply the fix in each of those locations and to |
| build another fix that will apply edits to all the locations at which the |
| diagnostic was reported. |
| |
| The fix-in-file fix will only be shown when there is more than one diagnostic |
| for which your fix is able to produce an edit. For example, if there are three |
| fields that could all be made final, but the fix only produces an edit for one |
| of those places (maybe because the other two are in lists of fields), then the |
| fix-in-file fix won't be shown. |
| |
| If a fix-in-file fix can be created then it will be returned along with any of |
| the single-site fixes computed at the cursor location. |
| |
| In other words, most of the support for doing this comes for free; the only |
| requirement is that you declare that your fix can be used this way. |
| |
| ### Design considerations |
| |
| So why isn't this the default? Because it isn't always appropriate to support a |
| fix-in-file fix. We believe that it's better for users to sometimes not have a |
| fix-in-file fix that would have been appropriate than it is for them to have one |
| that isn't appropriate. While there isn't a complete list of situations in which |
| a fix-in-file fix isn't appropriate, let's look at a couple of the cases to |
| consider. |
| |
| First, some fixes, by nature, should only be applied after due consideration. |
| For example, there's a fix that will create a class declaration in response to |
| seeing a reference to an undefined name. While it's useful to have a way to |
| declare the name, how the name should be defined depends on the user's intent, |
| so it probably isn't appropriate to create class declarations for all undefined |
| identifiers without considering each case. |
| |
| Second, some fixes cause changes that impact other diagnostics of the same kind. |
| In such a case the automated support for fix-in-file fixes might well cause an |
| invalid edit to be produced, and fixes should never corrupt the user's code. For |
| example, there's a fix to add `const ` before a constructor invocation. If some |
| arguments to the invocation are also constructor invocations then the framework |
| could add an unnecessary keyword, which would be undesirable. |
| |
| ### Enabling the fix |
| |
| If it _is_ appropriate for your fix to be used as a fix-in-file fix, then there |
| are two things you'll need to do to enable the fix-in-file support. The first |
| step is to define another `FixKind` (in the same file in which you defined the |
| first fix kind) that looks something like the following: |
| |
| ```dart |
| static const ADD_FINAL_MULTI = FixKind( |
| 'dart.fix.add.final.multi', |
| DartFixKindPriority.IN_FILE, |
| "Add 'final' modifier everywhere in file", |
| ); |
| ``` |
| |
| This is different than the original fix kind in the following ways: |
| - both the name of the field and the identifier have a suffix added, |
| - the priority is lower than the single location fixes, and |
| - the message is worded in a way that makes it clear that multiple locations |
| will be changed. |
| |
| The second step is to implement a couple of additional getters in your |
| `CorrectionProducer`. First, let the framework know that your fix can be used in |
| this way: |
| |
| ```dart |
| @override |
| bool get canBeAppliedToFile => true; |
| ``` |
| |
| Then specify the fix kind associated with the fix-in-file fix: |
| |
| ```dart |
| @override |
| FixKind get multiFixKind => DartFixKind.ADD_FINAL_MULTI; |
| ``` |
| |
| And, if the message takes arguments (which the example above doesn't), provide |
| them using something like the following: |
| |
| ```dart |
| @override |
| List<Object> get multiFixArguments => ['first', 'second']; |
| ``` |
| |
| That's it; the fix-in-file fix should now appear. |
| |
| You should, of course, write tests for the fix. Those tests will be similar in |
| structure to those written to test the single application use case, but will be |
| in a subclass of `FixInFileProcessorTest`. They should be added to the test file |
| you created earlier for the single-site fixes |
| (`analysis_server/test/src/services/correction/fix/add_final_test.dart`). |
| |
| The most important distinction between the two is that for the fix-in-file tests |
| you'll need to ensure that there are at least two diagnostics produced for the |
| test code. If possible, those two diagnostics ought to be overlapping in terms |
| of the region of code impacted by the fix. For example, if the fix reverses the |
| order of two arguments in an argument list (such as by converting `m(a, b)` to |
| `m(b, a)`), then make one of the arguments be an invocation of `m` whose |
| arguments would also need to be swapped (such as `m(a, m(b, c)`). |
| |
| ## Bulk fixes |
| |
| A bulk fix is one in which all of the fixes for all of the diagnostics that have |
| been reported in one or more files, such as an entire package, are applied in a |
| single operation. This kind of fix is currently available through the `dart fix` |
| command-line tool. |
| |
| A bulk fix is computed in a manner similar to the way a fix-in-file fix is |
| computed except that all of the diagnostics across all of the specified files |
| are iterated over. Unlike the fix-in-file fixes, when bulk fixes are requested |
| only the one bulk fix that was computed is returned. |
| |
| ### Design considerations |
| |
| The considerations for why you might not want to enable the fix for bulk |
| application using `dart fix` include all of those given above for fix-in-file |
| fixes, but there are a couple of other considerations to keep in mind. |
| |
| First, `dart fix` is a command-line tool, so unlike changes made in an editor |
| there is no support for rolling back the changes it makes (other than whatever |
| support the user has from their source control system). |
| |
| Second, the tool applies fixes to more than just a single diagnostic, so there |
| are likely to be more edits applied. That means that there's an even higher |
| probability of having conflicts between edits. The framework handles some kinds |
| of conflicts automatically, but can't handle everything. |
| |
| ### Enabling the fix |
| |
| If you decide that you do want your fix to be supported by `dart fix`, the only |
| change that's required is to implement the following getter: |
| |
| ```dart |
| @override |
| bool get canBeAppliedInBulk => true; |
| ``` |
| |
| You should, of course, write tests for bulk application of the fix. Those tests |
| will be identical in nature to those written to test the fix-in-file use case, |
| but will be in a subclass of `BulkFixProcessorTest`. They should also be added |
| to the test file you created earlier for the single-site and fix-in-file fixes |
| (`analysis_server/test/src/services/correction/fix/add_final_test.dart`). |
| |
| [lintmap]: https://github.com/dart-lang/sdk/blob/cc461910af5f138bb54025e33f539835262b02bc/pkg/analysis_server/lib/src/services/correction/fix_internal.dart#L394 |
| [nonlintmap]: https://github.com/dart-lang/sdk/blob/cc461910af5f138bb54025e33f539835262b02bc/pkg/analysis_server/lib/src/services/correction/fix_internal.dart#L959 |
| [multimap]: https://github.com/dart-lang/sdk/blob/cc461910af5f138bb54025e33f539835262b02bc/pkg/analysis_server/lib/src/services/correction/fix_internal.dart#L791 |