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>
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>
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>
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>
This change moves the `variableType` method from the class
`TypeOperations` to a new class, `VariableOperations`, which in turn
allows removing the type parameter `Variable` parameter from
`TypeOperations`. A new class, `Operations`, is introduced to serve
the role served previously by `TypeOperations` for flow analysis
clients (i.e. it is the base class that clients should extend).
This paves the way for a future CL that will remove the type parameter
`Variable` from other classes inside flow analysis.
Bug: https://github.com/dart-lang/language/issues/2020
Change-Id: Ic45d07a0f873b692fda4b6f807c1130ac592b010
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/250108
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The migration tool has logic to suppress some of its heuristics for
suggesting `required` in redirecting factory constructors, to account
for the fact that a redirecting factory constructor is allowed to have
a non-required non-nullable argument with no default. But that logic
was overzealous and was preventing `@required` from being turned into
`required` (which is clearly still a good idea). This pattern crops
up frequently in classes using BuiltValue.
Bug: https://b.corp.google.com/issues/234631651
Change-Id: Ic0428a878184bb837d3f5b79cb6585ec4fa30344
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247361
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Half a second was becoming tedious as my current workflow means I
collapse 20 directories each time I re-run from source.
I'd love the tree to retain its state with re-runs, but that is more
work.
Change-Id: Ie53a535a05b445056f87b80131f96f78ae98de2d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/245562
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
The "Proposed Edits" panel displays the number of edits, but only when
this number not immediately displayed below and would need to be
calculated.
Change-Id: I89793305876d5488c9d14b03a91470ae3f4fe4f8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/241340
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Stephen Adams <sra@google.com>
This would be a breaking change, so for now it is disabled, but
the method `applyPendingFileChanges()` can be added, and the clients
will call it to be ready for the switching the flag.
Only clients that call methods like `changeFile` have to await applying
the file changes. Other clients can continue using synchronous
`currentSession`.
Change-Id: I0f8d4cc874f485776f611790f6c80fd7e07c8051
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/236041
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
This is a better migration because the type of
`Iterable<T?>.whereNotNull()` is `Iterable<T>` (whereas the type of
`.where(...)` is `Iterable<T?>`), so this avoids propagating
nullability unnecessarily through the program.
Change-Id: Id1f82422cedc7ba76d03fdc11f33d459533d2550
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/234623
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, we only inserted these nodes when the
`constructor-tearoffs` feature was active, as a way of reducing the
risk of breaking analyzer clients; however this behavioral
inconsistency is not something we want to keep for the long term.
Note: even though this is technically a breaking change, we haven't
found any analyzer clients that are affected by it, so we're going
ahead and landing it without an analyzer version number bump.
Change-Id: I71f0fb2862b644dd1a81245bd12f5b7b9ca45857
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/233653
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, when the migration tool encountered a built_value
`@nullable` annotation, it removed it, because when the built_value
code generator is consuming a library with null safety enabled, it
relies on the presence of a `?` to determine nullability, rather than
an annotation. However, there was no logic to actually ensure that
the `?` would actually get introduced, because I thought the code
generated by built_value would always create the conditions necessary
to convince the migration tool to introduce the `?` using its normal
graph traversal algorithm.
It turns out this is not the case: when `@nullable` appears in an
interface class that is used by other built_value classes, but is not
itself a built_value class, the migration tool sometimes doesn't have
enough information to figure out that it needs to add the `?`.
So in this CL, I'm doing what I probably should have done in the first
time: adding the necessary logic to the migration tool to ensure that
the `@nullable` annotation gets translated into a `?` regardless of
whether it is required to by generated code.
Bug: https://buganizer.corp.google.com/issues/217863427
Change-Id: I9efe5241634389981a4c56e764bac91b3350c4fb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/233003
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Annotations that refer to named constructors are represented a little
strangely in the AST, with Annotation.name being bound to a
PrefixedIdentifier and Annotation.constructorName being bound to
`null`. This was confusing the EdgeBuilder, causing it to visit the
class name as though it were an expression, and then crashing because
there was no associated expression type.
The solution is to simply not visit Annotation.name at all, because
there's no way it can have any effect on null safety.
Bug: https://buganizer.corp.google.com/issues/217386404
Change-Id: I2baf0a9e8d63a4a5bbff1b2c5ee2aeec52b2844a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/232031
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Paul Berry