Test language_2/void_type_usage_test.dart not discriminating closely enough

[eernstg]

The test language_2/void_type_usage_test.dart does not discriminate with sufficient precision, which means that it may seem to succeed by having a compile-time error (for the about 200 subtests with expected outcome //# compile-time error), but the static analysis still may not have detected the intended fault in the source code.

For instance, the subtest param_plus_eq is intended to verify that the following statement is a compile-time error, where x is a variable of type void:

  x += 1;  //# param_plus_eq: compile-time error

This is a compile-time error because the value of the expression x is used in a non-trivial manner (we have about 6 specific situations where such a value can be used, and this is not one of them), but it is also a compile-time error because the type void does not have an operator +, which is required for +=. So it may seem to succeed, but the error may still not be the desired one.

Similarly, the following is a compile-time error because the expression x of type void cannot be evaluated to serve as the condition for a while statement:

  while (x) {};  //# param_while: compile-time error

Given that void is a top type (like Object and dynamic), it should be allowed to have such a condition of type void and implicitly downcast it to bool as needed for while, and the statement should then be OK, except for the fact that it uses the value of x at all. However, the test currently succeeds with dartanalyzer --strong because there is a compile-time error --- but the error says "Conditions must have a static type of 'bool'" rather than something like "The value of an expression of type void can only be used in 6 specific situations listed in generalized-void.md". ;-)

All in all, this means that in a number of subtests, language_2/void_type_usage_test.dart does not currently discriminate between some right and wrong reasons for succeeding with outcome compile-time error, and this might justify a different approach (e.g., something like the "unit tests" used for testing dart2js, where it is possible to check that a specific error is emitted).

[MichaelRFairhurst]

We should probably also test that extends/implements/with void is an error.

I also have tests for some other things; depending on how conclusive we want to be:

void main() {
  void x;
  switch(x) { ...; }
  x();
  (x)();
  x.foo;
  x.foo = null;
  x?.foo;
  list[x];
  list[x] = null;
  x[0];
  x ? null : null;
  await x;

These may be overkill; however; these were cases that I wrote before implementing them in the analyzer, where each has a different path.

[eernstg]

Thanks for the input! We should indeed check those things, they are justified both from a black-box perspective ("this kind of expression exists, we should test it") and a white-box perspective ("if this is omitted, some code in the analyzer never gets tested").

[MichaelRFairhurst]

Also, in void_type_function_types_test, is this a mistake?

line 95:
Expect.isTrue(h2 isH2);
Expect.isTrue(h2 is G2);
Expect.isTrue(h2 isH2);
expectsF2(h2);
expectsG2(h2);
expectsH2(h2);

Looks to me like the first line was supposed to be Expect.isTrue(h2 is H2);. This is repeated with h1, h3, ...

[MichaelRFairhurst]

OK another suggestion regarding either the test or the spec;

Should for(void x in ys) be allowed? Regarding for loops, the spec only says void is allowed:

In the initialization and increment expressions of a for-loop

If I'm not mistaken, void x there is neither?

In then gives an example which is incomplete: "for (e1; e2; e3) {..}, e1 and e3 may have type void."

I'm assuming that this is an error in the informal spec rather than an error in the test (which seems to have very thoroughly tested that for(void x in ...) is allowed).

[eernstg]

Good catch, there's definitely no point in having the same test twice!

I believe the most meaningful fix is to change line 95 to Expect.isTrue(h2 is F2); and line 102 to Expect.isTrue(h3 is F3); (relative to this version of the file).

[eernstg]

Here's the CL which changes lines 95 and 102 as mentioned.

[eernstg]

About for (var x in <void>[]) {} and similar constructs: The discussion is being taken in #30177 (comment) and later comments.

[eernstg]

Cleanup occurs here: https://dart-review.googlesource.com/350921. This CL changes language/void/void_type_usage_test.dart (NB: language_2 does not exist today) to use the new test runner expectation syntax (// ^^^, // [cfe] ..., and // [analyzer] ...). This implies that the actual error message is recorded and compared to the expected one, which eliminates the main reason for creating this issue in the first place.

However, the CL also changes a rather large number of other things. In particular, the old multi-test had several duplicated labels, which means that some tests could fail (due to a missing compile-time error) and it would never be detected, as long as just one of the cases with the duplicated label would gave rise to a compile-time error. This fault has been removed (automatically) because of the switch to the new error expectation syntax, but it does imply that the version of the test which is introduced by this PR will cause the CFE and the analyzer to fail with about 130 missing compile-time errors each.

[eernstg]

Implementation of the missing compile-time errors is handled in #54889.