In certain error recovery situations, it's possible for code to refer
to a variable whose declaration has been lost by error recovery
mechanisms. To prevent flow analysis from crashing when this
happened, it assumed that any reference to a variable whose
declaration had not yet been seen was valid, and implicitly added that
variable to the flow analysis state.
This created a subtle problem: if a function contained a closure that
declared (and assigned to) a local variable, at the time the closure
was entered, flow analysis would get confused and temporarily put the
variable in the "write captured" state (because it hadn't yet seen the
declaration of the variable, so it didn't realize it was local to the
closure). Then, a boolean variable might capture that incorrect
state. Later, upon seeing the declaration of the variable, it would
fix the incorrect state, however it was possible that a later
reference to the boolean variable would re-vivify the old incorrect
state. This is precisely what happened in issue #47991.
This CL fixes the problem by giving flow analysis the ability to
detect, at the time the FlowAnalysis object is constructed, all
variables that are referred to but not explicitly declared, and add
them to the flow analysis state. This allows it to safely assume that
any variables that are not yet in the flow analysis state haven't been
declared yet (and hence can be ignored), so no variable is every
erroneously placed into the "write captured" state.
Fixes#47991.
Bug: https://github.com/dart-lang/sdk/issues/47991
Change-Id: I8d84fab96fad063f1d3ade3b8b9a6e9af88c3737
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/227361
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This change adds assertions to ensure that the first phase of flow
analysis (calls to the AssignedVariables class) is complete before the
FlowAnalysis class is created. It also calls AssignedVariables.finish
(which does additional assertions) from the FlowAnalysis constructor
(if the client hasn't done so already).
Adding these assersions exposed a few minor (but benign) bugs in the
usage of the flow analysis API.
- The flow analysis unit tests were failing to call
AssignedVariables.declare. This should have been caught by
AssignedVariables.finish, but the flow analysis unit tests were
failing to call that as well.
- Several flow analysis unit tests failed to declare the variables
they were using (or in one case, the declaration was at the wrong
location).
- In the CFE, DeclaredSourceConstructorBuilder.buildOutlineExpressions
was failing to set up scopes properly when compiling a const
constructor.
- In the CFE, SourceLoader.buildExpression was failing to call
AssignedVariables.declare for variables in scope.
- In the CFE, the TypeInferrerImpl constructor was constructing the
FlowAnalysis object eagerly, even though the AssignedVariables phase
often hadn't completed yet. I've fixed this by constructing
FlowAnalysis lazily.
A follow-up CL will take advantage of the invariants checked by these
assertions to introduce the logic necessary to fix issue #47991.
Change-Id: Id680020dcd525632afa0820bc49c7b966b1ea534
Bug: https://github.com/dart-lang/sdk/issues/47991
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/227320
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
These behaviors were introduced during the fix for
https://github.com/dart-lang/language/issues/1785, and at the time
they were tested using both unit tests and language tests. But it was
not possible to write ID tests for them, because ID tests don't
support turning on experimental language feature flags.
Now that the "constructor-tearoffs" feature has been turned on we can
test these behaviors using ID tests.
Change-Id: I6e1ccda4b5837ab61de80f15d2f31a82a90b4a22
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212265
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Previously, the analyzer and CFE were responsible for two pieces of
logic that arguably should be in the shared flow analysis engine:
- Deciding whether or not it's necessary to tell flow analysis about
initializer expressions.
- Deciding whether or not to promote the variable at the time of
initialization (we do this when the variable is implicitly typed,
and the initializer is a promoted type variable type).
It's better to just always tell flow analysis about the initializer
expression and let it decide what to do about it.
This paves the way for fixing
https://github.com/dart-lang/language/issues/1785 (which results from
initializer expressions sometimes being ignored when they shouldn't
be), by consolidating the broken logic into the flow_analysis library,
where it will be easy to unit test the fix.
Bug: https://github.com/dart-lang/language/issues/1785
Change-Id: Iec832e92995eb4f8d0c1fbd4e9be6c897e0917b5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/211180
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The ID test offset for a prefixed identifier is now the offset of the
identifier after the `.`. This makes the analyzer and CFE behaviors
match much more closely, which eliminates a lot of nuisance
differences in the ID test expectations.
Change-Id: Icef1b2bf56b79bcf584d1eb153bdafd575e6cabb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195483
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Previously, if a type mismatch occurred on a named argument, e.g.:
int.fromEnvironment('foo', defaultValue: 'BAD')
the error would be reported on the entire NamedExpression
(`defaultValue: 'BAD'` in this example). Now, the error is just
reported on the expression (`'BAD'` in this example). This makes the
analyzer behavior more self-consistent (since now assignability errors
are now consistently reported at the site of the expression that can't
be assigned). It also makes the analyzer behavior more consistent
with the CFE.
This will make it possible, in a follow-up CL, to eliminate a lot of
the inconsistencies between analyzer and CFE in the "why not promoted"
ID tests.
Change-Id: I17db256959d730a22a5766f65d5c0736b627f244
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195489
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This makes the flow analysis tests more similar to the anaylzer and
CFE implementations, which look up the type of a `this.` property get
at the time it is analyzed, rather than storing it in the AST node.
Change-Id: Idee2a9c3ba0acbd2752db511c1aabfdff075b363
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194544
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Flow analysis unit tests now support implicit branch targets, and use
`LabeledStatement` directly in the case where the branch target is
explicit. This is more similar to how the language acutally behaves,
and paves the way for adding more sophisticated tests in the future.
Note that at the moment, there are no flow analysis tests where a
`continue` statement has a different target than the innermost loop,
so for now, the `continue_` function doesn't accept a target
parameter. I plan to add this functionality in the future, at the
time that I add tests that require it.
Change-Id: I2591ee3322ea4adba3e32dc76a29f0b3956058e5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194040
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This CL changes the responsibility for doing flow analysis of the
implicit variable write in a `for-in` loop as follows: if the `for-in`
loop does not declare a variable, but it assigns to a local variable,
then the flow analysis client is responsible for calling `write` on
entry to the loop.
This in turn allows us to use a single code path to track "why not
promoted" information related to all possible local variable writes;
we no longer need as much special case logic to handle for-in loops.
To make the analyzer integration slightly cleaner, we change the
argument type of FlowAnalysis.write to Node rather than Expression, so
that the analyzer can pass in the ForEachParts object when analyzing a
for-in loop.
Bug: https://github.com/dart-lang/sdk/issues/44898
Change-Id: I24b47be8eac2e276cd291a5b2f2e4444c911138f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193837
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This makes the flow analysis unit tests more similar to the behavior
of the analyzer and CFE, in that the types of various properties are a
property of the program being analyzed, not of the individual property
gets themselves.
Change-Id: I6ea164665093ee285325a64e80f3f52d7a63cf74
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193836
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The ID tests for the "why not promoted" feature had some minor
unintentional errors in them. This CL fixes them:
- The constructor tests in
argument_type_not_assignable_nullability_error.dart tried to return
null from functions whose return type didn't allow it.
- The compoundAssignmentRhs test in
argument_type_not_assignable_nullability_error.dart tried to add an
`int` to an `int?`. The CFE considers this to be an error, but also
reports a follow-on error because it thinks the type of the
resulting expression is `num` (which is not assignable back to
`int`). To avoid the follow-on error, we use `num` for the type of
the variable to which compound assignment is being applied.
- The forElementInMap test in
for_in_loop_type_not_iterable_nullability_error.dart wasn't a proper
map (it contained two `null` elements, rather than a single map
entry `null: null`, which was what was intended).
There were also several tests for which we *don't* expect "why not
promoted" information; I added comments explaining why.
Change-Id: I91ddb88cb0f25f9b209df3eeb75d57d8270f877b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193561
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Rather than representing an assignment differently depending on what
kind of construct appears on its left hand side (as kernel does), we
represent all assignments as a single `_Write` type, and use an
`LValue` base class to represent the different kinds of constructs
that can appear on the left hand side. This representation will be
easier to expand into supporting more of the Dart language as we
expand the testing we want to be able to do in _fe_analyzer_shared.
Change-Id: I87b811176b2ba132fb992414c277cf2c6b81e03c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193180
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Add helper on DartType to compute the `NonNull` of a type and use this
instead of `withDeclaredNullability(Nullability.nonNullable).
Includes a fix in the computation of NonNull of FutureOr.
TEST=existing
Change-Id: I3399cbf89c7d3f3e90f1315b01f40957e798a1b1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193400
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
We now use a single AST node to represent try/catch/finally rather
than separate try/catch and try/finally nodes. This more closely
parallels the data structures used by the analyzer and CFE.
Change-Id: Ic4c1f1e4ac9e10ebf36afa4047c9c8b9ce151a1f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193093
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The type of `this` is now stored in the test harness rather than in
the individual `this` AST objects. This more closely parallels how
`this` is handled in the analyzer and CFE.
Change-Id: I7c60aec8a14680beecca161ad25d681b82f79a2b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193092
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The failure message now includes the stacktrace from when the
_CheckPromoted node was constructed; this makes it possible to quickly
find the line in the source code that encodes the expectation, which
is often much more useful than the stacktrace from the time the
failure was detected.
The stacktrace from the time the failure was detected still appears,
after the stacktrace from when the node was constructed.
Change-Id: If375e141a32b371e806399e42ff7cfaae9cf7873
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192290
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This is necessary because `whyNotPromoted` needs to be called right
after visiting a subexpression, but it's not always possible to tell
that the type of that subexpression will result in an error until more
code has been visited.
Change-Id: If44e2bb55612c1ba7996b523c3ec078d7b80c865
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191601
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Rather than represent types by strings in the flow analysis tests, we
have a "mini type" language that can represent function types, type
parameters, nullabilities, and promotions. This should make it easier
to build more sophisticated tests for shared code.
Change-Id: I66d15ad3dc6a871958ed01654600aea749fa0e65
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190340
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Flow analysis stopped using this argument (the AST node for the
finally block) some time ago, but I kept it around so I could assert
that clients didn't unnecessarily store assigned variables info for
it. It's been long enough now that we can eliminate this code
entirely.
Change-Id: I8c64e2b4fc5b154f441ec2d057637f3dc9ced277
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190060
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This CL plumbs the types of `this` and property get expression from
the CFE and analyzer into flow analysis, so that flow analysis will be
able to create more accurate "why not promoted" information for those
expression types. This made it possible to eliminate a clumsy aspect
of the previous implementation, namely that we would consider a
promotion attempt like `if (x.y == null) return;` as an attempt to
promote the type of `x.y` to `Object`; now we compute the type the
user is actually trying to promote to, so we will be able to generate
more accurate "why not promoted" messages.
Bug: https://github.com/dart-lang/sdk/issues/44898
Change-Id: I67f9fc59e72103194a1ea6b1c4dfeae8aeb194a2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187064
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
The Node class hierarchy shouldn't be const constructible, because
flow analysis assumes it can distinguish nodes by identity, and we
don't want const canonicalization to prevent this.
Change-Id: I60f70a0ece77c8efbeaa001cbddb21d1dc8b4d6e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187980
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This CL adds support for the following scenarios to the analyzer:
- Attempt to use a non-promoted nullable expression as the iterable of
a for-in loop
- Attempt to use a non-promoted nullable expression as the argument of
a `yield *` statement
- Attempt to implicitly invoke `.call` on a non-promoted nullable
expression
- Attempt to use a non-promoted nullable expression as the argument of
a spread operator (`...`) that is not null-aware
Some of these cases are already handled by the CFE. Others will be
addressed in a follow-up CL.
Change-Id: I3cf31b1496e1bd92fdd3f8192f04c98dff15077c
Bug: https://github.com/dart-lang/sdk/issues/44898
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186320
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
When reporting that a null check is needed due to a lack of type
promotion, and the thing that was not promoted was a reference to a
field or property, the CFE and analyzer now report "why not promoted"
using a context message that points to the definition of the field or
getter. (Previously the CFE reported a context message with no
location, and the analyzer didn't report "why not promoted").
Bug: https://github.com/dart-lang/sdk/issues/44898
Change-Id: If1841727b8b60dd87c43f239e6a06b98f363801f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184522
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Paul Berry