Make "why not promoted" information more discoverable in command-line analyzer

[stereotype441]

(Parent issue #44897)

As of a42244f73b27256eca44def4daa552e91c2dbc55, the analyzer now generates context messages in some circumstances explaining why type promotion failed. dartanalyzer and dart analyze expose these messages to the user only when the --verbose option is provided, and the format is not easy to read, e.g.:

Analyzing /usr/local/google/home/paulberry/tmp/test.dart...
Loaded analysis options from /usr/local/google/home/paulberry/tmp/analysis_options.yaml
  error • The property 'isEven' can't be unconditionally accessed because the receiver can be 'null'. • /usr/local/google/home/paulberry/tmp/test.dart:4:3 • unchecked_use_of_nullable_value
       Variable 'i' could be null due to a write occurring here. at /usr/local/google/home/paulberry/tmp/test.dart:3:3
       Try making the access conditional (using '?.') or adding a null check to the target ('!').
       https://dart.dev/tools/diagnostic-messages#unchecked_use_of_nullable_value
1 error found.

By contrast, the CFE output looks like this:

../../tmp/test.dart:4:5: Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
Try accessing using ?. instead.
  i.isEven;
    ^^^^^^
../../tmp/test.dart:3:3: Context: Variable 'i' could be null due to a write occurring here.
Try null checking the variable after the write.
  i = j;
  ^

Yesterday we discussed some ideas for how we might improve the presentation of these context messages in the command line analyzer output, such as:

• Using relative file paths rather than absolute ones
• Color coding the error red and the context message green (as CFE does)
• Printing the line of code where the error/context occurs, followed by a line of ^ symbols pointing to a particular location on the line
• Always printing these context messages (not just when --verbose is supplied)

[stereotype441]

I'm not sure who to assign this to. Perhaps @bwilkerson?

[devoncarew]

I believe the above is showing the output for dartanalyzer; we should optimize the output for dart analyze instead.

Paul, do you mind re-running the above example w/ dart analyze? It would be useful to know what our baseline is before we make changes.

[stereotype441]

I believe the above is showing the output for dartanalyzer; we should optimize the output for dart analyze instead.

Paul, do you mind re-running the above example w/ dart analyze? It would be useful to know what our baseline is before we make changes.

As of 86636aff07efb55c2d700a749aad9ca81130f6c2, with --verbose I get:

Analyzing proj...                      0.8s

  error • The property 'isEven' can't be unconditionally accessed because the receiver can be 'null' at test.dart:4:3 • (unchecked_use_of_nullable_value)
          Variable 'i' could be null due to a write occurring here. at /usr/local/google/home/paulberry/tmp/proj/test.dart:3:3
          Try making the access conditional (using '?.') or adding a null check to the target ('!').
          https://dart.dev/tools/diagnostic-messages#unchecked_use_of_nullable_value

1 issue found.

(Note that the path is slightly different from the output I pasted into the bug description; I had to move the test file into a subdirectory to work around #44910).

Without --verbose I get:

Analyzing proj...                      0.8s

  error • The property 'isEven' can't be unconditionally accessed because the receiver can be 'null' at test.dart:4:3 • (unchecked_use_of_nullable_value)
          Try making the access conditional (using '?.') or adding a null check to the target ('!').

1 issue found.

[devoncarew]

As of 86636af, with --verbose I get:

Thanks! The above list of ideas looks good.

Wrt the specific error's text:

Variable 'i' could be null due to a write occurring here. at /usr/local/...

We might want to write the message w/o the use of the word 'here'. The CLI tool can strip off any trailing sentence punctuation (as it does for the regular issue text). If we use relative paths for the location and tweak the error text, we might end up with something like:

Variable 'i' could be null due to a write at test.dart:3:3

Note that the error text is now Variable 'i' could be null due to a write.. We could also say 'due to a preceding write' if we know that was reasonably likely. Or, 'an intervening write'?

[stereotype441]

Wrt the specific error's text:

Variable 'i' could be null due to a write occurring here. at /usr/local/...

We might want to write the message w/o the use of the word 'here'. The CLI tool can strip off any trailing sentence punctuation (as it does for the regular issue text). If we use relative paths for the location and tweak the error text, we might end up with something like:

Variable 'i' could be null due to a write at test.dart:3:3

Note that the error text is now Variable 'i' could be null due to a write.. We could also say 'due to a preceding write' if we know that was reasonably likely. Or, 'an intervening write'?

Good idea, thanks! I'll take responsibility for changing the error message text.

[devoncarew]

https://dart-review.googlesource.com/c/sdk/+/184104 for some of the cleanup mentioned above.

[stereotype441]

@devoncarew As of 7fc9a1a14c1538ff2b2e6d27b0c6d67e1e77481a it looks like both dart analyze and dartanalyzer still require the --verbose flag in order for the "why not promoted" information to appear. Do you think we will be able to enable this by default before the beta cut?

[devoncarew]

Yes, this is likely to land in the next few days -

[stereotype441]

@devoncarew any news?

[devoncarew]

Yes, this did land.

[stereotype441]

@devoncarew it looks like only the behavior of dart analyze was changed; for the dartanalyzer tool, it is still necessary to supply --verbose to see "why not promoted" information.

Can we get both tools changed? It sounds like there's still at least several months before the dartanalyzer tool will be removed, and I'd like to try to ship "why not promoted" functionality before that.

[stereotype441]

@devoncarew any update on this?

[devoncarew]

Hey - I was't planning on adding this to dartanalyzer, just dart analyze. We do hope to deprecate dartanalyzer in the near term (1-2 quarters), other factors permitting.

Can you open a separate issue for dartanalyzer? If you feel strongly about this that would influence whether we add support, but I suspect this is more a completeness thing than a gating factor for the overall feature.

[stereotype441]

We do hope to deprecate dartanalyzer in the near term

Ok, I filed #45546 requesting to backport the support to dartanalyzer.