Previously, initializers of implicitly typed variables did not
contribute to the SSA tracking performed by flow analysis (see
https://github.com/dart-lang/language/issues/1785). This change fixes
the bug, however the fix is conditioned on the "constructor tearoffs"
language feature to avoid compatibility issues (we don't want someone
to publish a package taking advantage of the fix, without realizing
that it makes their package unusable on older SDKs).
TEST=standard trybots
Bug: https://github.com/dart-lang/language/issues/1785
Change-Id: I1143440c7a9795b059e8f4b84e3f4125cd80732c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/211306
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Previously, the analyzer and CFE were responsible for two pieces of
logic that arguably should be in the shared flow analysis engine:
- Deciding whether or not it's necessary to tell flow analysis about
initializer expressions.
- Deciding whether or not to promote the variable at the time of
initialization (we do this when the variable is implicitly typed,
and the initializer is a promoted type variable type).
It's better to just always tell flow analysis about the initializer
expression and let it decide what to do about it.
This paves the way for fixing
https://github.com/dart-lang/language/issues/1785 (which results from
initializer expressions sometimes being ignored when they shouldn't
be), by consolidating the broken logic into the flow_analysis library,
where it will be easy to unit test the fix.
Bug: https://github.com/dart-lang/language/issues/1785
Change-Id: Iec832e92995eb4f8d0c1fbd4e9be6c897e0917b5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/211180
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This logic was previously in LibraryScope and ScopeExtension. It
really isn't related to scopes, though, since all it does is to look
through the library's imports and parts. (The only use it made of
scopes was to locate the library element).
Separating this code from scopes will facilitate some other changes
I'm working on to streamline the use of scopes in the analyzer.
Change-Id: Ic45e3382628a433a96cc7ecf751b4df5d681b42a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208522
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This is reasonable, since every CompoundAssignmentExpression *is* an
expression, and it allows clients (such as the migration engine) to
avoid nuisance casts.
Change-Id: Ic6f0fc88672248e2de4410a0f1c16b7f476fa55c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/207360
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
In https://dart-review.googlesource.com/c/sdk/+/191862 we added two new
required fields to `Location`. Unfortunately this was a breaking change
because plugins using an older version of the `analyzer_plugin` produce
location objects without those fields, leading to deserialization failures.
This CL makes those fields optional in order to fix the deserialization
issue.
Unfortunately, the `analyzer_plugin` package was published after the
required fields were added. Making them optional is a breaking change
because the constructor parameters go from being positional to being
named parameters.
We also neglected to update the version number of the protocol as part
of the previous CL. Technically this is also a breaking change for clients
of the analysis server, but given that they had no way to test to see
whether these fields existed they would need to have been written as if
the fields were optional in order to reference them at all, so I think
that from a practical standpoint it isn't a breaking change. That does,
however, raise the question of whether we should increment the version
numbers as part of this CL.
Change-Id: I35fc1f8e950669a3d8dd33cee6b81890261b5c47
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206942
Reviewed-by: Danny Tuppeny <danny@tuppeny.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
The migration tool already correctly infers nullability or
non-nullability based on the code generated by the built_value
package. However, post-migration, code should no longer use
built_value's `@nullable` annotation, since the built_value package
gets its signal from the `?` type suffix instead.
The migration tool already has the ability to remove annotations in
certain rare circumstances; it's a relatively trivial matter to extend
it to remove `@nullable` annotations when appropriate.
Note that we don't just remove any `@nullable` annotation; we only
remove those annotations that resolve to the `nullable` constant
defined in the `built_value` package.
Fixes#43878.
Bug: https://github.com/dart-lang/sdk/issues/43878
Change-Id: I39c9b5e6fe4cd149b6d9549cd785d7b87c3185d6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206940
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Previously, when removing all metadata nodes from code such as this:
f({
@removeMe
int? i}) {}
If the node(s) being removed occupied entire lines (as they do in this
example), the logic for trying to decide whether to collapse together
empty `{}`, `[]`, or `()` would get confused by the fact that the text
being removed began at the start of the line, even though the parent
AST node (`@removeMe int? i`) didn't begin until partway through the
line; this would cause an assertion failure (or, if assertions were
disabled, an out-of-range error).
As of yet, this has never been a problem in practice, because the only
circumstance in which the migration engine removes metadata is when
adding other metadata (and thus the logic for trying to collapse
together empty `{}`, `[]` or `()` doesn't trigger). But in a future
CL I'll be adding logic that *can* remove the only metadata and
trigger this problem. So to prepare for that, this CL fixes the
out-of-range problem.
Change-Id: Ie2d050d656512a7c725752427eaaec1c087f3c82
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206941
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change makes the migration tool understand that the method
`BuiltValueNullFieldError.checkNotNull` expresses non-null intent.
Since the built_value code generator calls this method from generated
built_value constructors, this is sufficient to ensure that built
value getters that lack the `@nullable` annotation will be treated by
the migration tool as non-nullable.
Bug: https://github.com/dart-lang/sdk/issues/43878
Change-Id: I37a9ce584a10d2a1bb27da92f3f664632547f6ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206881
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Previously, when the migration tool encountered code like this:
class C {
int i;
C(this.i) {
assert(i != null);
}
}
the assertion would not confer non-null intent on the `i` parameter of
the `C` constructor, because the `i` referred to in the assert
statement is technically the `i` field, not the `i` parameter (and we
do not propagate non-null intent through fields).
With this CL, the migration tool treats field references in
constructors to refer to field formal parameters when possible; even
though this technically does not exactly conform to Dart semantics, it
captures user intent far better; the only cases where this would
produce an incorrect result are when the field is reassigned and then
used in the constructor; such cases are quite rare and should be easy
for the user to compensate for using migration hints when necessary.
Change-Id: I4c624dab051e62fa29f103ad08ce2ce9c5137663
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206880
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This class wasn't really necessary; it was just a container for a few
extra methods used only by the EdgeBuilder. By moving those methods
to the EdgeBuilder and removing the _ScopedLocalSet class, we open up
the possibility of expanding the capabilities of those methods to take
advantage of other information the EdgeBuilder has access to. (I will
do this in follow-up CLs).
No functional change.
Change-Id: I7cf19b8cf81db0413f4a9e747d46fb619d5eac4e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206840
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, _ScopedLocalSet extended ScopedSet<Element?>, but this was
unnecessary; it was an artifact of the fact that the migration tool
was migrated to null safety in a conservative way. We know that all
elements of a _ScopedLocalSet are non-null elements because they all
represent actual variables declared in the user's code.
Change-Id: I4d0c86997ff3c783cee6ea84cff80363e14c3060
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206670
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This is no longer needed now that the whole migration engine has null
safety enabled.
Change-Id: I834bfb18f9b4d838c0cc933acb6977566bc1643a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204484
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This change adds just enough migration hints so that I can run the
migration tool on itself and get reasonable results. Most of the
hints are just marking a few types as nullable or non-nullable that
the migration tool isn't able to correctly infer by itself. In a few
circumstances I've had to make a type explicit that was previously
inferred, so that I could mark it as nullable. In a few other
circumstances I've had to add `/*!*/` after an expression to let the
migration tool know that the expression should be null checked.
Note that unlike most of the rest of the Dart SDK, the migration tool
is not expected to be a long term part of the Dart ecosystem, since
the whole point of it is to help people migrate their code to null
safety. It's not likely to receive a lot more maintenance, and it
will likely be removed sometime around the time of Dart 3.0 once
people stop needing it. Accordingly, I've tried to err on the side of
liberally allowing types to be nullable where possible. Although this
means that we will wind up with more null checks in the final result,
it makes the migration process a lot easier and substantially reduces
the risk of introducing bugs during the migration process.
Change-Id: Ia348a09e94d22d83c2c15215ca7ddf5a5a4d05f9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204480
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
In order to migrate the migration tool to null safety, it can't have a
dependency on `package:dartdev`, because `package:dartdev` isn't
migrated yet. (This is a problem because once the migration tool's
tests have been migrated to null safety, they will require the
transitive closure of all the packages they depend on to also be
migrated).
Fortunately, it's quite easy to change the migration tool so that it
doesn't depend on `package:dartdev`; we only need to move the class
`MigrateCommand` from `package:nnbd_migration` to `package:dartdev`.
To make this work, we have to move the constant `migrationGuideLink`
from `MigrateCommand` to `MigrationCli`, and make the method
`MigrationCli._defineOptions` public.
Change-Id: Ie1ce8db0e8245e98bbb72facddd723300816340e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204540
Reviewed-by: Devon Carew <devoncarew@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
I removed the postmortem functionality in
203d0ed789 but instead of removing the
postmortem tool itself, I merely removed the body of its `main`
method. This CL finishes the removal.
Change-Id: I525afe40ab3b69b81edc28b0fdded06fed5710c6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204320
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
I've recently wasted a lot of time tracking down strange behaviors
that resulted from inadvertenly writing a migration API test
containing a static error. Failing the test early if there's an error
should avoid that problem in the future.
Change-Id: Idc9d84901a517ce9555fa3180f5c669aea449ea5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204180
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Data was being stored in this field but never read.
Change-Id: Ida763236b71401ba8ddd0ec5bf4880e7cc657d6a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204281
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Factory redirecting constructors inherit their parameter default
values from the constructors they redirect to, so lack of a default
value on one of their optional parameters doesn't mean it has to be
nullable.
Change-Id: I9572f4ac954e76a391c65515b30bdf613a78774c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203380
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Previously, such a hint would influence the nullability graph, which
would usually cause the migration tool to decide that a `required`
keyword was necessary. However, this didn't always work (for example
if the type was nullable, the hint would be ignored). Also, even when
the hint was accepted, the tool would insert a `required` keyword and
leave the hint; now it converts the hint into the keyword by dropping
the `/*` and `*/`.
Change-Id: I0620ceaef3fd5a13cfddf18c15e12e50e7574d73
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204062
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The only elements that were ever stored in this set were parameter
elements, and the only time this field was used was in the condition
`... && !_requiredHintedParameters.contains(staticElement) &&
!_flowAnalysis.isAssigned(staticElement)`. However, flow analysis
considers parameter elements to always be assigned. Therefore, if
`!_flowAnalysis.isAssigned(staticElement)` is satisfied, then
`!_flowAnalysis.isAssigned(staticElement)` must also be true. So
testing the two of them is redundant. And therefore we can remove
tracking of EdgeBuilder._requiredHintedParameters entirely without
affecting behavior.
The reason I want to do this is because we had a confusing
inconsistency in how we were populating
EdgeBuilder._requiredHintedParameters: we were only tracking
parameters whose hints took the form `/*required*/`; we were not
tracking parameters whose hints took the form `@required`. It seems
like since the whole variable has no effect, it's better to just
remove it rather than fix the inconsistency.
Change-Id: I3b36317e60e08b0577c504a773428b85fb9cdce1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203825
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
We used these "post-mortem" files for some early debugging, but they
were never useful enough for us to get around to hooking up a way for
the user to enable them. At this point, keeping around the capability
doesn't buy us anything, and adds to maintenance burden.
Change-Id: Ib25a98187b63cf785941a1b750615455dfea237c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203800
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, the migration tool had a small amount of special case
logic in `EdgeBuilder._dispatch` to ensure that if an `as`
expression's type was annotated with `/*!*/`, that `/*!*/` wouldn't be
misinterpreted as also applying to the `as` expression itself (causing
a `!` operator to be inserted). This logic wasn't sufficiently
general; it failed to handle `is` expressions (which can run into the
same issue), and it failed to handle situations where the `is` or `as`
expression was at the end of a larger expression (e.g. `y = x as
T/*!*/;`).
This change removes the special case logic and replaces it with a more
general-purpose mechanism: when visiting any type name, if it ends in
a `/*!*/`, the edge builder stores information about it in the
`_nullCheckHints` field. This ensures that when visiting an
expression ending in that type name, the `/*!*/` will be ignored
(since `EdgeBuilder._handleNullCheckHint` skips any hints that are
already stored in that field). This ensures that we properly handle
both `is` and `as` expressions, as well as larger expressions that
contain them.
Change-Id: I2b902ef809b4cc5eb8b493fa4405c0f0c8c10a96
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202261
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Prior to this fix, if the user's package had a relative import from
outside `lib` to inside `lib` (e.g. from `test` to `lib`), the
migration tool would regard the imported file as being reached via a
`file:` URI. This caused it to get confused and complain that the
user had a dependency on unmigrated code (this happened because the
check for dependency on unmigrated code excluded the user's files via
their canonical URIs, and files inside `lib` use `package:` for their
canonical URIs).
To fix the problem, we modify the check for dependency on unmigrated
code so that it excludes the user's files via their path rather than
their URI.
Note that relative imports into the `lib` directory are discouraged by the style guide*, but it still seems worth fixing this bug since they do crop up in the wild.
*https://dart.dev/guides/language/effective-dart/usage#dont-allow-an-import-path-to-reach-into-or-out-of-libFixes#45780.
Bug: https://github.com/dart-lang/sdk/issues/45780
Change-Id: Iff41ca0059d78bbb812dd6f421be6458e7049895
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196344
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Prevents file corruption if the user adds hints and then tries to
apply the migration result without running migration to account for
the hints.
Fixes#45571.
Bug: https://github.com/dart-lang/sdk/issues/45571
Change-Id: Id11584a1d8b7a6b35127ad828d37b3ef837a5e65
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194009
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
