dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Analyzer: Support generalized void #30177

Closed eernstg closed 5 years ago

eernstg commented 7 years ago

This is the analyzer specific issue for #30176, which has the details.

eernstg commented 7 years ago

This CL introduces support for the syntactic extensions in the analyzer, but does not yet check for usages of values of static type void. So, apart from the usual subtype checks, they are still allowed everywhere (but it should be a compile-time error to use such a value in most situations, e.g., print(v) and var y = v; should be compile-time errors when v is declared as void v = null; ).

eernstg commented 7 years ago

The above-mentioned change was landed on July 27th as 9336e199fd7360ee229de5b4ebef5940aab266ec. This issue will be closed when the static checks have been implemented as well.

bwilkerson commented 6 years ago

@eernstg Does the informal spec (https://github.com/dart-lang/sdk/blob/master/docs/language/informal/generalized-void.md) include descriptions of all of the places where there ought to be an error?

eernstg commented 6 years ago

(I'm pretty sure I answered this question several days ago, but I can't find the answer anywhere, so here we go again ;-).

Yes, the informal spec of generalized void is intended to contain a specification of all contexts where it is an error to evaluate an expression of type void. However, it's specified in a negated manner, as a list of cases where it's OK, and all other contexts are then the ones where an error should occur (because those other situations are the vast majority).

The whitelist can be found in the informal spec by searching for situations:.

The only other error is concerned with overriding; search for overrides to find that.

Note that there will be more errors in the future, when we introduce the notion of 'voidness preservation'. E.g., with voidness preservation we will almost certainly (it is not yet fully specified) have errors in the following situations:

List<Object> xs = <void>[]; // Voidness preservation violation.

void foo() {}
Object Function() f = foo; // Voidness preservation violation.

The general idea is that we will prevent some indirect patterns where voidness is "forgotten".

But this is in the future, so it's currently only required to raise errors in the cases which are listed in the document.

devoncarew commented 6 years ago

/cc @MichaelRFairhurst

MichaelRFairhurst commented 6 years ago

Given that this was reopened with the intention of supporting dart 2, that seems to indicate to me based on the informal spec that what we actually want to implement now is "voidness preservation."

Some other changes:

the type void is considered to be equivalent to the built-in class Object in Dart 1.x, except when used as a return type, in which case it is effectively considered to be a proper supertype of Object. In Dart 2 subtyping, the type void is consistently considered to be equivalent to the built-in class Object.

Also note that we may not allow return statements returning an expression of type void in Dart 2, but it is allowed here for backward compatibility.

In Dart 2, it is a compile-time error when a method declaration D2 with return type void overrides a method declaration D1 whose return type is not void.

Also this:

direct usage of the value of an expression of type void is a static warning (in Dart 2: an error)

Crucially, it looks like we're missing this:

this document does not specify voidness preservation

and an anwer to whether return statements will still be allowed

MichaelRFairhurst commented 6 years ago

Ah, I see that I misunderstood. This ticket was never closed and reopened. I misread this:

@bwilkerson bwilkerson referenced this issue 22 days ago
 Closed
Support generalized void in the analyzer #31636

I suppose that means, though, that we have two tasks here...to support generalized void checks at all, and then to support dart-2.0 generalized void checks.

leafpetersen commented 6 years ago

The voidness preservation is not part of this. In order of priorities:

1) Allow void to be used syntactically as a type argument 2) Make sure subtyping/least upper bound work with types containing void 3) Make sure inference does the right thing 4) Add the limited syntactic checks specified in the informal proposal (not voidness preservation)

The first two I think are required to unblock https://github.com/dart-lang/sdk/issues/31869

eernstg commented 6 years ago

Right, I added support to the legacy front end of the analyzer for the generalized void syntax (cf. 1) in July, and generalized_void_syntax_test.dart succeeds now, so that as apparently working.

For subtyping (cf. 2), void is a top type, and that's clearly not the way it is treated. E.g., this causes a compile-time error in main of void_type_usage_test.dart, in the subtest none (that is, code which is always present), that shouldn't be there: "The argument type 'int' can't be assigned to the parameter type 'void'" --- yes it can! So that needs to be fixed.

