Previously, if a variable with a generic type was null-checked, and
the migration engine did not add a question mark to the type of the
variable itself, e.g.:
f<T extends Object?>(T x) {
if (x == null) { ... }
}
Then the migration tool would erroneously consider the null check
unnecessary. It failed to account for the fact that the bound on the
generic type might be nullable.
This change fixes the tool so that it now properly accounts for the
bound on the generic type variable.
Bug: https://b.corp.google.com/issues/247496662
Change-Id: I2b131e75b7e3fc5db0b668f1064340d404442c82
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/264501
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Tested with:
* top-level variables
* local variables
* instance and static fields declared in classes and mixins (enhanced enums are not supported pre-null safety)
Fixes https://github.com/dart-lang/sdk/issues/41389
Change-Id: I8a89668fffe64fff508d0aeb36aaebef5c84f223
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/263460
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Previously, when the migration tool needed to change one type variable
to another during a substitution, it created a DecortedType to
represent the substitution using
`DecoratedType._forTypeParameterSubstitution`. This was problematic
because the resulting DecoratedType had a null nullability `node`.
This in turn forced nearly every reference to `DecoratedType.node` to
be followed by a `!` operator.
With this change, we now have a new base class, `SubstitutedType`,
which can either be a `DecoratedType` (representing a fully general
substitution) or a `_TypeVariableReplacement` (representing a simple
change from one type variable to another). This paves the way for a
follow-up CL that will make `DecoratedType.node` non-nullable.
Change-Id: I7fdaef57994c7678ecd517ab4fe84d00f0d81424
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/261941
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously we expected the client to provide a set of promotable
fields, which worked well for testing. But using a callback gives the
client more flexibility (e.g. it would allow field promotability to be
computed on demand based on other data).
Bug: https://github.com/dart-lang/language/issues/2020
Change-Id: I40683a4c19a17147e86f0faa1339aa025da35229
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256962
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This issue (Migration tool null safety detection fails when tests use
path imports) was reported in December of 2020 and I can't reproduce
it. I believe there have been changes in analyzer package resolution
since then, aimed precisely at helping users who use this sort of
improper path import. So it is likely that those changes fixed this
bug.
I'm adding a test case to ensure that the bug isn't accidentally
un-fixed by future changes.
Bug: https://github.com/dart-lang/sdk/issues/44394
Change-Id: I81490e545aa41196c3c69bc4d74cd481079d59ba
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/259200
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
I've begun prototyping what it might look like to integrate the
current shared type analysis functionality with the analyzer and CFE,
and I've discovered some API improvements that are needed:
- The shared logic now handles the possibility that switch cases that
share a body have been merged prior to type analysis (because the
CFE merges them during parsing), in addition to the pre-existing
functionality which assumed that switch case merging had to be done
in the shared logic.
- The shared logic now returns several pieces of information as the
result of a call to `analyzeSwitchStatement`: whether the switch
statement had a `default` clause, whether it was exhaustive, whether
the last case body terminates, and the type of the scrutinee. These
are all needed by the CFE.
- The shared logic now allows `TypeAnalyzer.errors` to be `null`,
indicating that no errors should be reported. This reflects how
errors are suppressed during top level inference in the CFE.
- If a switch case lacks a `when` clause, this is reported by calling
`handleNoWhen` rather than passing a boolean to `handleCaseHead`.
- The shared logic now reports the appropriate error when a case
constant doesn't properly match the scrutinee's static type.
- Information about case labels is now delivered to flow analysis via
`switchStatement_endAlternatives` rather than
`switchStatement_beginCase`. This made it possible to rewrite the
shared `analyzeSwitchStatement` method in a way that requires less
bookkeeping, because it no longer has to peek ahead to look for
labels associated with a given case body.
- `TypeAnalyzer.analyzeExpression` is now responsible for
understanding that "no context" and a context of `dynamic` should
both be coalesced to `?`. The analyzer does this (although it's not
100% why), and it's definitely "business logic" that eventually
belongs in the shared type analyzer.
- `TypeAnalyzer.analyzeSwitchExpression` and
`TypeAnalyzer.analyzeSwitchStatement` no longer receive a list of
ExpressionCaseInfo / StatementCaseInfo objects describing the cases;
instead they query for them using a callback. This reduces the
lifetime of the ExpressionCaseInfo / StatementCaseInfo objects. In
the future, when we have record support, we could replace these
objects with records, which would then be passed on the stack,
avoiding any allocations.
- A new hook, `handleSwitchScrutinee`, is called right after visiting
the "scrutinee" expression of a switch expression or switch
statement. This hook is needed by the analyzer to compute
exhaustiveness. In a future CL, I hope to move exhaustiveness
analysis into the shared code as well, which should make this hook
unnecessary.
- `TypeAnalyzer.analyzeSwitchStatement` now reports an error if a
switch case completes normally and pattern support is not enabled.
- The test class `_MiniAstTypeAnalyzer` no longer overrides
`analyzeExpression` to provide a default context type; instead,
every call to `analyzeExpression` that didn't previously provide a
context now provides a context of `?`. Note that not all of these
are correct, but they are close enough for the unit tests we have
today. I plan to fix them in future CLs as I replace this logic
with shared logic.
- The hook `handleVariablePattern` is now always provided with a
static type. Previously, it was only provided with a static type if
this was the first time the variable was bound in the pattern.
Change-Id: I70e3c5468312a9329fcf4ad2e13749a32d2418e7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/257487
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
With this change, the migration tool no longer warns "Null-aware
access will be unnecessary in strong checking mode" for the specific
case where the target of a null-aware access is a non-nullable method
or getter invocation, and the method or getter in question is in
already-migrated code. The whole point of the warning is to help the
user identify situations where the migration tool might have made the
wrong decision about code that it's currently migrating; that's not
useful if the method or getter was migrated previously. So instead,
in these circumstances, the migration tool simply changes `?.` to `.`.
Fixes https://github.com/dart-lang/sdk/issues/49601.
Bug: https://github.com/dart-lang/sdk/issues/49601
Change-Id: I8fe26df696dafa28ac1104f4b721f29bfe7ba164
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256646
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The migration of the migration tool was done hastily (because it was
not expected to need much more maintenance) so it contains a lot of
nullable fields that don't need to be nullable. I've been cleaning
them up as I run across them in order to unblock other work. This is
one such cleanup.
Bug: https://github.com/dart-lang/sdk/issues/49601
Change-Id: I68413e23f565bc9d38af3345d28059d734a3c689
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256021
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
If a parameter of a public method is accessed using null-aware access
(`?.`), the migration tool now considers that to be an indication that
the parameter should be nullable.
Rationale: when migration doesn't have access to the entire code base,
the migration tool's approach of propagating nullability forward
through the program doesn't always work, because it's possible that
the sources of nulls are not visible to the migration tool. This has
often resulted in the migration tool marking a function parameter as
non-nullable, in spite of the fact that the use of `?.` in the method
body makes it clear that it's intended to be nullable.
This new heuristic is only applied to public methods; for private
methods there's no chance of there being callers outside of the code
that's immediately visible to the migration tool, so the problem
doesn't arise. For local functions and closures, the situation is
less clear-cut, but of the examples I've looked through so far in
Google's internal code base, it seems like more often than not, the
better behavior is to continue erring on the side of non-nullability.
Bug: https://github.com/dart-lang/sdk/issues/49601
Change-Id: I76531591ce0b3eb9fe62273130aa45eb4ff6d456
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253864
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This is a reland of commit 6abea1381b
Original change's description:
> Migration: fix generate_resources.dart to use `dart compile js`.
>
> This is necessary because the command `dart2js` is no longer present
> in the SDK.
>
> Change-Id: Iea3cf3092bcda6c889c55a51d1b5601773404fb9
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253862
> Reviewed-by: Samuel Rawlins <srawlins@google.com>
> Commit-Queue: Paul Berry <paulberry@google.com>
Change-Id: Ibc25641aca165b578b2c4348ef0b841263298e3c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255740
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
These fields can be `null`, but if they aren't, then the
`DecoratedType`s they contain cannot be `null`.
Change-Id: I9f07d06017ff9814e531f3517b4f452766dc0ebf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/255341
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This script does the same job as `generate_resources.dart`, but it
doesn't search the filesystem for the input files to use, nor invoke
the compiler to create the `migration.js` file. Instead, it simply
accepts a list of input files on the command line, and outputs their
contents in an appropriate form to be stored in `resources.g.dart`.
This script will allow us to integrate more cleanly with the internal
build system.
Change-Id: I4b5445d1f5fd7eccbb5fb36f6ee4fa97d2cea87e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254421
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Unfortunately my idea about checking for specific `DartType` subtype,
and only then asking for the element is too punitive. It almost
works in google3, but the amount and the kind of changes I had to
do make me realize that we should keep `get elementX` in `DartType`.
I guess this is the same as we had in AST when `get constructors`
does not make sense for mixins (?), but works for classes and enums,
and it is easier to pull it into the superclass.
Change-Id: Ibc4fac0b95d63748fa65de96d29300f477fdfc76
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254482
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
This change breaks flow_analysis.dart into the following libraries:
- assigned_variables.dart (for the AssignedVariables class and related
code)
- promotion_key_store.dart (for the PromotionKeyStore class)
- type_operations.dart (for the TypeOperations mixin and related code)
- flow_analysis.dart (for the rest of flow analysis)
And it breaks mini_ast.dart into the following libraries:
- flow_analysis_mini_ast.dart (functionality specifically concerned
with testing flow analysis)
- mini_ast.dart (functionality not specifically related to flow
analysis)
This is in preparation for trying to share some more type inference
behaviors between the analyzer and CFE.
Note that although the diff is big, the only changes in this CL are
moving code from one place to another, renaming some class members
from private to public, and updating imports.
Change-Id: I71768f03b1e75ed754c7b7af39f6cf7f03c4fe44
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254462
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This involves changing the target expression which must be null-checked
or awaited. When we detect the FutureOr and the await expression, we
instead null-check the await expression.
Fixes#44041
Change-Id: I08cf03f6975e2e70d5c87ebda92e9538de19e02e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253863
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
This is necessary to separate `ClassElement`, `EnumElement`, and `MixinElement`. And, in the future, augmentations like `ClassAugmentationElement`, etc.
Change-Id: Iecd2f8707212e53ef56f0e101880c7bab9e5d057
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254104
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This is safe because this field is always initialized to a non-null
value before the EdgeBuilder is invoked.
Also, make `MigrationVisitorTestBase.variables` non-nullable; this is
safe for similar reasons.
Change-Id: I3a79efa8fb602dfe417a1e53937ddac4e129c86f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253860
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This reverts commit 6abea1381b.
Reason for revert: Breaks internal build of migration tool
Original change's description:
> Migration: fix generate_resources.dart to use `dart compile js`.
>
> This is necessary because the command `dart2js` is no longer present
> in the SDK.
>
> Change-Id: Iea3cf3092bcda6c889c55a51d1b5601773404fb9
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253862
> Reviewed-by: Samuel Rawlins <srawlins@google.com>
> Commit-Queue: Paul Berry <paulberry@google.com>
TBR=paulberry@google.com,srawlins@google.com
Change-Id: I73c3321e6c473f5690eaddd94d13003c978234b6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253900
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This is necessary because the command `dart2js` is no longer present
in the SDK.
Change-Id: Iea3cf3092bcda6c889c55a51d1b5601773404fb9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253862
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
For `InterfaceType` keep `element2` deprecated and define
`InterfaceElement get element2` instead. Most changes are because
of this.
Change-Id: I13b888610fc707438c3c97b676f1460e7fc2b040
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253564
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Closes https://github.com/dart-lang/sdk/pull/49478
TEST=Manual
GitOrigin-RevId: f4c9c6869dfe73639295e86574a021523b3d374d
Change-Id: I134a97caed4eec59d70e9cbca16b7e9a472cf2c1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/251902
Reviewed-by: Michael Thomsen <mit@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Kevin Chisholm <kevinjchisholm@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
We need this for:
1. CompilationUnitElement changes its enclosing element from LibraryElement to LibraryOrAugmentationElement
2. Similarly PrefixElement.
Change-Id: I5e3719b4ef59d03caab1b20c9172a8c0fd786bdf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/251900
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
