https://dart-review.googlesource.com/c/sdk/+/498840 added support for
promotion of properties (private, final instance variables with a name
which isn't used much for other purposes) in the context of anonymous
methods.
This CL reduces the set of situations where this feature is enabled such
that only `this` will allow property promotions to be carried in (such
that `this._x` is promoted in `v.=> this._x` when `v` is such that
`v._x` has been promoted before the anonymous method occurs). It also
generalizes the mechanism such that property promotions are carried out
(so we can do `if (v.=> _x is int) v._x.isEven;`).
Tests has been adjusted accordingly.
Change-Id: Ibe70713d3d9c89a6d95f9c3dd28df8f147cb518d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/502660
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
This CL adds support for promotion of certain private final instance
variables along with anonymous methods. The promotions do not differ
from the ones which are already available in Dart without anonymous
methods, but it requires some generalizations to handle the changing
value of `this` which is made possible by anonymous methods.
Change-Id: I720a5fa6d29a8a7d19bb2e167dc135f97492b525
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/498840
Reviewed-by: Paul Berry <paulberry@google.com>
This reverts commit 3eb697c0af.
Reason for revert: Internal Google3 breakages
Original change's description:
> [flow analysis] Fix unsound type promotion in inner async/generator functions.
>
> An `await` expression or `yield` statement suspends the current
> function and allows other code in the same isolate to execute. In the
> case of nested functions, an `await` or `yield` in the inner function
> can allow the outer function to continue executing. That means that if
> the inner function promotes a local variable belonging to the outer
> function, then it isn't sound to carry that promotion past an `await`
> or `yield`.
>
> This change fixes the unsoundness by adding a flow analysis method
> `suspension`, which the shared type analysis logic uses to tell flow
> analysis that an `await` or `yield` has been found. The `suspension`
> method un-does the promotions of any variables that might be written
> to while the inner function is suspended.
>
> Fixes https://github.com/dart-lang/sdk/issues/62889.
>
> Change-Id: I77eaf997159819a7c50f44b67174d2aa6a6a6964
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/499382
> Reviewed-by: Johnni Winther <johnniwinther@google.com>
> Commit-Queue: Paul Berry <paulberry@google.com>
> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
> Reviewed-by: Bob Nystrom <rnystrom@google.com>
Change-Id: I187ba9a347394946ecc8749d35dc7914d271b90a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/500540
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Bob Nystrom <rnystrom@google.com>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Paul Berry <paulberry@google.com>
An `await` expression or `yield` statement suspends the current
function and allows other code in the same isolate to execute. In the
case of nested functions, an `await` or `yield` in the inner function
can allow the outer function to continue executing. That means that if
the inner function promotes a local variable belonging to the outer
function, then it isn't sound to carry that promotion past an `await`
or `yield`.
This change fixes the unsoundness by adding a flow analysis method
`suspension`, which the shared type analysis logic uses to tell flow
analysis that an `await` or `yield` has been found. The `suspension`
method un-does the promotions of any variables that might be written
to while the inner function is suspended.
Fixes https://github.com/dart-lang/sdk/issues/62889.
Change-Id: I77eaf997159819a7c50f44b67174d2aa6a6a6964
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/499382
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Bob Nystrom <rnystrom@google.com>
Prior to the introduction of the "anonymous methods" experiment, a
`return` statement and a `throw` expression behaved identically from
the point of view of flow analysis, since both had the effect of
causing control flow to jump outside the function that flow analysis
is analyzing*. So they were both implemented using a single flow
analysis method called `handleExit`.
(*Technically a `return` from an inner function could lead to a point
in an enclosing function, and a `throw` could lead to a `catch`, but
flow analysis handles both of these possibilities using a conservative
approximation (see the `FlowModel.conservativeJoin` method), rather
than modeling them as direct jumps.
But a `return` statement inside a block-bodied anonymous method is
known to jump directly to the code that follows the anonymous method
invocation, so `handleExit` is not the correct way to model it.
Prior to this CL, this was handled in the analyzer's resolver (the
corresponding CFE logic hasn't been written yet) by treating anonymous
methods as a kind of loop construct. When visiting a return statement,
the resolver would find the innermost enclosing function expression,
local function, or block-bodied anonymous method; if it was a
block-bodied anonymous method, then it would achieve the desired
effect by calling `FlowAnalysis.handleBreak` rather than
`FlowAnalysis.handleExit`. This was an abstraction leak, because in
effect it put some of the business logic of flow analysis in its
client (namely, the knowledge that return statements in block-bodied
anonymous methods have a different flow analysis behavior than return
statements elsewhere).
This CL moves this business logic into flow analysis through the
addition of a `FlowAnalysis.handleReturn` method.
Flow analysis keeps track of whether the current point in the code
being analyzed is inside a block-bodied anonymous method using the new
field `FlowAnalysis._anonymousBlockContext`, which points to either
`null` or an instance of a new type, `_AnonymousBlockContext`. This
field is updated in proper nesting fashion by the methods:
- `anonymousBlockBody_begin`
- `anonymousBlockBody_end`
- `_functionExpression_begin`
- `_functionExpression_end`
Finally, some aspects of
https://dart-review.googlesource.com/c/sdk/+/482786 that are no longer
necessary are rolled back:
- A node no longer needs to be passed to `anonymousBlockBody_begin`.
- The mapping from nodes to branch targets is changed back to a
mapping from statements to branch targets, since it no longer needs
to accept an anonymous method invocation as a key.
Change-Id: I8b0f35cab016fc5bd609cfa1581ecaa36a6a6964
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/485020
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Removes the `_flowAnalysisInfoMap` field from the `Harness` class,
which serves as the test harness for the mini_ast used in flow
analysis testing, along with the methods `getFlowAnalysisInfo` and
`storeFlowAnalysisInfo` that did map lookups. Calls to
`getFlowAnalysisInfo` are replaced with logic that pulls the flow
analysis expression info directly from the expression analysis result,
and calls to `storeFlowAnalysisInfo` are dropped (since they are no
longer needed).
Also, the assertion is dropped from `dispatchExpression` that used to
verify that the information stored in the map matched the information
stored in the expression analysis result.
Change-Id: I6a6a69643e0345a1f5ef84f09340119b56bfed12
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482580
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Migrates the mini_ast used for flow analysis testing so that instead
of associating expressions with expression info objects using
`FlowAnalysis.getExpressionInfo` and
`FlowAnalysis.storeExpressionInfo`, it does so using its own private
map, which it accesses using the new methods `getFlowAnalysisInfo` and
`storeFlowAnalysisInfo`.
This paves the way for two independent arcs of work:
- Removing the `getExpressionInfo` and `storeExpressionInfo` methods
from `FlowAnalysis`.
- Simplifying mini_ast so that it tracks expression info objects using
`ExpressionTypeAnalysisResult.flowAnalysisInfo` rather than its own
private map.
A similar effort is underway for the analyzer and front_end, which
should carry the following benefits:
- It will make type analysis more performant by avoiding map lookups
- It will decrease the risk of subtle bugs when one expression is
changed into another.
Change-Id: I6a6a6964f0c41c62be21efc0fa16b22b10e85b34
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482561
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Previously, the mini_ast used for flow analysis testing simulated
prefix increment/decrement operations using a `Write` node with a
null right-hand side. This commit introduces a dedicated `PreIncDec`
node to represent these operations more accurately.
The test "write() permits expression to be null" is removed in favor
of two new tests:
- "preIncDec() stores expressionInfo in the write"
- "preIncDec() demotes to the written type"
These new tests parallel the corresponding tests that already exist
for postIncDec.
Change-Id: I6a6a69646c63ca1c605272d4170eae3729ce90e1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482560
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Adds a language test and a flow analysis unit test to cover a flow
analysis behavior of post-increment and post-decrement operators that
wasn't previously covered.
The tests verify that the expressions `x++` and `x--` demote `x` in
the same way that `x = x + 1` and `x = x - 1` would. This demotion is
only user-visible if the type of `x` is a user-defined type.
In the process of writing these tests, I noticed that the "mini-AST"
implementation of post-increment (which is used solely for flow
analysis unit testing) was not correct; it presumed that the type read
from the target, the type written to it, and the type of the whole
expression were all the same. This is not correct; the type written to
the target is determined by the return type of the `+` operator. I've
fixed this as part of this CL so that the unit test properly exercises
flow analysis.
I will follow this up with some refactoring of how flow analysis
handles post increment/decrement operations. Landing the test first
allows us to be confident that the refactor won't change the tested
behavior.
Change-Id: I6a6a6964417b48db0c1681c06d7418bd79e96357
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482342
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Removes the method `FlowAnalysis.forwardExpression`, whose job was to
handle expressions that were rewritten during resolution, transferring
`ExpressionInfo` objects that were associated with the old expression
to the new rewritten expression.
Calls to `FlowAnalysis.forwardExpression` are replaced by an
equivalent construct: a call to `FlowAnalysis.getExpressionInfo` and a
call to `FlowAnalysis.setExpressionInfo`.
This change reduces the number of flow analysis methods that need to
interact with the map that associates expressions with
`ExpressionInfo` objects, paving the way for eventually removing that
map entirely.
Change-Id: I6a6a696486ea5bd30a6b2945f827a320fce1588b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/480720
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Changes the signature of `FlowAnalysis.whyNotPromoted` so that the
caller is responsible for looking up the expression info of the
matched value, and passing it in to flow analysis.
Change-Id: I6a6a69647bd1006c4ed4776a20bf36cd4c923498
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/480620
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Changes the signatures of the following flow analysis methods:
- `assert_afterCondition`
- `conditional_elseBegin`
- `conditional_end`
- `conditional_thenBegin`
- `doStatement_end`
- `ifStatement_thenBegin`
- `logicalBinaryOp_end`
- `logicalBinaryOp_rightBegin`
- `whileStatement_bodyBegin`
so that the caller is responsible for looking up the expression info
of the matched value, and passing it in to flow analysis.
Change-Id: I6a6a6964c2f975a9baf16bca5b92693af5d9fb41
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/475126
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The return types of the following methods are changed to
`ExpressionTypeAnalysisResult`:
- `NullShortingMixin.finishNullShorting` (previously returned
`SharedTypeView`).
- `NullShortingMixin.handleNullShortingStep` (previously returned
`void`).
- `TypeAnalysisNullShortingInterface.finishNullShorting` (previously
returned `SharedTypeView`).
- `TypeAnalyzer.analyzeExpression` (previously returned
`SharedTypeView`).
With one exception, these methods previously returned a
`SharedTypeView` representing the expression's static type. (The
exception was `NullShortingMixin.handleNullShortingStep`, which
previously returned `void`).
This paves the way for allowing these methods to return additional
information beyond the static type of the expression, such as flow
analysis results and tree rewrite information. (This information is
currently carried around in auxiliary data structures, which leads to
bookkeeping headaches and increases the risk of bugs.)
Change-Id: I6a6a6964572a5fd7fac5496cdbfbc82a10272bba
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/471323
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Removes the type argument `Type` from the `FlowAnalysis` class and
related classes.
There is no change in functionality, since all these type arguments
were always being instantiated with the same type argument
(`SharedTypeView`).
Change-Id: I6a6a6964698494c8df721ae422080032aaaf7dcf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/469920
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Changes the test types used in the flow analysis `joinPromotionChains`
tests from type `Type` to type `SharedTypeView`. There is no change in
functionality.
This paves the way for a follow-up CL that will change all the flow
analysis logic to use the `SharedTypeView` extension type.
Change-Id: I6a6a6964eadaa414b60537125427b5205b49772f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/469900
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The condition variable feature is a feature of flow analysis in which
writes to local variables cause flow analysis state to be saved, and
reads of local variables cause flow analysis state to be partially
restored. This is what allows type promotion in examples like the
following:
int? x = ...;
var xIsNonNull = x != null; // The following state is now saved: if
// `xIsNonNull` is `true`, `x` is known
// to be non-null
...Other statements...
if (xIsNonNull) { // The state is now restored
print(x.isEven); // Therefore this is ok, because `x` is
// known to be non-null.
}
See https://github.com/dart-lang/language/issues/1274, the original
feature request for this feature.
This CL makes the following changes:
- It adds documentation of how the feature works.
- It renames `SsaNode.expressionInfo` to
`SsaNode.conditionVariableState` and `_Reference.addPreviousInfo` to
`_Reference.restoreConditionVariableState`, so that the connection
to the "condition variable" feature is clearer.
- It removes an unnecessary call to `_Reference.addPreviousInfo` from
`_FlowAnalysisImpl._handleEqualityCheckPattern`, and documents why
it wasn't necessary.
- It changes the signature of the `SsaNode` constructor so that (a) at
call sites that don't need to save flow analysis state, there's no
need to pass in `null`, and (b) at call sites that do need to save
flow analysis state, it's clear that the state that's being saved is
`conditionVariableState`.
Change-Id: I6a6a6964f953b4e853efa95a32b1fbaa5f6c09ca
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/446600
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Flow analysis didn't handle invalid type so non-null promotion would occur on declaration and initialization on erroneous code, leading to warnings about null-aware that is likely valid. For instance
f(Unresolved o) { // Error: Unresolved is unresolved
int? i = o.property;
i?.isEven; // Warning about unnecessary null-aware access
if (i != null) { // Warning about unnecessary null comparison
i.isEven;
}
}
To handle this fully we need to track invalid nullability (if that is even feasible) but for now we change the default to avoid non-null promotion in such cases.
This *does* change the kind cascading errors/warnings that we produce. For instance
f(Unresolved o) { // Error: Unresolved is unresolved
int? i = o.nonNullProperty;
i.isEven; // Error for access on int?
}
but since it probably more likely for code to *not* depend on non-null promotion, this should be less noise for the user.
Change-Id: Ia2bc3505a43b52e5151b93a7fee24e95246b4bbc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443320
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
In https://dart-review.googlesource.com/c/sdk/+/443540, the
representation for promotion chains in flow analysis was changed so
that an empty promotion chain is now represented by an empty list
rather than `null`, but to reduce the risk of mistakes, only minimal
changes were made to flow analysis unit tests.
This CL updates the unit tests to follow the same convention.
This change also fixes a previously unnoticed bug in the flow analysis
unit tests. Previously, there were several test cases that passed a
`null` value to `_matchVariableModel`'s `chain` parameter, with the
intention of checking that the promotion chain was empty. However,
`_matchVariableModel` converts a `null` value of `chain` to `anything`
(to handle test cases where the promotion chain isn't of interest), so
as a result, these test cases accidentally failed to check that the
promotion chain was empty. These test cases have been changed to pass
`isEmpty`, so these unit tests are actually stronger now.
Change-Id: Ibf19d1ff64075d9f37fa85f85e11df417c0a68e1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443541
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Previously, flow analysis represented a promotion chain as
`List<Type>?`, with `null` representing an empty promotion chain. This
was an unnecessary optimization, and it was a source of confusion when
comparing the implementation of flow analysis to the spec.
This CL changes the representation of a promotion chain to a
non-nullable `List<Type>`, and represents an empty promotion chain as
an empty list.
To reduce the risk of mistakes, I've tried to minimize the changes to
flow analysis unit tests; for the most part, they still consider an
empty promotion chain to be represented as `null`, and convert to the
new representation at the last minute. In a follow-up CL, I'll modify
the flow analysis unit tests to better follow the new representation.
Change-Id: I72afd18f9d7729f3109e3e3b5c776c5a23860985
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443540
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates the mini_ast testing infrastructure so that:
- It properly models the "exception type" part of a catch clause (the
type named after the `on` keyword).
- It requires an exception variable to be specified if there is a
stack trace variable (this is required by the Dart grammar).
- It requires an exception type to be specified if there is no
exception variable (this is required by the Dart grammar).
- During the "pre-visit" stage, the exception variable and stack trace
variable are registered with the `AssignedVariables` object, so that
they can be properly handled by type promotion.
- During the main "visit" stage, the exception variable and stack
trace variable are assigned the appropriate types.
Flow analysis unit tests are updated in order to meet the new
requirements, and flow analysis tests are added to check that stack
trace and exception variables are promotable and appropriately typed.
By ensuring that stack trace and exception variables are properly
typed during flow analysis tests, this paves the way for some
follow-up work, in which I plan to re-work how flow analysis keeps
track of variable types.
There is no behavioral change to the analyzer or compiler pipeline;
these changes are confined to `pkg/_fe_analyzer_shared/test`.
Change-Id: I49c4b894d82d1dc58d62e3d3f25d232c9106922e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/434145
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This change introduces some helper functions to avoid duplication
between the enabled/disabled variants of each of the try/finally
ordering tests.
Thanks to Lasse for the suggestion.
Change-Id: I4538d44643fde7954a75bc1b6843fcc805d8afc4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432782
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change adds test-only methods `propertyPromotionChainForTesting`
and `variablePromotionChainForTesting` to flow analysis; these are
used by the flow analysis unit tests to query the full promotion chain
of a porperty (or variable, respectively). This allows tests to
observe the effect of try/finally ordering on promotion chains without
having to resort to clever control flow joins.
Thanks to Lasse for the suggestion.
Change-Id: Ida5349ecc93dbd4b3392becd20882be3eb101024
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432800
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
When an `is` test is trivially satisfied (i.e. `expr is T`, when the
static type of `expr` is a subtype of `T`), the `is` test is
guaranteed by soundness to evaluate to `true`, so any code path that
follows from the `is` test evaluating to `false` is unreachable.
This reasoning wasn't valid prior to sound null safety, because in
mixed mode programs, it was possible for an expression to evaluate to
`null` even if its static type wasn't nullable, and hence `expr is T`
might evaluate to `false` even if the static type of `expr` was a
subtype of `T`. So this change is gated on the `sound-flow-analysis`
language flag (which is enabled in Dart 3.9).
Fixes https://github.com/dart-lang/sdk/issues/60718.
Change-Id: I66a65580b738162f23b6fb468b71fcac66bfbb95
Bug: https://github.com/dart-lang/sdk/issues/60718
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431740
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The following renames are performed:
- The class `Declare` (which is the "mini-AST" representation of a
pattern variable declaration), is renamed to
`PatternVariableDeclaration`.
- The top level function `match` (which is used in tests to construct
a "mini-AST" representation of a pattern variable declaration), is
renamed to `patternVariableDeclaration`.
The new names should help avoid confusion in a follow-up CL I intend
to create, which will introduce a new `VariableDeclaration` class to
represent ordinary variable declarations.
There is no functional change. These renames only affect tests in
`pkg/_fe_analyzer_shared`.
Change-Id: Ic6b6f75cbb312180273a08f2c3926f83da254bc0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432120
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, when assiging to a local variable that was promoted, if
the newly assigned value was not compatible with any of the promotions
(i.e., the variable was fully demoted back to its declared type), then
the set of types of interest was cleared.
This behavior was not documented anywhere in the spec, and it seems
oddly inconsistent to me; as far as I can tell, flow analysis doesn't
clear types of interest in any other circumstances. I've looked
through git history as well as my personal notes, and I've been unable
to find any justification for this behavior. So, with the agreement of
the language team, I'm removing it when sound-flow-analysis is
enabled.
Fixes https://github.com/dart-lang/language/issues/4380.
Bug: https://github.com/dart-lang/language/issues/4380
Change-Id: Ic1ca80a61e21482e659afa8796b08fce707db3c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429227
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
A tricky part of the implementation of flow analysis is the handling
of try/finally statements. Although promotions are tracked separately
in the `try` and `finally` blocks, promotions from both blocks need to
be merged together at the conclusion of the finally block. This
creates an ambiguity, because each type in a promotion chain is
required to be a subtype of the previous, and hence multiple
promotions of the same variable are inherently ordered. The ambiguity
is: when the promotions from the `try` and `finally` block are merged,
which promotions should be applied first?
In discussion with the language team, we've decided that the
promotions from the `try` block should be applied first, because that
matches the order of code execution. This change makes the behavior of
flow analysis more uniform, which should make it easier to reason
about and maintain.
In practice, the difference in behavior is quite subtle, and I don't
expect users to notice. However, to be on the safe side, the change in
behavior is conditioned on the `sound-flow-analysis` flag, so it will
only take effect when the user deliberately upgrades to language
version 3.9, and it will not affect already-published packages.
A test in google3 showed that no internal code would be broken by
force-enabling this change.
Fixes https://github.com/dart-lang/language/issues/4382.
Change-Id: I0e9f6db808a964e0b4325d3020654a9f2be273a2
Bug: https://github.com/dart-lang/language/issues/4382
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432001
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, flow analysis had the rule that type promotion only
occurred when the type being tested was a subtype of the previously
promoted type (or the declared type, if there was no previous
promotion). This led to counterintuitive behaviors when the type being
tested and the previously promoted type were mutual subtypes (see
https://github.com/dart-lang/language/issues/4368).
With this change, the rule is updated so that type promotion only
occurs when the type being tested is a subtype of the previously
promoted type _and_ the previously promoted type is _not_ a subtype of
the type being tested. The user-visible difference is that promotion
to a mutual subtype no longer occurs.
This change makes flow analysis easier to reason about, and improves
its behavior in corner cases, but I believe it will have minimal
impact on real-world code. But to reduce the risk to existing code,
the change only takes effect when the `sound-flow-analysis` language
feature is enabled.
Fixes https://github.com/dart-lang/language/issues/4368.
Bug: https://github.com/dart-lang/language/issues/4368
Change-Id: I30dab017e043e75603d618df721c8a2683667cd5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429200
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
I accidentally called the test group "Sound null safety", which is a
related concept, but not the same thing.
Sound null safety is a compilation mode in which no legacy code is
allowed, and so it is sound to assume that an expression with a
non-nullable type cannot evaluate to `null`. It has been the only
allowed way to compile Dart programs for some time.
Sound flow analysis, on the other hand, is a set of improvements to
flow analysis which are possible now that all Dart programs are now
compiled in sound null safety mode. It is a language-versioned feature
that is enabled in Dart 3.9.
Change-Id: Ib13d28be7fecea6d0e931b33d6643afab2349629
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432004
Auto-Submit: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
The way this is accomplished is that in
`_FlowAnalysisImpl.nullAwareAccess_rightBegin`, any expression
reference associated with the target of the null-aware access is
restored, and the corresponding SSA node is associated with the guard
variable (if any). These changes ensure that if the null-aware access
is a property get, the subsequent call to `propertyGet` will pick up
the appropriate SSA node, so it will be able to locate the promotion
key for the property.
This functionality is only enabled when the language feature
`sound-flow-analysis` is enabled.
To prevent test regressions, a few related changes need to be made at
the same time:
- `_FlowAnalysisImpl.nullAwareAccess_end` is changed so that it clears
any expression info or expression reference that was associated with
the null-aware access expression. This prevents flow analysis
information from being erroneously propagated out of a null-aware
expression, which would have led to assertion failures when
analyzing null-aware expressions inside of conditional
expressions. This wasn't previously a problem because the expression
reference used to be consumed by
`_FlowAnalysisImpl.nullAwareAccess_rightBegin`, preventing further
expression references and expression infos from being recorded
further along in the null-aware access.
- The test framework in `mini_ast.dart` is fixed so that `!` is
considered to participate in null shorting. This was a bug in the
test framework that wasn't previously caught because it happened not
to produce any test failures.
- The analyzer's method `PostfixExpressionResolver._resolveNullCheck`
is changed so that it calls `nonNullAssert_end` before terminating
null-aware access. Previously, the order was swapped, causing
`nullAwareAccess_end` to be called before `nonNullAssert_end` when
analyzing expressions like `a?.b!`. This used to be benign, but now
that non-cascaded field accesses participate in field promotion,
flow analysis needs the methods to be called in the correct order.
Fixes https://github.com/dart-lang/language/issues/4344.
Bug: https://github.com/dart-lang/language/issues/4344
Change-Id: I523be1b4be1af3f68654a745187a546728c878fe
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/427820
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
These unit tests exercise flow analysis behaviors for patterns that
one might plausibly assume are part of the `sound-flow-analysis`
feature, but actually have been present ever since the `patterns`
feature was introduced.
Adding these tests helps me be confident that the behavior of flow
analysis after `sound-flow-analysis` has all of the soundness
behaviors I expect; even though some of those behaviors aren't new.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: I77a269907c67d643d0ef23d4f76545e36761bbfc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421960
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates the logic in the flow analysis method
`promoteForPattern`, so that when the language feature
`sound-flow-analysis` is enabled, the following additional behaviors
are added:
- If the matched value type is non-nullable, and the pattern
implicitly performs an `is Null` test, then the pattern is known not
to match.
- If the matched value type is `Null`, and the pattern implicitly
performs an `is T` test, where `T` is a non-nullable type, then the
pattern is known not to match. Note that this reasoning step is
sound regardless of whether the program is running with sound null
safety enabled, but since it is a new reasoning step, it only takes
place if the `sound-flow-analysis` feature is enabled.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: I5a6e8def050c95b6c1ad01d37584d17a0cd590c8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421900
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This change updates the flow analysis logic for map patterns, so that
when the language feature `sound-flow-analysis` is enabled, an empty
map pattern is considered to match a non-nullable map.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: I3f5a79c00cfe91d37528a3790b5cedb9a8f010fc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421584
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates the flow analysis logic for null check patterns,
so that when the language feature `sound-flow-analysis` is enabled,
the matched value type is checked for nullability. If it's
non-nullable, then the null check pattern is known to succeed.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ie68d98d95e30053f992a3f8db6ccdd3978960eb7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421583
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
It turns out that the flow analysis logic for null-aware map entries
has always presumed sound null safety. That is, given a map entry with
a null-aware key (`{?x: y}`), flow analysis assumed that if the key
was non-nullable, then the value was guaranteed to execute.
However, another piece of logic that could have been implemented, and
wasn't, was that if the key had static type `Null`, then the value was
guaranteed _not_ to execute. This logic would have been sound even
without assuming sound null safety (because even in unsound null
safety mode, the type `Null` was only inhabited by the value `null`).
This change implements the missing logic. Even though it doesn't
strictly depend on the assumption sound null safety, it still makes
sense to guard it by the `sound-flow-analysis` flag, because (a) it's
a potentially breaking change, and (b) it brings the flow analysis
behavior of null-aware map entries into alignment with the other
behaviors that are being implemented as part of the
`sound-flow-analysis` feature.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ie711f582660a31a411be0dc339995df140feb04f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420800
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates the flow analysis logic for `??` and `??=`
expressions, so that when the language feature `sound-flow-analysis`
is enabled, the static type of the left hand side is checked for
nullability. If it's non-nullable, then the right hand side of the
expresison is considered unreachable.
These new behaviors break assumptions made by two pre-existing flow
analysis tests. I changed those tests to run with
`sound-flow-analysis` disabled.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: I33d6d256bd3c41b764245f50ad34eb5c8b33878e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420740
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
This change updates the flow analysis logic for `?.` expressions, so
that when the language feature `sound-flow-analysis` is enabled, the
static type of the target is checked for nullability. If it's
non-nullable, then the "shortcut" control flow path (the control flow
path in which the null-aware operation is not executed) is considered
unreachable.
One pre-existing flow analysis test was made redundant by this
change. Another needed a minor tweak to continue passing.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: I3cd92dd49d4393b40f0ec888b643f0353de5e2d4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420466
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change updates the flow analysis logic for `==` and `!=`
expressions, `==` and `!=` patterns, and constant patterns, so that
when the language feature `sound-flow-analysis` is enabled, the static
types of the two expressions being compared are checked for
nullability. If one of the types is non-nullable and the other type is
`Null`, then it is known that the values will be unequal.
Note that these new behaviors break assumptions made by several
pre-existing flow analysis tests. I was able to adjust some of the
tests to preserve their old behavior, either by adjusting expectations
or running the test with `sound-flow-analysis` disabled. Some other
tests became redundant, so I removed them.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ib65477e064bb8dcd761542ebe187843fe265a24b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420463
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
This change updates the flow analysis logic for `is` and `as`
expressions so that when the language feature `sound-flow-analysis` is
enabled, the static type of the operand is compared to the type to the
right of the `is` or `as` keyword. If one of the types is non-nullable
and the other type is `Null`, then the type test is known to fail. For
an `as` expression, this means that the code path following the
expression will be marked as unreachable. For an `is` expression, this
means that any code paths that assume it evaluates to `true` will be
marked as unreachable.
Note that these new behaviors break assumptions made by three
pre-existing flow analysis tests. I was able to adjust one of the
tests ("equalityOp_end does not set reachability for `this`") to
preserve its old behavior. The other two tests became redundant, so I
removed them.
There is no behavioral change if the feature `sound-flow-analysis` is
disabled.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ib3a9e96bd39cf7df4c6c297568763c0f25bc9e39
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420164
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, flow analysis was configured by passing a set of named
booleans to its constructor, each to enable or disable a separate
language feature.
This change merges the flow analysis configuration with the
`TypeAnalyzerOptions` class (which was already being used for
configuring the shared `TypeAnalyzer` class). This should make it
easier to add language features to the shared code base in the future,
since there will be one common place where all features will be
configured.
To avoid duplicating the logic that creates `TypeAnalyzerOptions`,
I've had to do a bit of minor surgery to the clients:
- In the test harness in `_fe_analyzer_shared`, there is a common
method (`Harness.computeTypeAnalyzerOptions`) that constructs
`TypeAnalyzerOptions` based on the harness configuration. It is
called from a few different tests.
- In `pkg/analyzer`, there is a common method
(`computeTypeAnalyzerOptions`) that constructs `TypeAnalyzerOptions`
based on a `FeatureSet`. It is used by
`LibraryAnalyzer.analyzeForCompletion`,
`LibraryAnalyzer._resolveFile`, and the late variable
`AstResolver._typeAnalyzerOptions`.
- In `pkg/front_end`, I've moved computation of `TypeAnalyzerOptions`
from the `InferenceVisitorImpl` constructor to the
`TypeInferrerImpl` constructor; the options are then passed to the
`InferenceVisitorImpl` by `_createInferenceVisitor`.
I'm doing this work now as preparation for adding support for sound
flow analysis (https://github.com/dart-lang/sdk/issues/60438), so that
I can add the logic to enable it in a clean way.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ib845194adb404b4c0a3feeff17a14ae641d515eb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419940
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
A null-aware access can result in dead code if the target has static
type `Null`. The analyzer wasn't properly accounting for this,
resulting in some confusing ranges reported for the DEAD_CODE warning.
This change causes the following expressions to report dead code for
the code ranges indiced by `^`:
Null myNullVar = null;
myNullVar?[index];
// ^^^^^^ DEAD_CODE
myNullVar?[index] = value;
// ^^^^^^^^^^^^^^ DEAD_CODE
myNullVar?.method();
// ^^^^^^^^ DEAD_CODE
myNullVar?.property;
// ^^^^^^^^ DEAD_CODE
myNullVar?.property = value;
// ^^^^^^^^^^^^^^^^ DEAD_CODE
Note that the bug was confined solely to the logic that reports the
DEAD_CODE warning; there is no change to the reachability inferred by
flow analysis (and hence, this is a non-breaking change).
Fixes https://github.com/dart-lang/sdk/issues/60364.
Bug: https://github.com/dart-lang/sdk/issues/60364
Change-Id: I068826282fba6b9057e9c27d1d9310c65714e203
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416723
Auto-Submit: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
The getter `SharedType.nullabilitySuffix` is replaced by
`SharedType.isQuestionType`, which returns a boolean.
The method `TypeAnalyzerOperations.withNullabilitySuffixInternal` is
replaced by `SharedType.setNullabilitySuffix`, which accepts a
boolean.
Support for `*` types has been removed from `mini_types.dart`.
A few test cases in `flow_analysis_test.dart` previously used `*`
types as a way of exercising corner cases involving types that were
mutual subtypes of each other. These tests have been changed to take
advantage of the fact that `dynamic` and `Object?` are mutual
subtypes.
Change-Id: Id9904f9570fc738b388192db8536848204af03e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/414581
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This field was only used to populate expectation strings in "id"
tests; it did not affect any user-visible behavior of the analyzer or
CFE.
Including information in "id" tests that doesn't affect any
user-visible behavior isn't helpful. Removing this field will enable
some upcoming flow analysis refactoring work (I intend to remove the
`ExpressionInfo._type` field, replacing its remaining usages with a
more reliable mechanism).
Change-Id: Id4c6593fae4ef25b8c21f0625e6c1f9eaa766e17
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406403
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Erik Ernst