(cf. 4) void_type_usage_test.dart serves to test that a lot of usages of expressions of type void are rejected as compile-time errors, or that they are accepted. However, that test fails in confusing ways. In particular, a number of test cases incur a compile-time error as expected, but because of the compile-time error in the code which is always present the intended compile-time error may or may not occur (and in some cases it certainly doesn't). Similarly, the always-present compile-time error makes all cases fail that have an ok expected outcome. In summary, the status file looks like this test is running almost successfully, but it's actually failing all over the place.

MichaelRFairhurst commented 6 years ago

Thanks for the info everyone. I think I have everything I need!

That said, I'll be taking a quick break on this today to get ready for dartconf. Hopefully its not an all-day break, but, so far the stuff that was supposed to be quick and easy there hasn't been!

eernstg commented 6 years ago

Please note the issues described in #31883: It explains why void_type_usage_test.dart may not easily be improved such that it avoids reporting a successful outcome even though we "got the wrong error". There are lots of other subtests in void_type_usage_test.dart, so I've marked it as P3, but you may wish to check manually that you actually get a "can't evaluate an expression of type void here" error rather than some other error, when implementing this feature.

MichaelRFairhurst commented 6 years ago

@leafpetersen feel free to take this on if you have time; if you do then just let me know. I'll do the same, if my dartconf work wraps up quickly.

Like you said, should be not much more work than adding void to _isTop(). Haven't verified though.

And I'll fininsh up the GLB of the top types now so that both of us would have that working once we get to this.

MichaelRFairhurst commented 6 years ago

I also ran the quick test of supporting _isTop to include void. I'm seeing 6 failures in the dart analyzer test cases, which is really not many, so it should be easy to tackle this overall unless, say, the language tests have more failures or something.

MichaelRFairhurst commented 6 years ago

I also noticed some discrepancies between the internal spec and the language test:

void testVoidParam(void x) {      
  ...                
  return x;   //# param_return: compile-time error 

and the spec:

It is a static warning for an expression to have type void (in Dart 2: a compile-time error), except for the following situations:
...
In a return statement return e;, when the return type of the innermost enclosing function is the type void, e may have type void.

I'm assuming we should go with err on returning void values for now, that the language test here is what we want to go with rather than the informal spec.

eernstg commented 6 years ago

@MichaelRFairhurst Good catch! This CL makes corrections to the test such that all the existing return statements are compile-time errors as specified (because the return types of test.. functions are now dynamic rather than void), and a new test function has been added to test that return-void-in-void-function (e.g., void foo(void f()) { return f(); }) is ok.

This particular exception was controversial, but we have it because of the breakage that would be introduced by prohibiting return-void-in-void-function, which has been expressible and allowed also in Dart 1. Also, it has been argued that it is convenient to be able to write write return f(); rather than f(); return;, especially in contexts like if (..) return f();, even when f returns void, as long as it is in a void function.

Edit: Just saw 'I'm assuming we should go..': No, we should adjust the test to follow the feature spec, the return-void-in-void-function exception should not be made an error, especially not temporarily.

eernstg commented 6 years ago

Said CL has been landed.

Note that the resulting void_type_usage_test.dart looks a lot like it has dead code (e.g., the new function testReturnToVoid has many consecutive return statements), but since this is a multi-test we won't actually have any dead code in each test. So there is no need to "fix that".

MichaelRFairhurst commented 6 years ago

Thanks Erik for the quick clarification!

MichaelRFairhurst commented 6 years ago

Also, is it really desirable that as on void is ok, and is isn't?

People will have to write

  void f() {
    final v = voidMethod();
    try {
      final secretlyReturnedVal = v as ReturnedVal;
      ...
    } on ..... {
      ...
   }

instead of

 void f() {
   final v = voidMethod();
   if (v is ReturnedVal) {
     ...
   } else {
     ...
   }
 }

My initial impression is that this is boilerplate, and perf hit, for no value. So, is this a slip-up, or is there value in making people go through these hoops?

MichaelRFairhurst commented 6 years ago

I guess I just realized; the examples have errors, right? The assignment doesn't fall into the case of where void may be used.

So I guess the reason we only allow as is that the as version can be rewritten to not use a temporary ie

void f() {
  try {
    final secretlyReturnedVal = voidMethod() as ReturnedVal;
    ...
  } on ..... {
    ...
  }

whereas the equivalent is not possible with is unless we allow void values to be assigned to void variables, and/or the user relies on the method being idempotent

  void f() {
    if(voidMethod() is ReturnedVal) {
      v = voidMethod as ReturnedVal; // better be idempotent!
    ...

Hmm. Well, nobody said designing a language was easy. Though this escape hatch definitely feels much more like an escape hatch than a feature, in this case.

leafpetersen commented 6 years ago

Also, is it really desirable that as on void is ok, and is isn't?

Erik can comment definitively, but I believe the answer is that you really shouldn't be looking at void values to begin with - you're in some sense violating the implicit contract ("don't use the return value, it's arbitrary, and can change"). We allow it, but I don't think we need to make it easy. If you really want to test the result, you can do (voidMethod() as dynamic) is ReturnedVal with presumably no perf hit.

MichaelRFairhurst commented 6 years ago

voidMethod() as dynamic ah yes, use a different top type to get out of voidness semantics. Nice.

MichaelRFairhurst commented 6 years ago

This is just about ready for review, but I ran into issues within ddc that I'm not sure are very "safe."

void myFun() {}

myOtherFun() => myFun();

results in 'the return type void isn't a dynamic, as defined by myFun'.

This begs the question; does this part of the spec have to be done now?

In Dart 2, it is a compile-time error when a method declaration D2 with return type void overrides a method declaration D1 whose return type is not void.

Though, the problem may be in the inference rather than in this rule.

leafpetersen commented 6 years ago

Can you elaborate the example a bit? Bleeding edge DDC compiles and runs this program just fine:

void myFun() { print("hello");}

myOtherFun() => myFun();

void main() {
  myOtherFun();
}

I'm not sure I understand the reference to the override error.

MichaelRFairhurst commented 6 years ago

for full info, this is the current state of the CL: https://dart-review.googlesource.com/c/sdk/+/35002

MichaelRFairhurst commented 6 years ago

After the void changes, that is no longer valid and it requires an explicit "void" return type annotation on "myOtherFun".

I don't think I touched inference in any ways that affect that, though I will double check.

The only other failing code that I saw was cases where it indeed was using a void value, which previously was only a hint, in package:expect. In that case I used the as dynamic escape hatch. If I had changed the type of the corresponding typedef to () -> dynamic from () -> void, then I was worried that occurrences of specifying that function would have problems overriding the dynamic return type to be void, as in the DDC case at inference time.

leafpetersen commented 6 years ago

but I ran into issues within ddc that I'm not sure are very "safe."

Maybe I'm misunderstanding. Are you saying that you ran into issues with the DDC source code, or with running DDC tests, or... ?

I made some comments on the CL.

MichaelRFairhurst commented 6 years ago

Are you saying that you ran into issues with the DDC source code

I wasn't sure! I think now, the answer is no.

So not only does the example I gave (myOtherFun() => myVoidFun()) currently work on bleeding edge, but it should work after the void changes are landed, right?

On my CL, it complains about this case: "the return type void isn't a dynamic, as defined by myFun". The first case of this I saw was in DDC, but its definitely more than just DDC. Seeing lots and lots of language tests that don't like this change, so I'm assuming its a bug in my CL.

MichaelRFairhurst commented 6 years ago

Ah, this is way simpler than I realized.

This rule is too strict, at least for now:

In a return statement return e;, when the return type of the innermost enclosing function is the type void, e may have type void.

In the case provided, myOtherFun is typed as () -> dynamic. There is no type inference here. Correct?

Unfortunately, that means it can't => return a void result, such as myVoidFun().

So we can either special case arrows, or dynamic/void, or maybe there is some inference here that I don't know about (or should be).

MichaelRFairhurst commented 6 years ago

Or fill in the type ie void myOtherFun() => myVoidFun() in all cases, which appears to be a lot.

leafpetersen commented 6 years ago

So if I understand you correctly: the code that has these errors (that is, the code being analyzed) is in language tests?

I think @eernstg and @lrhn should comment on whether there is an intended exception for either dynamic contexts or for => functions, but as I read the proposed generalized void rules, that code should be an error.

Is it possible to land this incrementally? In the priority list that I gave above, I believe that these errors fall under 4.

MichaelRFairhurst commented 6 years ago

Is it possible to land this incrementally? In the priority list that I gave above, I believe that these errors fall under 4.

Yes, incrementally seems like the right call here.

MichaelRFairhurst commented 6 years ago

The errors in the language tests are here: https://logs.chromium.org/v/?s=dart%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8957199686594153152%2F%2B%2Fsteps%2Fanalyze_tests_checked%2F0%2Fstdout, many or most of them are this specific "returning null in a dynamic function" problem.

I will ask Keerti to check this against google3, too, see if its only the tests that are affected or if its a common pattern/mistak.

eernstg commented 6 years ago

I'll respond to a few things that I noted in the discussion above; I'm not 100% sure which things have been resolved already (or how ;-), so bear with me if there's a little bit of redundancy.

Also, is it really desirable that as on void is ok, and is isn't?

Both on void and is void are errors (they're actually syntax errors according to Dart.g).

final v = voidMethod();

That would be an error (assuming that voidMethod has return type void) because it evaluates an expression of type void in a context that is not one of the six exceptions. But you could "step out of voidness" immediately with final v = voidMethod() as ReturnedVal;, or you could allow for a broader range of values with final v = voidMethod() as Object; and then later use v is T for various T as needed. Note that as Object can be a no-op at run time, so there is no performance hit; and v is ReturnedVal is still in your "instead of" example, so that cost would be paid anyway.

In any case, casting-out-of-void is intended to be a very, very special case. You can do it if you're in trouble (because of legacy code in a third party library or whatnot), but in general it shouldn't count because it doesn't happen.

The next comment has this:

void f() {
  if(voidMethod() is ReturnedVal) {
    v = voidMethod() as ReturnedVal; // better be idempotent!
    ...

In this case you can't use promotion (because promotion only applies to variables, not to non-trivial expressions like function calls), but you can cast out of void in one step, and then promote:

void f() {
  var v = voidMethod() as Object;
  if(v is ReturnedVal) {
    // `v` has type `ReturnedVal` here, and evaluation happened only once.
    ...

Again, it doesn't matter so much if this is slightly verbose and inconvenient, because it just shouldn't happen. It's an emergency-only feature to cast out of void.

void myFun() {}
myOtherFun() => myFun(); // (*)

The line marked (*) is a compile-time error because the expression myFun() is evaluated and has type void, and the situation is not one of the six exceptions where that is allowed. I can see that there was some doubt on this:

it requires an explicit "void" return type annotation on "myOtherFun". ... [but it] should work after the void changes are landed, right?

No, void myOtherFun() => myFun(); would be OK because of the special exception for void .. => functions (and it's very similar to exception 6), but myOtherFun() => myFun(); is an error.

lrhn commented 6 years ago

but myOtherFun() => myFun(); is an error.

It is only an error if we don't infer void as the return type, which we should since the body expression has type void. We cannot talk about what the return type of a function is until its return type has been inferred, which is why type inference must be one of the first things to happen.

I have one rule that I insist on: It shouldn't make any difference whether a type is inferred or written explicitly. (Or, more precisely, if you take a program, infer types, and then insert all the inferred types explicitly, then the program should do exactly the same). So, I don't want a rule that differs whether void is written or inferred as return type.

eernstg commented 6 years ago

Right, I was assuming that myOtherFun and myFun are library functions (top level), in which case I do not think any inference would be applicable to fill in missing type annotations in the signature. So myOtherFun would have return type dynamic.

For a function literal we could infer the return type using the type-from-context, and for an instance method we could obtain the return type from a declaration in a superinterface of the enclosing class. Granted, for a situation where a function literal is used in a context that provides no input on the desired return type, we would use the return expression, and that would make () => myFun() an expression of type void Function(), with no errors.

In any case, it's a separate issue how the return type for myOtherFun() => .. is computed, and if the return type is dynamic the body => myFun() is an error.

MichaelRFairhurst commented 6 years ago

It seems to me we may want to allow

x ? voidValue : voidValue;

I only saw this once, but it seems to me that the standard rules would then apply -- the ternary would have a void type due to LUB even if only one of the cases results in a void type. It would then be an error to pass that into functions/assign its result, etc etc.

Makes it legal to do like

void increment() ...
void throwException() ...
void foo() => isLocked() ? throwException() : increment();
eernstg commented 6 years ago

The language team discussed allowing boolValue ? e1 : e2 where e1 or e2 (or both) have static type void (and the conditional expression as a whole would then also have static type void, which would prevent it from occurring as a subexpression of all sorts of "normal" expressions). But after a (loud ;-) discussion the majority decided against it. The main point was that we wanted to keep the set of locations where a void expression can occur as minimal as possible while still allowing it in the locations where the value is directly discarded, plus the cast for emergencies. With ?: we would go beyond this rationale because there is an implied computational step whereby "the void-typed value is passed through the branching point of the conditional operator". So it lets "the void value" take one step into compute-land, and we only wanted to allow it to live in discard-land. ;)

MichaelRFairhurst commented 6 years ago

Should these no longer be compile time errors?

void badReturnTypeAsync() async {} // //# 01: compile-time error                 
void badReturnTypeAsyncStar() async* {} // //# 02: compile-time error            
void badReturnTypeSyncStar() sync* {} // //# 03: compile-time error 

I can see both ways. Its sound, after all. And on the one hand, it essentially lets void do this:

  void myVoidAsyncFun() => myAsyncFun();
  Future<void> myAsyncFun() async { ... }

in a single keyword. On the other hand, the above code is a very intentional way of disallowing api consumers of doing a very normal thing. Maybe its good that that's not easy to do on accident.

This should probably at the very least get paired with a lint "async_functions_prefer_future_void_to_void" which suggests "did you mean to return Future instead of void from your async function so that its awaitable?"

MichaelRFairhurst commented 6 years ago

Another problem here, this time in dart2js, which may back up that ternaries shouldn't be an error:

  void assertCondition(bool condition, Object arg) => _isConsoleDefined          
      ? JS('void', 'console.assertCondition(#, #)', condition, arg)              
      : null;

If I'm not mistaken (and I don't know much about the inference, so this could be way off), JS is of type <T>([...]) -> T, and in this case T is inferred as void, giving the whole JS invocation a resulting type of void which is then not allowed to be the second operand of the ternary operator, so this fails the DDC build, unless you specify JS<dynamic>(....) here, which seems frivolous.

MichaelRFairhurst commented 6 years ago

Only just noticed that Erik had responded to assert that x ? voidVal : voidVal was explicitly decided against.

I guess in this case, this code in dart2js should be flagged. There's nothing in my CL that suddenly causes it to infer void. Its just that before it was not flagged for being an operand of the ternary.

A lot more breaking changes in here than I expected; coordinating this will be tricky. I will likely have to start with Hints to ease the transition.

I suppose I could try writing a dartwing migration! @mk13 a lot in here, but here are the major issues I'm seeing for migration purposes:

Dartwing should be able to do all three of these things pretty straightforwardly, right?

Trickiest thing I can see in the above is that there's some overlap (technically we could sove foo() => voidValue; be changing it to foo => voidValue as Object, but we shouldn't), and handling cond ? voidValue : voidValue in nested cases....which we could fix by cond ? voidValue as Object : voidValue as Object, but that assumes that we want to suggest such hacky code to people in the first place...

MichaelRFairhurst commented 6 years ago

@leafpetersen @eernstg is it a good idea to land this change as simply exposing void as Top, maybe with hints for where its used?

Upsides:

Downsides:

The two downsides do mean that while some teams will be able to slowly clean up their code, others will be vulnerable to a slip in code quality, soundness, and/or productivity.

Perhaps if the migration on the whole is quick, unblocking others makes this a net win. But the sidecast issue is a huge one.

leafpetersen commented 6 years ago

I'm concerned about biting off another migration here, and I'm also concerned that if we're seeing a lot of uses of a particular code pattern, that might be a sign that we're not on the right track by disallowing it. For example, perhaps we should extend the exception on returning void things to dynamic as well? And if a lot of users are using conditional expressions returning void, then maybe that's a sign that it's a useful pattern?

I think the highest priority is making void properly useful as a top type. I would suggest landing that as soon as possible. If you also prepare a CL that issues the specified warnings, then I (or you) can test it on internal code to better understand the magnitude of the migration, and what kind of code patterns we would be outlawing.

lrhn commented 6 years ago

I'm sure that there are uses of the conditional operator as a short-hand for an if statement. I just didn't want to encourage that. It's hard to read - using an expression means that the reader will expect it to return a value, and they have to study it closely to realize that it's just "returning void". (That's also my opposition to allowing void-arrow functions).

If if statements are too cumbersome to write, or if braces are too cumbersome to write, we can improve the syntax. We shouldn't encourage using an expression where a statement is intended, that moves the burden from the person writing (not so many) characters to the person who has to read the underlying concepts from the syntax.

That is, I'm very, very happy making => condition ? voidExpression() : null invalid. It's much more readable as:

void assertCondition(bool condition, Object arg) { 
  if (_isConsoleDefined) JS('void', 'console.assertCondition(#, #)', condition, arg);
}

Checking against internal code, I don't see many cases of void .. => .. ? ... (regexp void.*=>.*[^?]\?[^.?]). It won't find the multi-line cases, but there were only three of these simple cases, so I believe the problem to me manageable.

As for allowing void as return type on async, async* and sync* functions: I definitely want it for async, but it makes less sense for async* and sync* because the function does nothing if you don't use the returned value, so it's most likely an error.

(I'm all for a gentle migration experience, as long as we end up being fully migrated.)

eernstg commented 6 years ago

Should these no longer be compile-time errors?

void badReturnTypeAsync() async {} // //# 01: compile-time error
void badReturnTypeAsyncStar() async* {} // //# 02: compile-time error
void badReturnTypeSyncStar() sync* {} // //# 03: compile-time error

The feature spec of generalized void does not address this case.

The language specification (dartLangSpec.tex) has not been updated with respect to the generalization of void, which means that some sections will need to be changed a lot (e.g, \ref{typeVoid}).

Still, the following verbiage in the language specification could be relevant, and might survive (with warnings changed to errors):

It is a static warning if the declared return type of a function marked \ASYNC{}
  is not a supertype of \code{Future<\mbox{$T$}>} for some type $T$.
It is a static warning if the declared return type of a function marked \SYNC*
  is not a supertype of \code{Iterable<\mbox{$T$}>} for some type $T$.
It is a static warning if the declared return type of a function marked \ASYNC*
  is not a supertype of \code{Stream<\mbox{$T$}>} for some type $T$.

But void is now a top type, so the given declarations are no longer compile-time errors (T can be an arbitrary type and it will always will work).

Unless we specifically decide that they should be errors, and change the spec accordingly.

If the bodies are not empty, the following may also be relevant:

It is a static type warning if the body of $f$ is marked \ASYNC{} and the
type \code{Future<$flatten(T)$>} (\ref{functionExpressions}) may not be
assigned to the declared return type of $f$.
Otherwise, it is a static type warning if $T$ may not be assigned to the declared return type of $f$.
...
It is a static type warning if either:
\begin{itemize}
\item the body of $f$ is marked \ASYNC* and the type \code{Stream<T>}
  may not be assigned to the declared return type of $f$.
\item the body of $f$ is marked \SYNC* and the type \code{Iterable<T>}
  may not be assigned to the declared return type of $f$.
\end{itemize}

Here, the type T is the static type of the expression e in a return statement return e; which is under scrutiny. Given that we have no special rule against assignments to a void typed variable (we can do void x = 42 now), and given that void is a top type, this would be interpreted to mean that those declarations are no longer compile-time errors.

It is again a consistent choice to allow these kinds of functions.

However, there is a "moral" equivalent here which says the opposite. I included the sentence Otherwise, .. above, because it's the place where it is specified that it is an error to do return e; in a "normal" function (not async etc.), unless the type of e is assignable to the return type. So we might think that these declarations would be compile-time errors if something like the following declarations are compile-time errors:

void badReturnTypeAsync() {
  return new Future.value(null);
}

void badReturnTypeAsyncStar() {
  return new Stream.empty();
}

void badReturnTypeSyncStar() {
  return new [];
}

These declarations have been flagged with a static type warning in Dart 1, based on the criterion that the returned expression should have a type which is assignable to the return type, and in Dart 1 nothing other than dynamic, void, and bottom is assignable to void.

In Dart 2, everything is assignable to void, but in the language team we have had many strong expressions of a desire to ensure that return e; will remain an error in a function with return type void when e has a static type different from void.

The argument has always been that this is error prone: The developer wishes to return something (presumably something meaningful), but far away there is a declaration of a void return type which declares to the world that the returned value should be discarded.

There is no support in the feature spec for these errors, and we've seen that the current wording in the language specification will make it a non-error, but I suspect that the language team will be quite happy about deciding this.

So here's a CL on the feature spec to consider, Lasse and Leaf.

eernstg commented 6 years ago

As for allowing void as return type on async, async and sync functions: I definitely want it for async, ..

@lrhn, why was that? The CL mentioned above makes it an error because such a function would semantically be like a regular function with return type void doing return aFuture.

If the point is that some async functions ought not to be awaited ("it is never interesting to the caller when this task has been completed") then I can understand the reason, but it seems rather error prone (I suppose we will have bugs like "I wanted to express that the returned value isn't interesting, so I used void f() async ..", but it should have been Future<void> f() async ..).

Is it really so useful to be able to say "you must avoid detecting when this is done"? (One would have to do await (foo() as Future); in order to detect it, so anyone trying to await foo() would get the message that they can't do this).

eernstg commented 6 years ago

If we end up adding exceptions such that b ? e1 : e2 are allowed even when e1 or e2 has type void (and then giving the whole expression the type void, such that we can't, say, put it into bigger expressions like foo(true ? print('Hello, voidness!') : null)), here's a CL which adds support for that.

I'm not advocating that we should make this decision, but if there's a substantial migration cost then I'm not strongly against it, either.

MichaelRFairhurst commented 6 years ago

I've asked Keerti to help me find the scope of these breakages.

I think the highest priority is making void properly useful as a top type. I would suggest landing that as soon as possible.

@devoncarew @leafpetersen This does nothing but support void as Top. It has 248 failing language tests due to being far too permissive. Maybe that's not a problem, but, that makes me think that the easiest rollout strategy is:

leafpetersen commented 6 years ago

I've asked Keerti to help me find the scope of these breakages.

I think we need this data before we can make further decisions.

Maybe that's not a problem, but, that makes me think that the easiest rollout strategy is:

Rolling out hints has a long turnover cycle: users won't see the hints until the next dev release, they only see them in the IDE, packages usually don't get fixed unless we intervene directly. I'm not saying that it's a bad path, but given the tight timeline, I'm skeptical that this will let us land the feature in a reasonable time frame. Unless Keerti's data show relatively few breakages, I think we're at best going to have explicitly suppress some error messages (e.g when returning void in a dynamic context), or else not issue the error messages at all.

If we do got the "hint" direction, I think we may do better to make it warning rather than a hint. This will break external builds, but I believe that internal code can still be cleaned up incrementally. If we want to go that route, I think we should probably test this out on Flutter to be sure that we are releasing everything in a coherent state that doesn't block rolling to them.

MichaelRFairhurst commented 6 years ago

I think we're at best going to have explicitly suppress some error messages (e.g when returning void in a dynamic context)

Agreed. The suppressable parts are:

Currently, most disallowed usages of void are related to it simply not having the getters/methods/operators. These therefore just don't exist ie voidFn() + 2 and the language tests are happy with the new errors in these cases.

The next most common currently rejected usage of void is side-casts, ie, takeInt(voidFn()), which no longer fail once void becomes a form of Top, since it becomes a simple upcast. The language tests don't like the lack of an error here.

We can then disallow void usage in this case by saying that void cannot be used as an argument with the new warning code USE_OF_VOID_RESULT.

However, that's not fully backwards compatible either, because not all usages of void involved side-casts -- some were upcasts from void to Top. This creates problems where people did things like print(voidFn()), which you would think are rare, but definitely occurred in the language tests. The result of trying the CL will tell us how rare this really is.

It does go beyond arguments & things that were rejected due to missing methods, too. That's where we get x ? voidFn() : voidFn() as the prime example, but there are others such as x ?? voidFn() and void x = voidFn(). These don't result in any erroneous usages of void, by themselves. So in theory we could leave this part out for now.

Maybe we can attempt to catch and alert all downcasts x = voidFn() or useInt(voidFn()) for all cases where the cast is not to Top, which I think would be the most backwards compatible option. However, this is essentially not making void Top, is it not? We'd have to be careful to do it in such a way that lets Future<void> be a supertype of Future<int> without letting int x = voidFn() work via implicit downcast. We would have to make sure that those checks go through different paths that are reliably used since the idea of having different subtyping rules in these cases would be new.

MichaelRFairhurst commented 6 years ago

@eernstg what exactly makes for (x in <void>[]) {} an error?

Using void as a type argument in a list literal?

Iterating over an Iterable<void>?

Both of these have different correlated edge cases (ie, <void, void>{} vs await for (var x in new Stream<void>()) or however that stream/for stuff works again).