9fec00aeedea94ac96d6b97f9ae36910591c826f
268 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Kallen Tu
|
c8ee0e78a5 |
[flow] Pipe the language feature flag inference-update-4 through flow analysis.
Weaves the flag `inference-update-4` through to `FlowAnalysis` and updating both the analyzer and CFE point-of-entry to include the flag. This flag will be used in `flow_analysis.dart` to hide upcoming bug fixes to flow analysis. Change-Id: Ib0004eb4bcf0b6e579116632b5973fe969e51e90 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/388582 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Kallen Tu <kallentu@google.com> Reviewed-by: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
38c08f919d |
[flow analysis] Remove ExpressionInfo.after.
Previously, the base `ExpressionInfo` class contained four fields: - `type`: the type of the expression. - `ifTrue`: a flow model describing the state of the program after the expression is evaluated, assuming the expression evaluates to `true`. - `ifFalse`: a flow model describing the state of the program after the expression is evaluated, assuming the expression evaluates to `false`. - `after`: a flow model describing the state of the prorgam after the expression is evaluated, making no assumptions about what value the expression evaluates to. The `after` field was largely redundant, since it tracked the same information as `FlowAnalysisImpl._current`. In fact, flow analysis contained a substantial amount of code to copy from `ExpressionInfo.after` to `FlowAnalysisImpl._current`, or vice versa, in order to keep the two in sync. The one exception was in `FlowAnalysisImpl.conditional_end`, which is called at the end of visiting a conditional expression (`e1 ? e2 : e3`): it joined the `after` flow models from `e2` and `e3` in order to determine the state of the program after the conditional expression completes. To preserve this behavior, a small amount of extra accounting logic had to be added to the handling of conditional expressions, to keep track of these flow models. (`e2.after` is now stored in `_ConditionalContext.thenModel`, and `e3.after` comes from the state of `_current` at the time of entry into `FlowAnalysisImpl.conditional_end`). Change-Id: I46e771f8b029550d43a5fe50366177f189a6a91d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/388081 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Kallen Tu <kallentu@google.com> |
||
|
Paul Berry
|
b452721942 |
Change "mini types" type parameters to match CFE and analyzer.
Previously, the "mini types" representation used in `_fe_analyzer_shared` unit tests represented unpromoted type parameters using the `PrimaryType` class (which was also used for interface types and special built-in types like `void`) and represented promoted type parameters using a separate `PromotedTypeVariableType` class. This CL changes the "mini types" representation to use a single `TypeParameterType` class for both unpromoted and promoted type parameters. This parallels the representation used by the analyzer and CFE, so it should help pave the way for sharing type system logic between the analyzer and CFE. To allow the `Type` constructor to distinguish whether a given identifier represents an interface type or a type variable, tests must register all type names they will need, using either the static method `TypeRegistry.addTypeParameter` or the static method `TypeRegistry.addInterfaceTypeName`. To prevent the type names registered by one unit test from interfering with those registered by another, tests should call `TypeRegistry.init` in a `setUp` callback and `TypeRegistry.uninit` in a `tearDown` callback. Methods in `TypeRegistry` contain error checks to help make sure these calls aren't forgotten. Change-Id: I701842ad94899c819f1a059e660510a616d00456 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/387822 Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Commit-Queue: Paul Berry <paulberry@google.com> |
||
|
Chloe Stefantsova
|
bc8104f3b2 |
[analyzer][cfe] Implement flow analysis for null-aware map entries
Closes https://github.com/dart-lang/sdk/issues/56786 Change-Id: I738c98b6f4e632cfbbe51221bbc3547edbc718fe Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386800 Commit-Queue: Chloe Stefantsova <cstefantsova@google.com> Reviewed-by: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
13d4eaf99a |
Implement matchInferableParameter in MiniAstOperations.
Previously the implementation of this method was a stub. It turns out that all the necessary infrastructure was in place already, however the type arguments supplied by MiniAstOperations to TypeAnalyzerOperationsMixin and TypeAnalyzerOperations needed to be changed: in the "mini_ast" representation of types, an InferableParameter is represented by a String, not a PromotedTypeVariableType. This is because InferableParameter is meant to represent the declaration of the type parameter (StructuralParameter for the CFE, TypeParameterElement for the analyzer), not the type itself. The types used for unit testing in _fe_analyzer_shared don't have a separate notion of the declaration of a type parameter, so we just use its name. Implementing this logic required adding a method `TypeSystem.matchTypeParameterType`, which checks if a Type is a type parameter type, and returns the name of the type parameter if so. I based this on the previously existing `TypeSystem._isTypeVar` method (which performed the same job but did not return the type parameter name). I also took the liberty of fixing a flow analysis test that treated `T` as a type variable but failed to mark it as a type variable by calling `addTypeVariable`. This should help pave the way for unit testing more of the shared infrastructure for types. Change-Id: Ia7a9777ec3d90a5886567dcb9f831e388e372f32 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386607 Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Commit-Queue: Paul Berry <paulberry@google.com> |
||
|
Chloe Stefantsova
|
3acd125e44 |
[analyzer][cfe] Implement types and type schemas as extension types
This CL removes Type and TypeSchema type variables from the abstract classes with shared code between the CFE and the analyzer. Extension types SharedTypeView and SharedTypeSchemaView are declared to replace the type variables. The update propagates the discipline of distinguishing between types and type schemas into the clients of the shared code. Now the code in the CFE and the Analyzer that uses the shared code needs to statically specify the interpretation of their type objects as either types or type schemas. Another benefit of the update is SharedTypeView and SharedTypeSchemaView being less opaque than the Type and TypeSchema type variables, which removes the necessity for some code duplication in abstract methods for types and type schemas. Finally, the update enables some further changes in the shared code between the Analyzer and the CFE. Change-Id: I88e8cfcd47d4f721974b4f2612521e85bb54c30f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/379302 Commit-Queue: Chloe Stefantsova <cstefantsova@google.com> Reviewed-by: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
b026068313 |
Flow analysis: fix handling of list pattern type promotion.
When analyzing the type test implied by a pattern, flow analysis uses three variables to control promotion behavior: - `matchFailsIfWrongType`, which indicates whether flow analysis needs to account for the possible control flow path resulting from the type test failing. (This is `false` for cast patterns, because in the case where a cast pattern fails, an exception is thrown). - `matchMayFailEvenIfCorrectType`, which indicates whether flow analysis needs to account for the possible control flow path resulting from the type test succeeding, but some other check causing the match to fail. (This is `true` for most list patterns, because the list pattern will fail to match if the list has the wrong length). (Note that `matchMayFailEvenIfCorrectType` doesn't account for the fact that a pattern match might fail due to failure in a subpattern match; this is automatically handled by the fact that flow analysis walks through the complete pattern in the order in which it executes.) - `coversMatchedType`, which indicates whether the type test is guaranteed to succeed due to a subtype relationship between the matched value type and the type being tested (e.g. a `num x` pattern is guaranteed to succeed if the matched value type is `int`). In the case where `matchFailsIfWrongType` is `true`, `matchMayFailEvenIfCorrectType` is `true`, and `coversMatchedType` is `false`, flow analysis must account for the fact that there are two ways that the pattern match might fail: the type test might fail, or the type test might succeed but then the pattern match might fail for some other reason. Before this change, this was done incorrectly, and flow analysis only accounted for the possibility of the type test failing. Fixes #55543. Bug: https://github.com/dart-lang/sdk/issues/55543 Change-Id: I86603ec5f940402313f32177212b7960878db97f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364942 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
0c7585c907 |
Start building a shared class hierarchy for types.
This commit introduces the following new classes: - SharedType, which represents the common interface between the DartType classes in the analyzer and the CFE. - SharedRecordType, which represents the common interface between the RecordType classes in the analyzer and the CFE. - SharedNamedType, which represents the common interface between the analyzer and CFE representations of a name/type pair. - SharedUnknownType, which represents the common interface between the analyzer and CFE representations of the unknown type (`_`). This allowed three methods to be removed from the `TypeAnalyzerOperations` class: - `areStructurallyEqual`, which is replaced by `SharedType.isStructurallyEqualTo`. - `asRecordType`, which is no longer needed because `is SharedRecordType` can be used instead. - `isUnknownType`, which is no longer needed because `is SharedUnknownType` can be used instead. And one method to be removed from the `FlowAnalysisTypeOperations` class: - `isSameType`, which is replaced by `operator ==`. (Technically this could have been done even without introducing a shared class hierarchy, since `operator ==` is defined in the shared base class `Object`). The long term goal is to fill out the shared class hierarchy to cover other kinds of types (interface types, function types, void, etc.), and to move most of the shared logic from the analyzer and CFE DartType class hierarchies into shared code. This should reduce the risk of implementation skew between the analyzer and CFE, and to streamline the implementation of future features. Additionally, the hope is to eventually remove, or drastically simplify, classes like `TypeAnalyzerOperations`, so that the code in `_fe_analyzer_shared` can be written in simpler and more straightforward way. Change-Id: I5d3a929057959f77ccff8dbed5671f9bca6259c5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362481 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Commit-Queue: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
97edad1568 |
Flow analysis: Use extension type erasure for implicit is reachability.
Whenever a pattern performs an implicit `is` test, flow analysis attempts to determine whether the `is` test is guaranteed to succeed; if it is, then flow analysis considers the code path in which the `is` test fails to be unreachable. This allows flow analysis to recognize switch statements that are trivially exhaustive (because one of the cases is guaranteed to match), avoiding spurious errors such as "variable must be assigned before use" or "missing return statement". This change upgrades the logic for computing when an `is` test is guaranteed to succeed, so that it accounts for type erasure of extension types. This brings flow analysis's treatment of switch statements into closer alignment with the exhaustiveness checker, which should reduce the risk of confusing error messages. For more information see https://github.com/dart-lang/language/issues/3534#issuecomment-1885839268. Fixes https://github.com/dart-lang/language/issues/3534. Bug: https://github.com/dart-lang/language/issues/3534 Change-Id: Ib73d191e04e7fa8c0e6888c2733dae73d8f389da Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345822 Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Commit-Queue: Paul Berry <paulberry@google.com> |
||
|
Paul Berry
|
c9cf5b9916 |
Add a type parameter to TypeAnalyzer to represent type schemas.
The front end and analyzer use the same representation for types and type schemas, with the unknown type schema (`_`) treated as a pseudo-type. This creates the risk of accidentally mixing types and schemas, resulting in `_` accidentally "leaking" into the type system and showing up in static analysis results or error messages. As a step toward reducing this risk, this change adds a type parameter to `TypeAnalyzer`, preventing the shared type analysis code from being able to assume that types and schemas are represented the same. This extra discipline makes it much easier to search through the code and identify how types are manipulated vs. how type schemas are manipulated, and makes it impossible for the shared type analysis to accidentally leak `_` into the type system. I believe this change will also make it easier to implement some type inference improvements we've been contemplating, such as improved type inference of `.map(...).toList()`, as well as https://github.com/dart-lang/language/issues/3471, because those improvements may require introducing new kinds of type schemas. Change-Id: Ifcd7e2c4e1172ee39719ce8c8b10d7f10f6a7b6f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345353 Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> |
||
|
Paul Berry
|
5510ef63a6 |
Clean up shared Operations classes.
The classes `Operations`, `TypeOperations`, and `VariableOperations` are reworked as follows: - The class `FlowAnalysisOperations<Variable, Type>` contains the API used by flow analysis to manipulate the client's representation of types and variables. This class (and the classes that support it) is located in `flow_analysis_operations.dart`, a sibling of `flow_analysis.dart`. - Note that `FlowAnalysisOperations<Variable, Type>` has a supertype, `FlowAnalysisTypeOperations<Type>`, which contains the portion of the API that is only concerned with types. This simplifies the implementation of flow analysis, by letting some of its lower-level operations be agnostic to how the client represents variables. - The class `TypeAnalyzerOperations<Variable, Type>` contains the API used by the shared type analyzer to manipulate the client's representation of types and variables. This class (and the classes that support it) is located in `type_analyzer_operations.dart`, a sibling of `type_analyzer.dart`. - Several abstract methods and getters that were previously in the `TypeAnalyzer` class are moved to `TypeAnalyzerOperations`, for consistency with `FlowAnalysisOperations`. Since the type analyzer calls out to flow analysis, the class `TypeAnalyzerOperations` is a subtype of `FlowAnalysisOperations`. This means that clients only need to implement a single API, `TypeAnalyzerOperations`. Change-Id: I7f33976d49bc998a7edd3f07f31576fe54886999 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/343280 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> |
||
|
Paul Berry
|
d42390d7c1 |
Clean up enable/disable logic for flow analysis and type inference tests.
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> |
||
|
Paul Berry
|
c23ac067eb |
Improve the behavior of "why not promoted" when field promotion disabled.
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> |
||
|
Paul Berry
|
bb524468ef |
Wire up "why not promoted" messages for failed field promotion due to conflict.
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> |
||
|
Paul Berry
|
8529e31a0f |
Flow analysis: track detailed non-promotability reasons for getters.
- 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> |
||
|
Paul Berry
|
2887fe0b2c |
Flow analysis: rework of promotionInfo data structure.
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>
|
||
|
Paul Berry
|
f5208f5d59 |
Flow analysis: prepare for rework of promotionInfo data structure.
`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>
|
||
|
Paul Berry
|
ef74475404 |
Create an enum to represent links used in "why not promoted" context messages.
This makes it easier to see at a glance which messages need to be supported by the website. I've included the new links that I intend to support as part of the new "field promotion" feature. Bug: https://github.com/dart-lang/sdk/issues/53102 Change-Id: I67ad47c5a00db9807a6c726677a06427cdbe02c2 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/325803 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com> |
||
|
Paul Berry
|
602e38eab4 |
Flow analysis: fix integration of field promotion with object patterns.
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> |
||
|
Paul Berry
|
aef81ef08e |
Flow analysis: fix scrutinee promotion for fields.
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> |
||
|
Paul Berry
|
b29e4a8554 |
Flow analysis: unit test that join variables are usable and promotable.
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> |
||
|
Paul Berry
|
30e8ed8410 |
Flow analysis: fix field promotion based on condition variables.
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> |
||
|
Paul Berry
|
2ecc88e7ef |
Flow analysis: Replace merge with join followed by unsplit.
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> |
||
|
Paul Berry
|
5c018e5c65 |
Flow analysis: Simplify Reachability class.
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> |
||
|
Paul Berry
|
b9adb3d398 |
Flow analysis: adjust nomenclature to reflect field promotion.
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> |
||
|
Paul Berry
|
7703609cc1 |
Flow analysis: fix interaction of field promotion and try/finally.
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> |
||
|
Paul Berry
|
d31689ae2f |
Flow analysis: unify field promotions when joining SSA nodes.
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>
|
||
|
Paul Berry
|
907e705307 |
Flow analysis: use a more precise split point for refutable patterns.
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>
|
||
|
Paul Berry
|
9a3420f1d4 |
Flow analysis: fix field promotion within cascades of non-promotable targets.
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>
|
||
|
Chloe Stefantsova
|
bd951bb5ad |
[cfe] Treat promitions from T? to non-nullable as two-step
This CL introduces handling of promotions of the form `x is T` where `x` has static type `X?`, `X` is a type variable, and `T` is non-nullable. They are treated as an equivalent of `x == null || x is T`, effectively resulting in the static type of `x` being `X & T` in the promoted code. Closes https://github.com/dart-lang/sdk/issues/52973 Change-Id: Ibfa6e32e14202651db09cfb4fbbdec864552b96a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316381 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Chloe Stefantsova <cstefantsova@google.com> |
||
|
Paul Berry
|
5a79f8a9f3 |
Unify "mini-ast" constructs for rest pattern elements.
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> |
||
|
Paul Berry
|
03a91b5d2a |
Simplify handling of variable expressions in shared analysis tests.
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> |
||
|
Paul Berry
|
64ecabdf16 |
Add function second to flow analysis tests.
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>
|
||
|
Paul Berry
|
bcaab7e985 |
Make more "mini-ast" constructs into expressions.
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> |
||
|
Paul Berry
|
7dc587d344 |
Simplify handling of switch heads in shared analysis tests.
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> |
||
|
Paul Berry
|
ecf71eb63b |
Simplify handling of collection elements in shared analysis tests.
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> |
||
|
Paul Berry
|
53ead360bb |
Flow analysis: Test that functionExpression_begin() preserves promotions of initialized vars.
While working on some other changes to flow analysis, I discovered that this particular behaviour wasn't unit tested. Change-Id: Ia9b27672c62177ffed80d4143f33c5b764ac7bbe Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313242 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com> |
||
|
Paul Berry
|
6233cede64 |
Simplify the "mini-ast" for shared flow analysis and type analysis tests.
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> |
||
|
Paul Berry
|
3ae6793469 |
Flow analysis: remove redundant assertion from constructor.
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> |
||
|
Paul Berry
|
6dff4755e6 |
Rename Ir to IR, per dart style conventions
"IR" is a two-letter acronym (short for "intermediate representation"), not a two-letter abbreviation, so both the "I" and the "R" should be capitalized. See https://dart.dev/effective-dart/style#do-capitalize-acronyms-and-abbreviations-longer-than-two-letters-like-words. Change-Id: I641b6d4ffc656ba9cd527e05df6e865527ec56d3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/310080 Auto-Submit: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Johnni Winther <johnniwinther@google.com> |
||
|
Paul Berry
|
80bc65bb0b |
Flow analysis: add field promotion support for cascades.
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> |
||
|
Paul Berry
|
39de3f2e5b |
Field promotion: track field accesses via SSA node.
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>
|
||
|
Paul Berry
|
709c0e4ccd |
Flow analysis: combine ExpressionInfo, ReferenceWithType, and EqualityInfo.
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> |
||
|
Paul Berry
|
db46a46455 |
Fix incorrect map indexing operation in flow analysis test.
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> |
||
|
Sam Rawlins
|
f292b20dfd |
Replace deprecated lint rules in front-end packages
Two rules, `iterable_contains_unrelated_type` and `list_remove_unrelated_type` are deprecated (which fails the CI) and replaced with `collection_methods_unrelated_type`. See https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8780614474452305057/+/u/analyze_pkg_/stdout for the failures resulting from referencing deprecated rules. Change-Id: I00087e684bfb230b5dc94ab832c969ed67a631f7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/304780 Commit-Queue: Samuel Rawlins <srawlins@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> |
||
|
Paul Berry
|
70e7adff19 |
Field promotion: distinguish between this. and super. accesses.
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>
|
||
|
Konstantin Shcheglov
|
bd0e77c4fc |
Issue 52409. Reading a Never typed getter makes the flow unreachable.
Bug: https://github.com/dart-lang/sdk/issues/52409 Change-Id: I675ed8f8cfa0d2e5327c2c6bc0fb1bda7bde038c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303840 Reviewed-by: Paul Berry <paulberry@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com> |
||
|
Paul Berry
|
5043c38d25 |
Flow analysis: additional tests for error types.
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> |
||
|
Konstantin Shcheglov
|
c0d33e45d1 |
Issue 52151. Fixes for reachability in switch patterns when unresolved / error type.
Bug: https://github.com/dart-lang/sdk/issues/52151 Change-Id: Ib56df1711cbfb7a593c7b16fd6a67e45baeabc3c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302455 Reviewed-by: Paul Berry <paulberry@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> |
||
|
Jens Johansen
|
c0c9c98787 |
[flow analysis] Don't create a new empty map for every FlowModel
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>
|