test.py error tests should support context messages from the CFE

[stereotype441]

(Parent issue #44897)

As of a42244f, I've begun implementing "why not promoted" functionality in the CFE. For example, this code:

f(int? i, int? j) {
  if (i == null) return;
  i = j;
  i.isEven;
}
main() {
  f(null, null);
}

leads to the following output:

../../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;
  ^

I tried to write a language test to validate this functionality (see https://dart-review.googlesource.com/c/sdk/+/182901) but test.py currently only supports testing the main error message; it ignores the "Context:" messages entirely. We need a way to test the context messages as well.

(Eventually we'll want to extend these tests to support analyzer as well, but some analyzer work needs to be done first, to get the context messages exposed in a machine readable way, so it makes sense to start with CFE support).

[stereotype441]

@munificent any progress on this?

[munificent]

I made some progress today. I'm trying to figure out:

1. Whether we should track context for all static error tests or just some that opt in. I think if we want to add context for all tests, there are about 100 that would need updating. I don't mind updating them, but I don't know if it makes the tests noisier than we want.
2. How we want context expectations to look in tests. The simplest solution is to have them just appear like separate errors. I have this working locally:

class C {
  int? x;
  //   ^
  // [cfe] Context: 'x' refers to a property so it couldn't be promoted.

  f() {
    if (x == null) return;
    x.isEven;
//  ^
// [analyzer] COMPILE_TIME_ERROR.UNCHECKED_USE_OF_NULLABLE_VALUE
//    ^
// [cfe] Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
  }
}

Note the // [cfe] Context: 'x' refers... line. This works, but it's not clear which error this context is associated with. If there are multiple static errors in the same test, there's no real way to tell which context is for which error. The nice thing about this format is that if you insert lines or rearrange the test, the context should still be fine. You wouldn't have to manually update explicit line numbers or anything.

Another option is something more like:

class C {
  int? x;
  //   ^
  // [cfe] Context: 'x' refers to a property so it couldn't be promoted.

  f() {
    if (x == null) return;
    x.isEven;
//  ^
// [analyzer] COMPILE_TIME_ERROR.UNCHECKED_USE_OF_NULLABLE_VALUE
//    ^
// [cfe] Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
// [cfe context line 2, column 8] 'x' refers to a property so it couldn't be promoted.
  }
}

I think that's a little clearer, but it means changes to the test will change those line numbers. Of course, the test updater do this for you.

@stereotype441 and @johnniwinther (and anyone else who cares about CFE static error tests), any thoughts or preferences?

[stereotype441]

I prefer the format that doesn't have embedded line/column numbers for maintainability reasons, but I'm a little worried that it will get us into trouble by making the test ambiguous if there's multiple errors. How about an approach where we have numbered identifiers to point to the locations? Something like this:

class C {
  int? x;
  //   ^
  // [cfe @1]

  f() {
    if (x == null) return;
    x.isEven;
//  ^
// [analyzer] COMPILE_TIME_ERROR.UNCHECKED_USE_OF_NULLABLE_VALUE
//    ^
// [cfe] Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
// [cfe context @1] 'x' refers to a property so it couldn't be promoted.
  }
}

That way the @1 serves to tie together the two locations. It wouldn't interfere too badly with automatically updating test expectations, because the updater script could just number consecutively starting at 1.

[munificent]

I like that. I was idly considering something similar, so our convergent evolution here is a signal that it's a good idea. Let me see how hard that is to implement and I'll go from there.

[munificent]

Fixed by: https://dart-review.googlesource.com/c/sdk/+/193281