Previously, the logic for enabling and disabling language features in
flow analysis and type inference tests relied on setters in the
`Harness` class that (a) were mostly unmatched with getters, and (b)
were almost exclusively used in just a single one direction
(e.g. `Harness.legacy` defaulted to `true`, so it was only ever set to
`false`).
Cleaned up so that there are explicit `enable` and `disable` methods
in the `Harness` class to cover all the use cases.
Change-Id: I5ccc8585f803fec634cad1472395ea0d135c87c6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332064
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
If the user attempts to promote a property, and their language version
does not permit field promotion, the "why not promoted" logic now
checks whether the language version is the sole reason for the failure
in property promotion. In other words, it checks whether the property
would have been promotable *if* field promotion had been enabled. If
it would, then the context message displayed to the user explains that
the property did not promote because field promotion is not supported
by the current language version.
However, if there is some secondary reason why the property failed to
promote (in other words, if the property would not have been
promotable even if field promotion had been enabled), then the context
message now favors the secondary reason.
Rationale: imagine a user is maintaining a package that doesn't yet
support SDK version 3.2, and that package contains some property
that's non-promotable both because the language version is prior to
3.2 *and* for some other reason (e.g., because the property isn't a
private field). It would be quite frustrating if the user saw a
context message suggesting that the property would be promotable in
SDK 3.2, and then went to a lot of effort to bump their minimum SDK
version, only to discover *after* the bump that the property is still
not promotable.
In the process of making this change, I discovered that the CFE
doesn't support field promotion in patch files. This is because patch
files aren't listed in `SourceLoader.sourceLibraryBuilders`, so the
logic in the `FieldPromotability` is never invoked for those
files. Since patch files are an artifact of SDK development, and will
never be used by end users, it doesn't seem worth going to extra
effort to add this support. However, I've taken care to make sure that
the "why not promoted" logic recovers gracefully in patch files (by
simply not generating a context message).
Change-Id: I6c0d1c0f4b8a7690f6f775408cb5e857b2dd7b03
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330241
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
Previously, when field promotion failed due to a conflict with another
declaration in the same library, the client (analyzer or front_end)
was responsible for returning a value of
`PropertyNonPromotabilityReason.isInterferedWith` from
`Operations.whyIsPropertyNotPromotable`. Flow analysis would propagate
this value into a `PropertyNotPromoted` object whose
`documentationLink` getter returned `null`. The client was then
responsible for tracking down all the conflicting fields and getters
and creating the appropriate context messages for them (but this
functionality wasn't implemented yet).
With this change, the `PropertyNotPromoted` is now abstract, with two
subclasses to represent the two cases the client has to handle:
- `PropertyNotPromotedForInherentReason` to cover the case where a
property cannot be promoted due to the fact that it is inherently
not promotable (i.e. it's not final, it's public, it's external,
it's not a field, or it's in a library where field promotion isn't
enabled). In this case the client simply has to generate the
appropriate context message and attach it to the site where the
property is declared, and it can rely on having access to a non-null
`documentationLink` to include in the context message.
- `PropertyNotPromotedDueToConflict` to cover the case where the
property cannot be promoted due to a conflict with some other
property in the same library. In this case the client has to
generate multiple context messages, one for each conflicting
declaration, and it has to associate each one with the appropriate
documentation link from the `NonPromotionDocumentationLink` enum.
The `NonPromotionReasonVisitor` base class has been updated to reflect
this split, so that the logic for handling these two cases is in
separate methods in the client.
The front_end logic for handling non-promotion due to conflict is now
fully implemented. The analyzer logic will be addressed in a follow-up
CL, since it's more complex (it requires plumbing additional data
through the summary file format).
Finally, the nomenclature in the `FieldNameNonPromotabilityInfo` is
adjusted to match the new context messages.
Change-Id: Ieed70d1a3572abbc726ae34584d85c7a8aee0732
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327712
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
- When the shared logic in `field_promotability.dart` decides that a
given field should be non-promotable based on attributes of the
field itself (i.e. because it's public, non-final, or external), it
returns information to the caller about the reason for
non-promotability.
- This information is recorded by the analyzer and CFE, and delivered
back to flow analysis in response to a new callback method,
`whyIsPropertyNotPromotable`. Flow analysis records this information
in the `PropertyNotPromoted`, which is delivered to clients when a
compile-time error occurs due to a property access not being
promotable. This ensures that the appropriate
`http://dart.dev/go/non-promo-...` link will be associated with the
context message. In a future CL, the context messages themselves
will be updated to match the link.
- The shared logic in `field_promotability.dart` also collects, for
each field name that isn't promotable, all the reasons why that
particular field name is non-promotable, in a new data structure,
`FieldNameNonPromotabilityInfo`. In a future CL, this data structure
will be used to explain to the caller situations in which a field is
non-promotable due to interference from other fields or getters with
the same name.
Change-Id: I89ad102a4bec071bf59374971a8d83b061d4ec1d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327901
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates `FlowModel.promotionInfo`, the primary data
structure used by flow analysis to track program state, so that
instead of being a `Map<int, PromotionModel<Type>>`, it is represented
by a new data structure called a `FlowLink`, an immutable data
structure describing program state in a way that's particularly
optimized for flow analysis's usage patterns.
Like a map, a `FlowLink` data structure represents a collection of
key/value pairs (where the keys are integers), however instead of
storing the keys and values in a hashtable, each `FlowLink` object
contains a single key/value pair and a pointer to a previous
`FlowLink` object. The value associated with a given key can be looked
up by starting with the current `FlowLink` and walking backwards
through the linked list of `previous` pointers until a matching key is
found. (An empty map is represented by `null`). This makes it an
`O(1)` operation to update the promotion state associated with a
single promotion key (an operation that flow analysis performs
frequently), since all that is required is a single allocation.
If the `previous` pointers are regarded as parent pointers, all the
`FlowLink` objects produced by a given run of flow analysis form a
tree that mirrors the dominator tree of the code being analyzed.
To optimize reads of `FlowLink` data structures, there is a
`FlowLinkReader` class that keeps track of a lookup table reflecting
the implicit map represented by a given `FlowLink` object; this table
can be updated to reflect a different `FlowLink` object in `O(n)`
time, where `n` is the number of edges between the two `FlowLink`
objects in the tree. Since flow analysis is based on a depth-first
traversal of the syntax tree of the code being analyzed, it has a high
degree of tree locality in the `FlowLink` objects it needs to be able
to read, so these `O(n)` updates do not consume much CPU.
The `FlowLinkReader` class is also able to compute a difference
between the program states represented by two `FlowLink` objects, in
`O(n)` time, where `n` is the number of edges between the two
`FlowLink` objects in the tree. This is used by flow analysis to
compute the program state after a control flow join, so that it does
not need to spend any time examining promotion keys that are unchanged
since the corresponding control flow split.
For more information about the `FlowLink` data structure and how it
works, see the comments in `flow_link.dart`.
This change improves the performance of CFE compilation fairly
substantially:
instructions:u: -0.8167% +/- 0.0007% (-158214865.67 +/- 130252.25)
branches:u: -0.4694% +/- 0.0009% (-18575169.00 +/- 37220.97)
branch-misses:u: -1.0009% +/- 0.7189% (-575742.67 +/- 413521.70)
Change-Id: Ia87458ee599977e6efdc9f0e7aa283a41f84f616
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326900
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Morgan :) <davidmorgan@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
`FlowModel.promotionInfo` is currently a map from integer promotion
keys to PromotionModel data structures. This is inefficient because
FlowModel is an immutable data structure, so whenever the promotion
state of a variable changes, the map must be duplicated. In a
follow-up CL I will be changing `FlowModel.promotionInfo` to a much
more efficient data structure. However, that data structure will
require some extra plumbing. For ease in code review, I'm doing the
extra plumbing first, as its own CL.
This CL makes the following changes:
- Removes unnecessary null checks from the `FlowModel.withInfo`
constructor. These null checks are no longer needed because all the
clients of flow analysis are now fully null safe. This change is not
strictly necessary; it's just a long-overdue clean-up.
- Adds a `helper` argument (of type `FlowAnalysisHelper`) to
`FlowModel.conservativeJoin`, `FlowModel.declare`, and
`FlowModel.infoFor`, `FlowModel.inheritTested`, and
`FlowModel._updatePromotionInfo`. This is needed because these
methods will need access to `FlowAnalysisHelper` in order to read
and update the new data structure.
- Removes the `typeOperations` argument of `FlowModel.inheritTested`,
since it can be easily obtained from the new `helper` argument.
- Changes `FlowModel._updatePromotionInfo` to a public method
annotated with `@visibleForTesting`. This will be needed by flow
analysis unit tests to create the new data structure.
Note that this change causes a small regression in the performance of
CFE compilation, due to the extra `helper` arguments:
instructions:u: 0.0693% +/- 0.0008% (13413234.33 +/- 155671.09)
branches:u: 0.0886% +/- 0.0012% (3502620.67 +/- 45724.97)
The follow-up CL that switches to a more efficient data structure will
result in a performance improvement roughly an order of magnitude
larger.
Change-Id: I21c13fb817f05281b558f0473119473a26ea0fb8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326860
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
When entering visiting a subpattern of an object pattern, the flow
analysis engine now updates `_FlowAnalysisImpl._scrutineeReference` to
a `_PropertyReference` referring to the property being matched; this
ensures that if the subpattern match implies a type promotion, and the
property in question is promotable, the type promotion will be applied
to the property.
Also, if the property has already been promoted at the time of entry
to the subpattern, the promoted property type is used as the matched
value type.
Includes unit tests and language tests for the new functionality.
Fixes#53100.
Change-Id: I6d28e9a7d188bf1136e8517d6aa06af3b4c31c69
Bug: https://github.com/dart-lang/sdk/issues/53100
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323001
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Prior to this change, the SSA node stored in `PromotionModel.ssaNode`
was only correct for promotion models that represented variable
references. If a promotion model represented a promotable field, its
`ssaNode` pointed to a bogus SSA node. This had two undesirable
effects:
- It meant that `FlowModel.rebaseForward` needed to contain a hack to
prevent it from looking at the bogus SSA node for a promotable
field, and falsely concluding that the field's value had been
reassigned (which is impossible for promotable fields)--see
https://dart-review.googlesource.com/c/sdk/+/321752.
- It meant that if a promotable field was used as a scrutinee in a
refutable pattern match, the promotion logic would look at the bogus
SSA node for the field, and falsely conclude that its value had been
reassigned, preventing field promotion from working during pattern
matching.
This change ensures that the correct SSA node is always stored in
`PromotionModel.ssaNode`, and removes the hack in
`FlowModel.rebaseForward`. This required some re-ordering some of the
logic for control flow joins, to ensure that when a join creates a
fresh promotion model for a property, it has already created the
corresponding `_PropertySsaNode` (previously, it created the
`_PropertySsaNode` afterwards, but that is too late since the
`PromotionModel` class is immutable).
Unit tests and language tests are introduced to validate the newly
fixed behavior for promotable fields used as a scrutinee in a
refutable pattern match.
Also, the uses of `FlowModel.infoFor` in queries such as
`getMatchedValueType`, `isAssigned`, `isUnassigned`, and
`promotedType` were changed to simple map lookups, to prevent bogus
SSA nodes from being created and then immediately discarded. This
resulted in a fairly significant boost to CFE compilation speed:
page-faults:u: -1.2664% +/- 0.1535% (-2531.33 +/- 306.73)
instructions:u: -0.6210% +/- 0.0009% (-119891846.00 +/- 180585.35)
branches:u: -0.6765% +/- 0.0014% (-26637478.67 +/- 54272.64)
branch-misses:u: -0.9562% +/- 0.8909% (-548444.00 +/- 510991.65)
Change-Id: I30f82e8a4ba11236735258077d61d36717fa32c2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322443
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
These tests fill a coverage gap in the flow analysis unit
tests. Previously we tested that "join" variables were created by
logical-or patterns and switch cases that share a body, but we didn't
have any tests to verify that those variables could be used.
We now verify that those variables can be read from and promoted.
Change-Id: Ic8948cd307edff429aea9007183d65cc3a770ef3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322360
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
When the result of an `is` test or null check is stored in a boolean
variable, and later recalled for use in flow control, the flow models
that were computed at the time the variable was stored need to be
updated to reflect any further change to flow state that happened
between the test and the usage. This is done by
`FlowModel.rebaseForward` method. `rebaseForward` takes two flow
models as input: `this`, which represents the flow state that was
computed at the time the condition variable was stored, and `base`,
which represents the flow state at the time the condition variable is
recalled.
Flow analysis adds promotion keys for variables to the flow state at
the time their declarations are encountered, and in certain
circumstances removes them after they go out of scope. But for
properties, it only adds promotion keys when the promotion occurs. So
prior to the addition of field promotion, if `this` contained a
promotion key that wasn't present in `base`, that could only mean that
the promotion key was associated with a variable that had gone out of
scope; accordingly, it was safe for `rebaseForward` to simply ignore
that key. (It did so implicitly, by only ever examining the promotion
keys in `this`). But with the addition of field promotion, it is now
possible that the promotion key represents a property that was
promoted in `this`, and hence the promotion needs to be kept. This CL
adds the necessary logic to keep the promotion.
In addition, there is a subtle difference in the relationship between
the `PromotionModel` and `SsaNode` data structures for local variables
versus properties. For local variables, the promotion key is
determined solely from the variable name; then, this promotion key is
looked up in the current `FlowModel` to obtain a `PromotionModel`, and
the `PromotionModel` contains a prointer to the `SsaNode`. For
properties, the property name is looked up in the
`promotableProperties` map of the parent `SsaNode`; this points to a
`_PropertySsaNode`, which contains the promotion key, and when this
promotion key is looked up in the current `FlowModel` to obtain a
`PromotionModel`, that `PromotionModel` contains a pointer to a bogus
`SsaNode`.
For local variables, the `SsaNode` pointed to by the `PromotionModel`
is important, because if it's different between `this` and `base`,
then the variable in question received a new value between the time
the condition variable was stored and the time the condition variable
was recalled; therefore the promotion should be disregarded. However,
for properties, the `SsaNode` pointed to by the `PromotionModel` is
bogus, so if it's different between `this` and `base`, that shouldn't
block promotion. This CL adds the necessary logic to avoid the
`SsaNode` check for properties.
This situation is very confusing so I've added more detail to the
comment above `PromotionModel.SsaNode` explaining it. In a future CL I
will try to clean up the confusing situation by eliminating the bogus
`SsaNode`s pointed to by `PromotionModel`s for properties.
Fixes https://github.com/dart-lang/sdk/issues/53273.
Bug: https://github.com/dart-lang/sdk/issues/53273
Change-Id: I1d528e25de1eb2ed63d0ee1a00faa5ad5b5061ca
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321752
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The `merge` operation is equivalent to a `join` operation followed by
an `unsplit` operation, so there is no real benefit to having both
`merge` and `join` as separate methods. Removing `merge` will make it
easier to refactor the behavior of `join` in follow-up CLs.
Change-Id: I4a42cdd1cb2e795dcfeae86703fe5d3356c131ce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322140
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Remove the `Reachability.restrict` method--it hasn't been used since
2021, and was only kept around by mistake.
Remove the `Reachability.join` method--it was only used in the
circumstance where two flow control paths were being joined that had
equivalent reachability, in which case, it did nothing but return a
new reachability that was equivalent to one of its inputs. Call sites
have been updated to assert that the two control flow paths are indeed
equivalent, and then simply use the first control flow path's
reachability.
Change-Id: Ie24789ccd18723425b7db405d2122954dfae7b5b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321841
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
The class `VariableModel` is renamed to `PromotionModel` to reflect
that it tracks promotion information for both local variables and
fields. Variables and fields that map from a promotion key to an
instance of `PromotionModel` are renamed from `variableInfo` to
`promotionInfo`.
Also a few comments are re-worded to make them clearer.
There is no functional change--this is a rename-only CL.
Change-Id: Ie0e7147d290171407f31fe65af85ce96dd51694a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/321362
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Try/finally statements have an unusual property that needs to be
handled in a special way by flow analysis: within the `finally` block,
the `try` block might not have run to completion, but after the
try/finally statement, the `try` block is known to have run to
completion. Therefore, even though the code that follows the
try/finally statement is immediately preceded by the end of the
`finally` block, the flow states of those two control flow points
might not be the same.
Flow analysis accounts for this situation by analyzing the `finally`
block as though it started executing right after the beginning of the
`try` block, but with all variables that are written within the `try`
block demoted. Then, after it finishes analyzing the `finally` block,
it builds a fresh flow model by starting with the flow state after the
end of the `try` block, and then applying any promotions that were
performed within the `finally` block. This is accomplished by the
`FlowModel.attachFinally` method.
The following changes had to be made to make this work with field
promotion:
- If a given promotion key appears in the "after try" model but not
the "after finally" model, it might represent a field that was
promoted during the `try` block, so the promotion needs to be
preserved. Previously, this situation could only occur if the
promotion key represented a variable declared in the `try` block
(and therefore the variable would not be accessible after the
try/finally statement), so the promotion could be safely dropped.
- If a given promotion key appears in the "after try" model and the
"after finally" model, but not the "before finally" model, it might
represent a field that was promoted within both the `try` and
`finally` blocks, so the promotions need to be combined. Previously,
this situation could not occur, so the promotion could be safely
dropped.
- If a given promotion key is associated with the same SSA node in the
"before finally" and "after finally" models, but a different SSA
node in the "after try" model, that means that the corresponding
variable was assigned in the `try` block but not in the `finally`
block. If any properties of the variable were promoted within the
`finally` block, those promotions were applied to the SSA nodes used
by the `finally` block, and don't appear in the SSA nodes used in
the "after try" model. So those promotions need to be
transferred. This is accomplished by the new
`SsaNode._applyPropertyPromotions` method.
- If a given promotion key appears in the "after finally" model but
not the "after try" model, it might represent a field that was
promoted during the `finally` block, so the promotion needs to be
preserved. Previously, this situation could only occur if the
promotion key represented a variable declared in the `finally` block
(and therefore the variable would not be accessible after the
try/finally statement), so the promotion could be safely dropped.
Fixes https://github.com/dart-lang/sdk/issues/53225.
Bug: https://github.com/dart-lang/sdk/issues/53225
Change-Id: Ie4b635dbf838447d6964c326e1ecebfff99bed8e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/320961
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Flow analysis implements field promotion using an extension of SSA
(static single assignment) analysis.
In traditional SSA, an analysis phase assigns a collection of SSA
nodes to each local variable in the program, such that each SSA node
represents a region of control flow in which there are no assignments
to the variable, and no control flow joins that might affect the
variable's value. Each local variable read expression is therefore
associated with an SSA node, establishing the invariant that two reads
that share the same SSA node are known to produce identical results.
In flow analysis, SSA nodes are also assigned to property get
expressions, with a similar invariant: two property gets that share
the same SSA node are known to produce identical results. To ensure
soundness, flow analysis generally only considers a property get
expression to be promoted if the get has the same SSA node as a
previous type test.
There's an exception, though: if a property get is associated with an
SSA node that arose from a control flow join, it may be appropriate to
consider it promoted, if the SSA nodes that were joined are both
considered to be promoted. For example, consider the code below:
class C {
final int? _i;
C(this._i);
}
f(bool b, C c1, C c2) {
C c3;
if (b) {
c3 = c1;
if (c3._i == null) return;
} else {
c3 = c2;
if (c3._i == null) return;
} // (1)
print(c3._i + 1); // (2)
}
At (2), it makes sense to consider `c3._i` to be promoted, because
`c3._i` was type checked in both control flow paths leading up to the
join point at (1). However, since those two control flow paths contain
different assignments to `c3`, at the time that the join point (1) is
analyzed, flow analysis assigns a fresh SSA node to `c3._i`, distinct
from the two SSA nodes that were type checked.
To ensure that the promotion is preserved, a new method is introduced,
`SsaNode._join`, which creates the fresh SSA node and updates the
newly created flow model to preserve the promotion. The bulk of the
heavy lifting is done by `SsaNode._joinProperties`, which recursively
walks the `_promotableProperties` maps of the two SSA nodes being
joined, creating fresh promotions for all the properties that should
have their promotions preserved.
This required plumbing some new parameters through
`VariableModel.join` (which calls `SsaNode._join`), so that
`SsaNode._joinProperties` can find the promotion information along the
two incoming control flow paths, and can create fresh promotions for
the outgoing control flow path.
Fixes https://github.com/dart-lang/sdk/issues/53146.
Change-Id: I6e53b3363ab5d769bef1b96f0ccd380fa2ca39df
Bug: https://github.com/dart-lang/sdk/issues/53146
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/320580
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Previously, the flow control logic for patterns didn't use the
`FlowModel.split` or `FlowModel.unsplit` methods at all. This meant
that if a control flow join point occurred in pattern logic, flow
analysis would consider the split point to be whatever split point was
established by the enclosing expression or statement. In the case of
an if-case statement, it would consider the split point to be at the
beginning of the scrutinee expression.
Split points are used by flow analysis for the sole purpose of
ensuring that joins propagate type promotions the same way in dead
code as they do in live code (so that users introducing temporary
`throw` expressions or `return` statements into their code do not have
to deal with nuisance compile errors in the (now dead) code that
follows. The consequence of flow analysis considering the split point
to be at the beginning of the scrutinee expression is that if the
scrutinee expression is proven to always throw, then joins that arise
from the pattern or guard may not behave consistently with how they
would have behaved otherwise. For example:
int getInt(Object o) => ...;
void consumeInt(int i) { ... }
test(int? i) {
if (
// (1)
getInt('foo')
case
// (2)
int()
// (3)
when i == null) {
} else {
// (4)
consumeInt(i);
}
}
In the above code, there is a join point at (4), joining control flows
from (a) the situation where the pattern `int()` failed to match, and
(b) the situation where `i == null` evaluated to `false` (and hence
`i` is promoted to non-nullable `int`). Since the return type of
`getInt` is `int`, it's impossible for the pattern `int()` to fail, so
at the join point, control flow path (a) is considered
unreacable. Therefore the promotion from control flow path (b) is
kept, and so the call to `consumeInt` is valid.
In order to decide whether to preserve promotions from one of the
control flow paths leading up to a join, flow analysis only considers
reachability relative to the corresponding split point. Prior to this
change, the split point in question occurred at (1), so if the
expression `getInt('foo')` had been replaced with `getInt(throw
UnimplementedError())`, flow analysis would have considered both
control flow paths (a) and (b) to be unreachable relative to the split
point, so it would not have preserved the promotion from (b), and
there would have been a compile time error in the (now dead) call to
`consumeInt`.
This change moves the split point from (1) to (2), so that changing
`getInt('foo')` to `getInt(throw UnimplementedError())` no longer
causes any change in type promotion behavior.
The implementation of this change is to add calls to `FlowModel.split`
and `FlowModel.unsplit` around all top-level patterns. At first glance
this might appear to affect the behavior of all patterns, but actually
the only user-visible effect is on patterns in if-case statements,
because:
- In switch statements and switch expressions, there is already a
split point before each case.
- In irrefutable patterns, there is no user-visible effect, because
irrefutable patterns cannot fail to match, and therefore don't do
any control flow joins.
This change allows the split points for patterns to be determined by a
simple syntactic rule, which will facilitate some refactoring of split
points that I am currently working on.
Change-Id: I55573ba5c28b2f2e6bba8731f9e3b02613b6beb2
Bug: https://github.com/dart-lang/sdk/issues/53167
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319381
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, flow analysis used a hack to make it easy to generate "why
not promoted" messages when the user tried to promote a non-promotable
field: it treated all field accesses as stable for the purpose of
assigning SSA nodes, but avoided promoting non-promotable fields by
setting the `_Reference.isPromotable` flag to `false`. So, for
instance, in the following code, both subexpressions `c.i` got
assigned the same SSA node, even though there's no guarantee that
`C.x` will return the same value each time it's invoked.
class C {
int? get i => ...;
}
f(C c) {
if (c.i != null) {
var i = c.i; // Inferred type `int?`
}
}
This mostly worked, since the SSA node assigned by flow analysis is
only used for promotion, and promotion is disabled for non-promotable
fields. However, it broke when the field in question was used as the
target of a cascade, because fields within cascades always had their
`_Reference.isPromotable` flag set to `true` regardless of whether the
corresponding cascade target is promotable. For example:
class C {
D? get d => ...;
}
class D {
final E? _e;
...
}
class E {
m() { ... }
}
f(C c) {
(c.d)
.._e!.m() // OK; promotes _e
.._e.m(); // OK; _e is promoted now
(c.d)
.._e.m(); // OOPS, _e is still promoted; it shouldn't be
}
See
`tests/language/inference_update_2/cascaded_field_promotion_unstable_target_test.dart`
for a more detailed example.
This CL removes the hack; now, when a non-promotable property is
accessed more than once, flow analysis assignes a different SSA node
for each access. As a result, the `_Reference.isPromotable` is not
needed, because non-promotable fields simply never have the chance to
be promoted (since every field access gets a separate SSA node, so
type checking one field access has no effect on others).
To preserve the ability to generate "why not promoted" messages, the
`_PropertySsaNode` class now contains a `previousSsaNode` pointer,
which links together the separate SSA nodes allocated for
non-promotable properties, so that they form a linked list. The "why
not promoted" logic traverses this list to figure out which promotions
*would* have occurred if the property had been promotable.
In order to make it efficient to create this linked list, the
`SsaNode` class also had to acquire a `_nonPromotableProperties` map,
which records the SSA node that was allocated the last time each
property was accessed.
Fixes https://github.com/dart-lang/sdk/issues/52728.
Change-Id: I16a7b27f77c309bdccce86195a53398e32e8f75d
Bug: https://github.com/dart-lang/sdk/issues/52728
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/318745
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, in the "mini-ast" pseudo-language for shared flow analysis
and type analysis unit testing, there were two separate functions to
create a rest pattern (`...`):
- `listPatternRestElement` for creating a rest pattern for use in a
list pattern
- `mapPatternRestElement` for creating a rest pattern for use in a map
pattern
The latter is not allowed in Dart, but it's supported to allow for
better error recovery.
But having two functions wasn't necessary--the two functions did
exactly the same thing.
This CL simplifies things so that there is just a single function,
`restPattern`. It also renames the underlying representation class
from `RestPatternElement` to `RestPattern`, to match the nomenclature
in the patterns spec document.
Change-Id: Iecfe8c86f49161e0657cdab44f000f47c0e3c212
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315520
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
When using the "mini-ast" pseudo-language to write unit tests for the
flow analysis and type analysis logic in the `_fe_analyzer_shared`
package, it is no longer necessary to use `.expr` to turn a `Var` into
an `Expression`; this now happens automatically. The way this works
under the hood is that the `Var` and `Expression` classes implement
the `ProtoExpression` interface; constructs that expect expressions
are declared with input parameters of type `ProtoExpression`, and they
automatically convert to `Expression`s when necessary.
Change-Id: I85c493145c3fc41a5296c1807cd63fe1401672db
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315247
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
In several flow analysis tests we need to model an expression with two
subexpressions, where the type of the overall expression is the same
as the type of the second subexpression.
Previously we did this using `ProtoStatement.thenExpr`, which created
a magic compound expression consisting of an expression (or statement)
followed by a second expression. This was too general, since it
allowed modelling code that would not be possible in Dart.
This CL replaces uses of `ProtoStatement.thenExpr` with a function
`second`, which models a Dart function defined this way:
T second(dynamic x, T y) => y;
This has two advantages:
- The tests are now modelling code that could actually exist in the
real world, rather than modelling pseudo-expressions that are not
actually possible to write in Dart.
- It prepares for a follow-up CL in which I'll be unifying the
representation of patterns and expressions in the flow analysis
tests; eliminating `ProtoStatement.thenExpr` will prevent a conflict
between it and `PossiblyGuardedPattern.thenExpr`.
With this change, two tests had to be deleted, because the conditions
they were testing could no longer be expressed:
- `for_conditionBegin() handles not-yet-seen variables`
- `whileStatement_conditionBegin() handles not-yet-seen variables`
This is ok, because these tests were designed to verify correct
behavior in the circumstance where the client forgets to declare a
variable. This circumstance no longer arises, because the
`_FlowAnalysisImpl` constructor detects undeclared variables and
generates synthetic declarations for them.
Change-Id: I19275ba0b7fa143ce7051bf14d0d4c6f57a4ab8e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315244
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
The following functions, which are used to create constructs in the
"mini-ast" pseudo-language to write unit tests for shared flow
analysis and type analysis logic, now have a return type of
`Expression` rather than `Statement`:
- checkAssigned
- checkUnassigned
- getSsaNodes
- implicitThis_whyNotPromoted an expression
This allows them to be used either where an expression is exprected or
where a statement is expected, which should give us the ability to
write some tests that are not possible (or very difficult) to write
today.
Change-Id: Ie50e82ad05dc88182b59c99c11ffaac41515ebb9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315460
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
When using the "mini-ast" pseudo-language to write unit tests for the
flow analysis and type analysis logic in the `_fe_analyzer_shared`
package, it is no longer necessary to use `.switchCase` to turn a
`Pattern` (or a `GuardedPattern`) into a `SwitchHead`; this now
happens automatically. The way this works under the hood is that the
`PossiblyGuardedPattern`, and `SwitchHead` classes implement the
`ProtoSwitchHead` interface; constructs that expect switch heads are
declared with input parameters of type `ProtoSwitchHead`, and they
automatically convert to switch heads when necessary.
Change-Id: Ic16264fc52ffff08bf0e0cc72db064a6da63eda5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315180
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
When using the "mini-ast" pseudo-language to write unit tests for the
flow analysis and type analysis logic in the `_fe_analyzer_shared`
package, it is no longer necessary to use `.asCollectionElement` to
turn an expression into a collection element; this now happens
automatically. The way this works under the hood is that both the
`CollectionElement` and `Expression` classes mix in the
`ProtoCollectionElement` mixin; constructs that expect collection
elements are declared with input parameters of type
`ProtoCollectionElement`, and they automatically convert expressions
to collection elements when necessary.
Also, instead of using `.inContextElementType` to establish the
appropriate context when testing a collection element, the tests not
simply create a `listLiteral` of the appropriate type, containing the
appropriate collection elements. This makes the unit tests much more
similar to the way actual Dart code is written in the wild, and makes
the test infrastructure more closely mirror the way types are analyzed
by the analyzer and CFE.
Change-Id: Ib628ff12caa84254df069308ae3a25061377db29
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/314141
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
When using the "mini-ast" pseudo-language to write unit tests for the
flow analysis and type analysis logic in the `_fe_analyzer_shared`, it
is no longer necessary to use `.stmt` to turn an expression into an
expression statement; this now happens automatically. The way this
works under the hood is that both the `Statement` and `Expression`
classes mix in the `ProtoStatement` mixin; constructs that expect
statements are declared with input parameters of type
`ProtoStatement`, and they automatically convert expressions to
statements when necessary.
Also, the functions `checkNotPromoted`, `checkPromoted`,
`checkReachable`, `localFunction` now have a return type of
`Expression` rather than `Statement`. This allows them to be used
either where an expression is exprected or where a statement is
expected, which should give us the ability to write some tests that
are not possible (or very difficult) to write today.
Change-Id: I9f7ad5b15bcf8ccfccafc6985e0163b550c5ad1c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313680
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The assertion in the `_FlowAnalysisImpl` constructor was unnecessary
because it was checking that all variables that are reported to
`AssignedVariables` as read or written must also be reported as
declared. This is already checked by assertions in
`AssignedVariables.finish`, which is called by the `_FlowAnalysisImpl`
constructor.
I've added tests to `assigned_variables_test.dart` to confirm that
these assertions work, and I've also cleaned up the assertions a bit,
eliminating some redundancy and ensuring that in the event of a
failure the assertion failure message will be comprehensible.
Change-Id: Ife827c91d944707f093f4cb8421385f5355d11fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313140
Auto-Submit: Paul Berry <paulberry@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
This change updates the flow analysis support for field promotion
(which is not yet switched on by default) so that it supports field
accesses inside cascade expressions. The key moving parts are:
- The type hierarchy `PropertyTarget` (which is used by the client to
tell flow analysis whether the target of a property access is
`this`, `super`, or an ordinary expression) now has a new class,
`CascadePropertyTarget`, to represent the situation where the target
of the property access is an implicit reference to the target of the
innermost enclosing cascade expression.
- Flow analysis has two new methods on its API:
`cascadeExpression_afterTarget` and `cascadeExpression_end`, so that
the client can inform flow analysis when a cascade expression is
being analyzed.
- Flow analysis uses its `_makeTemporaryReference` method to track the
implicit temporary variable that stores the target of cascade
expressions. (This method was developed as part of flow analysis
support for patterns, where it creates the data structures necessary
to track the implicit variables that are created as part of pattern
desugaring).
- The "mini-AST" pseudo-language used by flow analysis unit tests now
has a way to represent cascade expressions and method invocations.
- In addition to unit tests for `_fe_analyzer_shared`, `analyzer`, and
`front_end`, there are new language tests in
`tests/language/inference_update_2` to test cascaded field
promotions in end-to-end fashion.
Bug: https://github.com/dart-lang/language/issues/2020
Change-Id: I21353bbc884ed599cb1739cecfb68ad1d975d18b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/309220
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
If a user promotes a field and then subsequently changes the variable
used to access it, the promotion must be discarded. For example, in
this code:
class C {
int? _i;
}
test(C c1, C c2) {
C c = c1; // (1)
if (c._i != null) { // (2)
print(c._i + 1); // (3)
c = c2; // (4)
print(c._i + 1); // (5)
}
}
The test at (2) promotes `c._i` to non-null, so (3) is ok. But since
`c` is reassigned at (4), (5) should be a compile-time error.
Previously, flow analysis used one promotion key to track `c` and one
promotion key to track `c._i`. The `PromotionKeyStore` associated each
promotion key with a map containing the promotion keys of all of its
properties. So, for example, if the promotion key for `c` was 10 and
the promotion key for `c._i` was 11, the `PromotionKeyStore` would
associate promotion key 10 with the map `{'_i': 11}`, so that each
time `c._i` was accessed, promotion key 11 would be found. In order to
detect the compile-time error at (5), it had to keep track of the fact
that keys 10 and 11 were related, so that the assignment to `c` at (4)
could invalidate the promotion of `c._i`. It accomplished this by
linking together all the related promotion keys in a circularly linked
list, which it would walk at the time of any variable assignment.
This worked, but it required a lot of complex bookkeeping. Also, it
posed problems for integrating field promotion with cascades, for
example, in the following code:
class B {
void f([_]) { ... }
}
class C {
B? _b;
}
test(C c1, C c2) {
C c = c1; // (6)
if (c._b != null) { // (7)
c.._b.f( // (8)
[
c = c2, // (9)
c._b.f(), // (10)
])
.._b.f(); // (11)
}
}
The cascaded access `.._b.f` at (8) should be ok, since `c._b` has
been promoted. But since there is an assignment to `c` at (9), the
promotion should not carry over to (10), and an error should be
reported. However, no error should be reported at (11) because the
cascaded access to `.._b.f` at that location is using the old value of
`c` that was captured at the beginning of the cascade, prior to the
assignment. There's no way to achieve this by invalidation alone,
since the code locations at which the promotion is valid ((8) and
(11), but not (10)) aren't even contiguous.
The solution to the problem is to store property promotion keys in
`SsaNode`s used by flow analysis, rather than in the
`PromotionKeyStore`. Since a fresh `SsaNode` is allocated each time a
variable is assigned, this automatically invalidates any previous
property promotions without the need for any extra bookkeeping. So, in
the first example, at (1), an `SsaNode` is allocated and associated
with the promotion key for `c`. At (2), a promotion key is created for
`c._i` and stored in `c`'s `SsaNode`, and the flow model is updated to
indicate that that key has been promoted to non-null. At (3), that
promotion key is reacalled from the `c`'s `SsaNode`, so the promotion
is still in effect. At (4), a fresh `SsaNode` is associated with
`c`. Since that `SsaNode` doesn't contain any promotion keys yet, at
(5), the access to `c._i` causes a fresh promotion key to be
allocated, with no associated promotions. So the invalidation happens
automatically due to the fact that a new `SsaNode` was created.
Flow analysis doesn't yet support cascades, but here's how the second
example is intended to work: as before, at (6), an `SsaNode` is
allocated and associated with the promotion key for `c`. At (7), a
promotion key is created for `c._b` and stored in `c`'s `SsaNode`, and
the flow model is updated to indicate that `c._b` has been promoted to
non-null. At (8), the `SsaNode` for `c` is captured and saved for
later use. At (9), a fresh `SsaNode` is created and associated with
`c`. That fresh `SsaNode` is consulted at (10), so `c._b` is not
promoted at this point. However, at (11), the previously stored
`SsaNode` is used, so the promotion of `._b` still works.
Change-Id: I64519fbcb2368a37aa18adf35cee0ffd290db9b9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/307140
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Previously, flow analysis used three classes to keep track of
information about expressions that have been visited:
- ExpressionInfo, to keep track of information about expressions whose
truth or falsity has an effect on flow analysis (such as `== null`
checks, `is` checks, and combinations thereof those using `&&` and
`||`), as well as identifying the literal expression `Null`.
- ReferenceWithType, to keep track of information about expressions
that represent something that can be promoted (references to
variables and fields), as well as the static type of the expression.
- EqualityInfo, which wrapped ExpressionInfo and ReferenceWithType,
and also redundantly stored the static type of the expression.
These have now been combined into a single class hierarchy with a base
class called `ExpressionInfo`. This makes the code easier to reason
about, because it is no longer necessary to think about which of the
three above classes is needed in a given situation. Also, it helps
prepare for a follow-up CL in which I plan to refactor how flow
analysis gathers this information; the refactor will be easier with
just a single class hierarchy of information to be gathered.
This required a modest expansion of the API to flow analysis to
include more static types, since previously, static types weren't
needed by the ExpressionInfo class.
Change-Id: Id3de8b19049f8d920ebe85ab58c624ae3e55f226
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/304211
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Because of the permissive type of `Map.operator []`, I forgot to
update this test when I changed the key type of
`FlowModel.variableInfo` from `Variable?` to `int` (way back in
https://dart-review.googlesource.com/c/sdk/+/250120). The test passed
anyway because it was attempting to check that a certain key was not
present, so of course the test expectation was satisfied by passing in
a key that couldn't possibly exist in the map.
Fortunately the functionality in question had not regressed.
Change-Id: I9a8d6e75e4ca9419b7dae02d3ececa7b9a6ac0dd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/305062
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
When promoting fields, in order to avoid unsoundness, we need to
distinguish between field accesses performed through `super` and field
accesses performed through `this`. Otherwise, a user could do
something like this:
class B {
final int? _i;
B(this.i);
}
class C extends B {
final int? _i;
C(this._i, int? superI) : super(superI);
int f() {
if (super._i != null) {
return this._i; // UNSOUND: `this._i` could be `null`
}
}
}
To avoid this problem, flow analysis now uses separate promotion keys
for `super` and `this`, so that promoting a variable through `this`
leaves it unpromoted when accessed via `super`, and vice versa.
Note that in principle the implementations could inspect the enclosing
class, and only distinguish `this.` and `super.` accesses in the case
where it contains a declaration matching the field name. But doing so
would carry a performance and implementation complexity cost, and
would confer very little real-world benefit (since in practice users
don't mix `this.` and `super.` accesses and expect them to refer to
the same field).
Fixes#50138.
Bug: https://github.com/dart-lang/sdk/issues/50138
Change-Id: Ia0fd79b5ed7649d23a28efcbffb59b4c9ad63f70
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/304364
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Verify that the presence of an error type doesn't accidentally trigger
"unnecessary cast pattern" or "unnecessary wildcard pattern" warnings
(this verifies that `promoteForPattern` returns the proper value when
an error type is encountered).
Verify that the presence of an error type doesn't alter reachability
conclusions that were established by previous subpatterns (this
verifies that `promoteForPattern` uses the proper join to update
`_unmatched` when an error type is encountered).
Change-Id: I351c71574dff31602c40b257c28681b81fad2867
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303260
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Currently every FlowModel creates a new empty map that's just supposed
to be empty. When compiling `compile.dart` (from the CFE) this creates
more than 200,000 maps for seemingly no reason.
This CL removes it, thus saving the creation of (...instrumenting the
platform...) 229,472 maps when compiling `compile.dart` (from the CFE).
Thinking it was done to avoid some polymorphism I have gone over the
created flowgraphs for the file in both JIT and AOT and found only
improvements.
AOT compiled `compile.dart` then compiling itself improves by ~3.5%:
```
Difference at 95.0% confidence
-0.1365 +/- 0.0576111
-3.55191% +/- 1.49912%
(Student's t, pooled s = 0.090011)
```
Change-Id: Ifdbb57e9aa3c23b2af512a2104aaf6caf72831ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/301061
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
This fixes a minor bug in flow analysis which was preventing it from
recognizing when a switch statement was trivially exhaustive, meaning
one of its reachable cases was guaranteed to always match.
This mostly addresses
https://github.com/dart-lang/language/issues/2980, but flow analysis
still fails to recognize that:
- A list pattern containing a just a single rest pattern always
matches (unless the rest pattern has a subpattern that may fail to
match).
- A null check pattern always matches if its subpattern always matches
and the matched value type is non-nullable.
- The relational pattern `!= null` always matches if its subpattern
always matches and the matched value type is non-nullable.
Fortunately, these drawbacks are small and don't lead to unsoundness.
I'll try to address them in follow up CLs.
Bug: https://github.com/dart-lang/language/issues/2980
Change-Id: Ie9f8564cde66a5a2c41114033ca3ff0e1a0f139a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/293860
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Judging by several test cases that have shown up in co19 tests and
informal discussions, it appears to be a common expectation that a
pattern like `int? x?` or `int? x!` should promote `x` to
non-nullable. Previous to this change, this didn't work in general.
Consider:
Object? o = ...;
switch (o) {
case int? x?:
print(x.isEven); // (1)
}
At (1), the null-check happens *before* the required type check of the
variable pattern; therefore it promotes the matched value type to
`Object`. This is not a subtype of the required type of the variable
pattern (which is `int?`), therefore, previous to this change, `x` was
not promoted.
With this change, since the matched value type of `Object` is
non-nullable, the required type of the variable pattern is
re-interpreted as its non-nullable counterpart, `int`.
In a fully null-safe program, this is sound, because if the matched
value type is non-nullable, that guarantees that the matched value is
not `null`, and therefore it is equivalent to type check against the
non-nullable counterpart of the required type.
In a program that is not fully null-safe, the matched value might have
originated in a non-null-safe library (and thus might be `null` in
violation of its static type). So, strictly speaking, it is not sound
to re-interpret the required type of the pattern as its non-nullable
counterpart. However, the only way this unsoundness can manifest is
for the matched value to be promoted to non-nullable when it is in
fact `null`, and that is precisely the sort of unsoundness thta we
permit in mixed-mode programs. So this change won't result in
unsoundness escalation.
Bug: https://github.com/dart-lang/sdk/issues/51644
Change-Id: I9479e3c29e12f2a62a9e165b32c3480d7e299c29
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/287040
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This unit test makes sure that parenthesizing a variable pattern
doesn't destroy its ability to track the flow analysis consequences of
a boolean expression.
No code changes were are required, since the implementation passes the
test. But prior to
https://dart-review.googlesource.com/c/sdk/+/282482 this test would
have failed, so it seems reasonable to include it as a unit test to
prevent regressions.
Change-Id: If5595a346757e608bc133846062ec37f47bc3e1e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/284485
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Previously, if multiple switch cases shared a body, and at least one
of those cases promoted a match variable using a `when` clause, the
promotion would not be carried over to the merged variable seen in the
shared case body. This was happening because promotions performed in
the `when` clause were applied to the promotion key associated with
the client's representation of the variable, whereas flow analysis was
computing the state of the merged variable based on the promotion keys
used internally while visiting the pattern (which don't include
promotions from the `when` clause).
With this change, flow analysis now computes the merged variable state
based on the promotion keys associated with the client's
representation of the variable components, so promotions from `when`
clauses take effect.
In the process, I've moved a lot of the tracking of these promotion
keys from the `TypeAnalyzer` class to the `_FlowAnalysisImpl` class.
This avoids the need to expose more flow analysis internals through
its public API.
Fixes#51399.
Bug: https://github.com/dart-lang/sdk/issues/51399
Change-Id: I8f1bb6e49ceb8441bb743c63af61c119df9041f2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/283441
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
For each subpattern of a record pattern, we define its "demonstrated
type" to be the type that the matched value has been promoted to if
the subpattern match succeeds. At the conclusion of visiting a record
pattern, we promote the whole record pattern's matched value to a
record type formed by combining together the demonstrated types of the
subpatterns. So, for example, the pattern `(int _, String _)`
promotes the matched value to `(int, String)`.
This change contains a lot of specific tests, to make sure the
demonstrated type of each kind of pattern behaves as expected; but the
actual machinery is general, since it just takes advantage of the type
promotion performed by the subpatterns.
Bug: https://github.com/dart-lang/sdk/issues/50419
Change-Id: Ie4da81964b5ade657c23a598ee18982b793fb4ac
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/282806
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Paul Berry