Full implementation of core "why not promoted" algorithm #44898

stereotype441 · 2021-02-09T14:50:25Z

(Parent issue #44897)

a42244f added basic functionality to the CFE and analyzer to show reasons for a promotion failure, but it only addressed two narrow corner cases:

Erroneous access to a property due to the target being nullable, where the target is a local variable that was promoted but then later demoted due to a write.
Erroneous access to a property due to the target being nullable, where the target is itself a property or field (and hence not promotable).

This logic should be expanded to address all scenarios where promotion might fail.

The text was updated successfully, but these errors were encountered:

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: #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>

The new message phrasing should flow better, especially in the analyzer CLI. For the motivation for this change, please see #44904 (comment) Bug: #44898 Change-Id: I595492c64b42aff25cae5a8936c32aa8a218edd8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184601 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

This required moving the logic for computing the context messages from the inference visitor to the type inferrer. This includes support for extension method invocations and invocation of `.call` on a function type. Bug: #44898 Change-Id: Icda160f69cc815953c43edc92be910c5f49f9401 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184842 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>

These comments address code review comments from https://dart-review.googlesource.com/c/sdk/+/181741. Bug: #44898 Change-Id: I93d35600048318c20e6c41b290b6736d0086a574 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184980 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

This CL ensures that we produce appropriate "why not promoted" context messages in all the scenarios where the CFE reports the error code `NullableExpressionCallError`. Analyzer support for these scenarios will be in a follow-up CL. Bug: #44898 Change-Id: Id1407b28a97fc917189e885f995a4efc0bf0c250 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185920 Reviewed-by: Johnni Winther <johnniwinther@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: #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>

Bug: #44898 Change-Id: Ia42db3d9d84d7815e61444990805048c50d1c179 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187063 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

Bug: #44898 Change-Id: Ief6014c34aef329e5f408916f5d8ffe14a62effd Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187940 Reviewed-by: Konstantin Shcheglov <scheglov@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: #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>

This CL implements "why not promoted" functionality in the front end for the following scenarios: - null iterable in for-loop - null iterable in yield* statement - null iterable or map after spread (`...`) operator Bug: #44898 Change-Id: I471b8bf558341514207fad527dde009f1372182c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/188160 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

Bug: #44898 Change-Id: Iea44cd482f21ef4406b28e264046025ce3f87695 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191385 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

It's been a while since I worked with this method and I'd forgotten what the parameters meant. A sure sign that documentation and clarification are needed! :) Bug: #44898 Change-Id: I815c35af243ed6c9611346a058a1c1bb37ace4db Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191480 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

This should make it easier to implement "why not promoted" logic for arguments, since we'll be reporting the errors at a time when flow analysis information hasn't been discarded yet. Bug: #44898 Change-Id: I8499d711acc88b8397597b6116e5f6cfde43dc53 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191805 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

Bug: #44898 Change-Id: Ib39ef04aadc2340c269878763e19a4a97001e2b5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191989 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

…rators. `&&` and `||` will be addressed in a follow-up CL go through a different code path in the analyzer. Bug: #44898 Change-Id: I20732ceceb113017a0b1e77134b1e28c4c924fbb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192607 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

…operators. Bug: #44898 Change-Id: If03750edf6bdf13fc2677e0c09f49f70d0da744e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192608 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

Bug: #44898 Change-Id: Ia8f3a5ad2971dfb6baeefb0d2f9c6998af0d897e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192611 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

In order to implement "why not promoted" functionality for variable declaration initializers, we will need to call checkForInvalidAssignment from the resolver, so that we can pass it failed promotion information. So move it into the ErrorDetectionHelpers mixin, along with _checkForAssignableExpression (which it calls). _checkForAssignableExpressionAtType can now be private, since all of its callers are now in ErrorDetectionHelpers. Bug: #44898 Change-Id: I6655e3a655d330d61459692804854c89c0dcff70 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192612 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

…Verifier. In order to implement "why not promoted" functionality for constructor field initializers, we will need to call checkForFieldInitializerNotAssignable from the resolver, so that we can pass it failed promotion information. So move it into the ErrorDetectionHelpers mixin. Bug: #44898 Change-Id: I95094e198a97608bf27f84cfb3cac40824a30ba4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192723 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

This CL handles ordinary assignment expressions, variable declaration initializers, and constructor field initializers. Bug: #44898 Change-Id: I06bae2c7d57213bd7769c931a03080d148af3dbe Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192724 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

We used to have some variables called `whyNotPromoted` and others called `whyNotPromotedInfo`, with no obvious distinction between the two names. Now we use `whyNotPromoted` for variables holding the map returned by `FlowAnalysis.whyNotPromoted`, and `whyNotPromotedList` for variables holding a list of such maps. Bug: #44898 Change-Id: I3823d16ef908a833e4d4526ae1c3576c73ecf1fb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193083 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

…ments. Bug: #44898 Change-Id: I57a2c62b8450c313887b516d6fe2ef85fdd0d92b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193084 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

