Addresses a concern from code review
https://dart-review.googlesource.com/c/sdk/+/129302 that
EditPlanner.surround is difficult to use correctly, by adding wrapper
methods for several common use cases that are easier to use correctly.
Also adapts fix_aggregator.dart to make use of the new methods.
EditPlanner.surround remains in the public API for EditPlanner because
there are still use cases for it that aren't supported by the wrapper
methods. My hope is that the remaining use cases are either rare
enough or trivial enough that we don't need wrappers for them; if that
proves to be incorrect we can always add more wrapper methods.
Change-Id: Ia1d6cc8b10ffaedcf8803ff7c8f9460ada1d7c4e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132462
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This CL allows tests based on AnalysisContextCollectionImpl to enable
NNBD by modifying the analysis options and introduce a different
feature set, rather than having to add virtual filesystem files to
convince the analyzer that NNBD is enabled for the package. This is
more consistent with the way we enable NNBD for other unit tests that
invoke analysis more directly, and is less sensitive to details of how
the analyzer infers the current language version.
Currently used in the migration engine's EditPlanTest, which in a
subsequent CL will need this functionality to test editing source code
that contains null checks.
Change-Id: I352fd2d7f6ee71d42f02a31714e3395567967240
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132461
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Now there are only two circumstances in which an AtomicEdit won't have
information about the reason for the change:
- The AtomicEdit object was associated with adding or removing parens,
in which case we don't compute information about the reason for the
change because it's not straightforward to compute, and it should be
clear to the user anyway.
- Information about the reason for the change hasn't been plumbed
through properly (this should be addressed in follow-up CLs).
Since we are rapidly approaching a situation where nearly all
AtomicEdits will have reason information, the separation between
AtomicEdit and AtomicEditWithInfo now seems silly, so I just moved the
`info` object into AtomicEdit and removed AtomicEditWithInfo entirely.
Change-Id: I61afd9cd58b71d4695685e65d0142a2693b8e6b2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132451
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Previously, we used the offset of an AST node as a unique ID to
represent it, with a special hack to make sure we can distinguish a
generic function type from its return type (which has the same
offset). But in an upcoming CL, I will need to start associating
unique IDs with expressions, so that will bring up a whole bunch more
ambiguity.
This CL switches to a technique where we combine the offset and the
end of any source span into a single unique identifier. The algorithm
produces an output that is no larger than the square of either input.
Since we have 64-bit integers in Dart, this should be adequate for any
reasonable input file size.
Change-Id: I68ce27533eb485cc824e1080326d02de98bff1e7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132404
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The new FixBuilder component of the migration engine communicates the
changes it produces using a "changes" data structure of type Map<int,
List<AtomicEdit>>, and is not compatible with the old
SingleNullabilityFix class (which made the incorrect assumption that
each SourceEdit was associated with a single fix). This CL changes
the migration engine's instrumentation API to use this data structure,
and changes its listener API to avoid reference to the
SingleNullabilityFix class. This allows SingleNullabilityFix to be
deleted.
This required changing AtomicEditWithReason so that it can hold
multiple fix reasons, and a NullabilityFixDescription. It does so
using a new object, AtomicEditInfo, and AtomicEditWithReason is
renamed to AtomicEditWithInfo.
In addition, the old FixInfo class is removed, since it carries the
same information as the new AtomicEditWithInfo class.
This required deleting the "incremental workflow" functionality from
UnitRenderer. This functionality was disabled, and I believe it
wouldn't have worked anyway (since it, too, made the incorrect
assumption that each SourceEdit was associated with a single fix).
Change-Id: Id965cfd803b408c66f15436017b87fc3e46521c6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132306
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The FixBuilder implemenation is now at feature parity with the
non-FixBuilder implementation when testing with api_test.dart, except
for one test of extension functionality (which doesn't seem worth
fixing right now, since extensions are still so rarely used).
Change-Id: I89df7ec388824fa9f4181706bfadbaeb232d2b5f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132169
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
Previously, we tried to hook in to the resolver so that every time it
tried to check the type of a TypeAnnotation, the FixBuilder would see
if it needed to add a `?` to it before letting the resolver continue.
This was clumsy and didn't catch all the TypeAnnotations. This CL
changes to a simpler technique where we simply visit all
TypeAnnotations, making note of where we need to add `?`s and updating
types, prior to re-running resolution.
Change-Id: Idf5ba286415fb2b90e2cd1d5ba3635b6b559aef6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132164
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
One minor issue remains: when dead code elimination causes a
collection to become empty, migration should insert explicit type
parameter(s) to ensure that the empty collection will still get
analyzed as having the right type. This is noted in TODO comments.
Change-Id: Ibed25f6cb8ed9eec3ddccb0444178960c6b35c84
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/131843
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
Looks like we used to do this, but we stopped when we started changing
this pattern:
```
dynamic x = 1;
int y = x;
```
Our first iteration treated `dynamic` as nullable all the time. This is
not ideal in the example above. So we went back and instead began
treating `dynamic` as nullable only when it was observed to have a null
value.
However, in the case of unbounded type parameters:
```
class C<T> {}
```
We don't want to wait to observe:
```
C<Object> o = C(null);
```
before treating the bound as nullable.
Change-Id: I50b8ca6188d82cd62f795ada99a355afe2eae771
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/131714
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Previously, TypedLiteralResolver obtained them from the library, but
this didn't work when the resolver was invoked by the migration tool,
because the migration tool needed to re-run migration of a non-NNBD
library using the NNBD TypeProvider and TypeResolver.
Change-Id: I73acca120a726c773c2e08b65d1227531a5fd2e1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/131704
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Consider a generic AST structure like this:
A
/ \
B C
/ \
D E
Previously, if we wanted to build a "pass through" edit plan for A,
incorporating inner plans representing changes to be made to B, D, and
F, we would do the following: (1) pass each inner plan through the
_incorporateParent function, which might leave it unchanged or might
produce a plan targeted at the parent node, and then (2) ask each
inner plan for its set of changes, and produce a new plan for A
containing those changes.
The purpose of step (1) was to allow an edit plan to describe changes
that might affect the node above it. For example, changing an
expression might cause the parentheses surrounding it to become
unnecessary, so those parentheses would be removed (that's the case
that's currently implemented). Or, removing a list element from a
list might necessitate adjusting the comma before or after it, which
belongs to the parent node (this isn't implemented yet).
This had a design flaw: step (1) might produce overlapping changes.
For instance, calling _incorporateParent on the plans for D and E
might conceivably both plans for C, and those plans might try to make
overlapping (and thus incompatible) changes to node C.
It turns out that this isn't a problem for eliminating unnecessary
parens, because it's impossible for two edit plans to target two
different non-overlapping AST nodes whose parents are both the same
ParenthesizedExpression node (because a ParenthesizedExpression has
only one child). But it will be a problem for removing elements
lists, statements from blocks, declarations from classes, etc.
The new strategy is that we will bubble inner plans up the tree one
level at a time. In the example above, we would first combine the
inner plans for D and E into a pass through plan for C. Then we would
combine the plans for B and C into a plan for A. This is a bit more
work, but I believe it can be made efficient in typical use cases, and
it will avoid the problem described above because at each stage, all
the inner plans we combine will have the same parent, so it will be
tractable to figure out their interactions with each other.
Some vestiges of the old _incorporateParent approach still remain; I
plan to remove them in a follow-up CL.
Change-Id: I9ecbdb1549b949177ffc5d66447d4aede77e2dd9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/131400
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
The derived class, NodeProducingEditPlan, represents an edit plan
whose effect will be to transform the source AST node into another AST
node (e.g. replace an expression with one of its subexpressions). The
base class, EditPlan, will be able to represent more general kinds of
edit plans.
Future CLs will introduce edit plans that don't result in an AST node,
e.g. an edit plan that removes a source statement or a collection
element, or an edit plan that replaces one source statement or
collection element with several.
Change-Id: I01455f643b858c6500d864be679b12768029cffb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130941
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
In later CL's, I will make it possible for dead code to be deleted
instead of commented out. This change prepares for that, by
explicitly noting which API tests assume that dead code is commented
out.
Change-Id: I4e0ff6f1de4c0ab5fda050956944725b2d23dc5e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130931
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
In upcoming CLs, I'm going to need access to the EditPlanner when
finalizing edit plans, and I'd rather do so by making finalize a
method on EditPlanner. Doing so now to make future code reviews
simpler.
Change-Id: I6182e2224b548b2919f0c8f454f558b72c17e5d5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130926
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This factory class serves three purposes:
- In a future CL, it will contain fields to customize the global
behavior of the edit plans that are created (e.g. whether code
should be removed or commented out).
- In a future CL, it will contain a pointer to the original (text)
contents of the source file. This will allow edit plans to make
more sophisticated changes such as adjusting indentation.
- In a future CL, it will contain methods for creating common kinds of
"extract" edit plans such as null-checking an expression or making a
type nullable. (Currently the client must create these edit plans
using EditPlanner.extract, which has an error-prone API).
Change-Id: I87ac75b73003d0e7b416e3d2121eadb2770da5ed
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129802
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
We were getting a static error in package:logging for
```dart
Zone recordingZone;
// ...
runZoned(() {
recordingZone = Zone.current;
// ...
});
// ...
expect(records.first.zone, equals(recordingZone));
```
That is a static error if we don't make `recordingZone` late or
nullable.
Change-Id: Iab5e47dd8df375c7dc5283ce4570ad1321ba919e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130738
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Turns out the flow analysis bug was actually an issue with calling
toString() on an enum. This happened to throw the same type of exception
as caused by the shared part file, so it blended in. Flow analysis was
not itself broken.
Tested the other methods on enum too, but no others seemed to have this
problem.
Change-Id: I5e9e1be3f02adf0177c276c63d0c4f4fa2cb94db
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130622
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Mike Fairhurst