Skip to content

Expose "why was this type promoted" information through analyzer and analysis server #38773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
stereotype441 opened this issue Oct 8, 2019 · 3 comments
Closed

Expose "why was this type promoted" information through analyzer and analysis server #38773

stereotype441 opened this issue Oct 8, 2019 · 3 comments
Assignees
@stereotype441
Labels
devexp-ux legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Milestone
Future

Comments

@stereotype441
Copy link
Member

Based on a conversation today with @bwilkerson and @jwren, as well as previous conversations with @MichaelRFairhurst and others:

Consider NNBD-enabled code that looks like this:

f(int? x) {
  ...some code involving the expression x! ... // (1)
  ...some other code...
  if (x == null) { // (2)
    ...
  }
}

Flow analysis will determine that since x is null-checked at (1), its type is thereafter promoted from int? to int. Therefore, the test x == null at (2) is guaranteed to evaluate to false, so the analyzer will issue a warning there.

It would be really nice if the user could click on the warning at (2), ask "why was this type promoted?", and be navigated to the null check at (1).

Similar situations exist for other scenarios where types are promoted, such as:

  • explicit "is" check.
  • assignment of a non-nullable value.

We almost certainly want to expose this information to editors via the analysis server. We might even want to expose it to users of the analyzer CLI.

Note that in order to produce this information we'll need to add the ability for flow analysis to track the "cause" of promotions.
@stereotype441 stereotype441 added legacy-area-analyzer Use area-devexp instead. type-enhancement A request for a change that isn't a bug NNBD Issues related to NNBD Release labels Oct 8, 2019
@stereotype441 stereotype441 added this to the Future milestone Oct 8, 2019
@stereotype441 stereotype441 mentioned this issue Oct 8, 2019
Closed
@stereotype441 stereotype441 mentioned this issue Jun 18, 2020
Closed
@MichaelRFairhurst MichaelRFairhurst mentioned this issue Jun 19, 2020
Closed
@devoncarew devoncarew added the P2 A bug or feature request we're likely to work on label Oct 29, 2020
@stereotype441 stereotype441 mentioned this issue Dec 16, 2020
Closed
@stereotype441
Copy link
Member Author

I'm going to experiment with this over the next week or two and see what I can come up with

@stereotype441 stereotype441 self-assigned this Dec 21, 2020
dart-bot pushed a commit that referenced this issue Jan 15, 2021
@stereotype441
Add a class to represent references tracked by flow analysis.
f452e76 
The only references that are promotable are references to variables,
however in the near future I'll be adding logic to flor analysis to
keep track of attempts to promote fields; this will require tracking
references to property gets as well as references to `this`, so we
need to start building up data structures to track those references.

Bug: #38773
Change-Id: I0e3fd44f580bb85db3e8f405675b66aa4069e455
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/179280
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
dart-bot pushed a commit that referenced this issue Jan 28, 2021
@stereotype441
Expose _flowAnalysis from ResolverVisitor.
d35cb90 
This saves a lot of plumbing work in the analyzer, because it allows
the individual resolver components to access flow analysis via the
resolver field rather than needing to maintain their own field.  It
should be helpful in supporting the upcoming "why not promoted"
functionality (#38773), which will require flow analysis to be visible
to even more parts of the resolver.

Bug: #38773
Change-Id: Ibea0ec7e263461d6c512b3dad59f10622b2aac6f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/181661
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
dart-bot pushed a commit that referenced this issue Feb 2, 2021
@stereotype441
Flow analysis: begin tracking non-promotion reasons.
a42244f 
This CL implements the core flow analysis infrastructure for tracking
reasons why an expression was not promoted.  It supports the following
reasons:

- Expression was a property access
- Expression has been written to since it was promoted

I expect to add support for other non-promotion reasons in the future,
for example:

- `this` cannot be promoted
- Expression has been write captured
- Expression was a reference to a static field or top level variable

These non-promotion reasons are plumbed through to the CFE and
analyzer for the purpose of making errors easier for the user to
understand.  For example, given the following code:

  class C {
    int? i;
    f() {
      if (i == null) return;
      print(i.isEven);
    }
  }

The front end now prints:

  ../../tmp/test.dart:5:13: Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
  Try accessing using ?. instead.
      print(i.isEven);
              ^^^^^^
  Context: 'i' refers to a property so it could not be promoted.

Much work still needs to be done to round out this feature, for example:

- Currently the analyzer only shows the new "why not promoted"
  messages when the "--verbose" flag is specified; this means the
  feature is unlikely to be noticed by users.

- Currently the analyzer doesn't show a "why not promoted" message
  when the non-promotion reason is that the expression is a property
  access.

- We need one or more web pages explaining non-promotion reasons in
  more detail so that the error messages can contain pointers to them.

- The analyzer and front end currently only show non-promotion reasons
  for expressions of the form `x.y` where `x` fails to be promoted to
  non-nullable.  There are many other scenarios that should be
  handled.

Change-Id: I0a12df74d0fc6274dfb3cb555abea81a75884231
Bug: #38773
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/181741
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
@stereotype441
Copy link
Member Author

Additional work on this feature will be tracked via #44897.

@stereotype441
Copy link
Member Author

Duplicate of #44897.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stereotype441 stereotype441

Labels
devexp-ux legacy-area-analyzer Use area-devexp instead. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
Future
Development

No branches or pull requests
3 participants
@stereotype441 @devoncarew @bwilkerson