In order to implement "why not promoted" functionality index expressions, we need to call checkIndexExpressionIndex from the resolver, so that we can pass it failed promotion information. So move it into the ErrorDetectionHelpers mixin. Bug: #44898 Change-Id: Idc70e3421142da1d3100c288c912b6483a91b28c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193087 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

Bug: #44898 Change-Id: I52a0fa33dbeb4f6382dd8b3154ead25ca2065878 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193088 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

…ll_error Previously, I only did the proper flow analysis integration in the case where `useNewMethodInvocationEncoding` was true, so these cases didn't work when it was false. Bug: #44898 Change-Id: I6e126f4b8dc9a159087669c3d6ffa332373f8ce4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193590 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>

This is clearer because: - The use of "could not be promoted" is consistent with the other "why not promoted" messages. - We avoid speculating about whether the variable might be assigned a null value; this should help avoid confusion in cases where the user can see that assigning a null value is impossible, but promotion still fails. - We avoid the phrase "intervening write", which is a little too erudite. Bug: #44898 Change-Id: I0b8a2132dc99dd06769677f98e6257e9b55ad9d6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193820 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Bob Nystrom <rnystrom@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: #44898 Change-Id: I24b47be8eac2e276cd291a5b2f2e4444c911138f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193837 Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

This is possible now that the test runner uses the analyzer's new JSON output format. Bug: #44898 Change-Id: Ib38e52e0c6a8f73074ef702b8a8b18bd9b128743 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194014 Reviewed-by: Bob Nystrom <rnystrom@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

These targets are needed for "why not promoted" error messages, and it's easier to have flow analysis keep track of them than to have the analyzer and CFE try to reconstruct them at the time of reporting the error. Bug: #44898 Change-Id: Ia8ef4a7ce13cc30860e59b7369e6230d233e252d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193832 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

This reverts commit bc6cdf5. Reason for revert: Broke Golem benchmarks - https://golem.corp.goog/PerformanceChanges?repository=dart&revision=91561 Original change's description: > Flow analysis: track property get targets. > > These targets are needed for "why not promoted" error messages, and > it's easier to have flow analysis keep track of them than to have the > analyzer and CFE try to reconstruct them at the time of reporting the > error. > > Bug: #44898 > Change-Id: Ia8ef4a7ce13cc30860e59b7369e6230d233e252d > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193832 > Commit-Queue: Paul Berry <paulberry@google.com> > Reviewed-by: Johnni Winther <johnniwinther@google.com> > Reviewed-by: Konstantin Shcheglov <scheglov@google.com> TBR=paulberry@google.com,scheglov@google.com,johnniwinther@google.com Change-Id: Ic2b66b1db621c0f4e4c5398acfe2ae61bd7625ca No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: #44898 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194104 Reviewed-by: Paul Berry <paulberry@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

This is a reland of bc6cdf5 Original change's description: > Flow analysis: track property get targets. > > These targets are needed for "why not promoted" error messages, and > it's easier to have flow analysis keep track of them than to have the > analyzer and CFE try to reconstruct them at the time of reporting the > error. > > Bug: #44898 > Change-Id: Ia8ef4a7ce13cc30860e59b7369e6230d233e252d > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193832 > Commit-Queue: Paul Berry <paulberry@google.com> > Reviewed-by: Johnni Winther <johnniwinther@google.com> > Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Bug: #44898 Change-Id: Ib5e6a0eafb381f3a9c8770c6c1c3e55581e44b01 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194105 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Reviewed-by: William Hesse <whesse@google.com>

Bug: #44898 Change-Id: I40c6087a0db2c67106f9ce138b79a8a5769b37de Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193834 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>

Bug: #44898 Change-Id: Iea9300cf2cae52355bfa5a71f773c8136da2c644 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195306 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

In a previous commit, I accidentally named these test functions `test`. Replace with a more descriptive name. Bug: #44898 Change-Id: I662b6003a893dbb05fea88000f39e4e56b0cb725 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196107 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

These test cases already work properly, but they weren't previously covered by tests. Bug: #44898 Change-Id: I4fc6506230af203a361631afc542e0db08bd6f27 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196106 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>

… target. Bug: #44898 Change-Id: If464a54bdb63fc661db312df5dc10f108049286a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196240 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>

stereotype441 · 2023-04-13T17:36:04Z

Unassigning myself because I’m not actively working on this feature right now.

stereotype441 added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Feb 9, 2021

stereotype441 self-assigned this Feb 9, 2021

stereotype441 mentioned this issue Feb 9, 2021

Meta-issue for "why not promoted" functionality #44897

Open

15 tasks

scheglov added the P2 A bug or feature request we're likely to work on label Feb 9, 2021

stereotype441 mentioned this issue Mar 3, 2021

Front end produces bogus "Expected ','" message for type mismatch in spread collection #45174

Closed

stereotype441 removed their assignment Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full implementation of core "why not promoted" algorithm #44898

Full implementation of core "why not promoted" algorithm #44898

stereotype441 commented Feb 9, 2021

stereotype441 commented Apr 13, 2023

Full implementation of core "why not promoted" algorithm #44898

Full implementation of core "why not promoted" algorithm #44898

Comments

stereotype441 commented Feb 9, 2021

stereotype441 commented Apr 13, 2023