dart-lang / sdk

The Dart SDK, including the VM, dart2js, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
9.97k stars 1.53k forks source link

void is inconsistent about whether non-void values can be assigned to it #32558

Open Hixie opened 6 years ago

Hixie commented 6 years ago

I may be confused, but my understanding and intuition is that void means nothing, and something of type void cannot take on any values. This understanding is supported by the way that Dart doesn't let you return anything from void-returning methods:

void foo1() {
  return 0; // "The return type 'int' isn't a 'void', as defined by the method 'foo1'"
}

It also doesn't let you read anything from arguments that are explicitly typed void:

import 'dart:async';

void main() {
  final Completer<void> completer = new Completer<void>();
  completer.complete(0);
  completer.future.then((void value) {
    print(value); // "The expression here has a type of 'void', and therefore cannot be used"
  });
}

(In both these cases, these are only analyzer restructions; the compiler allows both. But that's fine, so long as one of them complains, I consider it "invalid Dart".)

Somehow, though, this doesn't always hold. For example, you can return a value from an async method that returns Future<void>:

Future<void> foo2() async {
  return 0; // no error (!!)
}

You can also pass values to methods expecting void with no ill effects:

void main() {
  final List<void> a = <void>[];
  a.add(0); // no error at analysis time or runtime (!!)
  print(a); // prints [0] (!!)
}

According to the analyzer, you can pass "void"-expecting objects into APIs that expect other types, though that is a runtime error in Dart2 (not Dart1):

void addNumber(List<int> list) {
  list.add(0);
}

void main() {
  final List<void> list = <void>[];
  addNumber(list); // no analyzer error, though the Dart2 runtime does throw here
  print(list);
}

You can pass non-void expecting objects into APIs that expect void objects, with unexpected results:

void printVoidList(List<void> list) {
  print(list); // prints [0] (!!)
}

void main() {
  printVoidList(const <int>[0]); // no error at analysis time or runtime (!!)
}

This leads to situations where you can be completely messing up the types, but neither the analyzer nor the compiler will complain. This code is based on actual code in the Flutter framework that tries to use generics to catch programmer errors, but currently fails to catch any errors because of this:

import 'dart:async';

List<Completer<dynamic>> pendingWork = <Completer<dynamic>>[];

int addWork<T>(Completer<T> work) {
  pendingWork.add(work);
  return pendingWork.length - 1;
}

void completeWork<T>(int index, T value) {
  pendingWork[index].complete(value);
}

void main() {
  Completer<int> completer = new Completer<int>();
  int index = addWork<void>(completer); // no error (!!)                                                                                                                                                                                                       
  completeWork<void>(index, 0); // no error (!!)                                                                                                                                                                                                               
  completer.future.then(print); // prints "0"                                                                                                                                                                                                                  
}
Hixie commented 6 years ago

In summary, I guess I don't understand why return 0 in a void-returning method is considered an error while completer.complete(0) on a Completer<void> is not. They seem to me to be semantically equivalent.

MichaelRFairhurst commented 6 years ago

Ah, I misunderstood the confusion (Null vs void rather than return vs arg).

So in this case you're right its pretty inconsistent and I agree with you.

I do think its important to clarify that allowing both or neither is not an issue of soundness -- void can not be used, so its "safe" to put anything (or nothing) inside a void value.

I say "safe" (with quotes) because it is usually not something people want to do -- statically determine that people want to put data somewhere that it can't be used! That said, drawing the line is kind of funky to determine.

final funcBug = (x) => ++x; // (int) -> int
final funcOk = (x) {  // (int) -> int
 print(++x);
 return x;
}

myInts.forEach(funcBug); // accepts (int) -> void, so this is safe, but useless, indicating a bug
myInts.forEach(funcOk); // accepts (int) -> void, so this is safe, and useful

In this example we can't statically determine when this is a Good idea or a bad one.

I think there are a few places where we can determine that its a bad one, such as void x() => 0.

I would say acceptsVoid(0) should be bad as well, but there's a critical use case here, which is Mockito.

verify(list.add(x)).called(1);

verify has to accept void results to have this API. The type of verify should then be

Verification verify(void _) {
  ...
}

And this is a "safe" way to pass around void because we still know it can never be used.

If we made it illegal to assign non-void to void, then

verify(list.toString()).called(1);

would no longer work without using a different signature for verify:

Verification verify<T>(T _) ...

though, its funny to say, this is what we already do, but just for backwards compatibility so that it works on pre-generalized-void VMs.

This I think would be a safe restriction but still can be misused in two ways

// 1: we lifted an unusable value `void` to a usable value `T`!
void printAll<T>(List<T> l) => l.forEach(print);

// 2: we think a void function returns a list rather than populates one!
int lazyStr;
Future<void> populateStr() => lazyStr = await ...;
List<T> toList<T>(T item) => [item];
Future<List> buggy() => populateStr.then(toList).then((x) => x..add("adding to a List<void>, not a List<String>!"))
// this can be even worse but its generally pretty contrived examples to make this go wrong.

As well as the original "funcOk" "funcBug" example before.

None of these examples result in type errors, but they can still result in bugs.

So I think we have good reason for drawing the lines where we've drawn them, but I personally would like to see "void" not be assignable from non-void, and keep the mockito solution as-written. That said, haven't done experiments on how this works in practice.

MichaelRFairhurst commented 6 years ago

something of type void cannot take on any values

This is not true. Something of void can not be used. This implies that it can be any value, as that will be safe since we guarantee it will not be accessed.

Notably, methods with no return statement return null. So void x() {} returns null. But we could safely change that to 0, its an implementation detail, really.

you can return a value from an async method that returns Future

This is an open issue.

According to the analyzer, you can pass "void"-expecting objects into APIs that expect other types, though that is a runtime error in Dart2 (not Dart1)

The reified type of List<void> is List<Object>. So this isn't always a runtime error, you're just seeing assignability checks but they are not guaranteed to crash :( This I think may be fixed later? I'm not sure.

You can pass non-void expecting objects into APIs that expect void objects, with unexpected results:

This is because a generic of type T may use that type T (for instance, in List.toString()) and then later it may be applied as void. This means you can effectively use the void value in generic code. We could only allow void with things like T extends void, perhaps. But that may not always be intuitive either.

eernstg commented 6 years ago

I may be confused, but my understanding and intuition is that void means nothing, and something of type void cannot take on any values.

I don't think that's a good intuition to have about void in Dart. Dart 2 comes from Dart 1, and Dart 1 never supported that intuition: It has always been true that a function returning T is a subtype of a function returning void (when the parameter types allow it, e.g., when the two function types have identical parameter types). So when you think you're invoking a function returning void it could actually be a function returning T, for any T.

The fact that void foo() {} is correctly overridden by T foo() {} for all T illustrates that the situation is the same for instance method invocations as it is for invocations of first-class functions.

The intuition that does work is that void is a top type (like Object, so a value of type void can be anything whatsoever), but it is marked with the property that "this value should be discarded".

This understanding is supported by the way that Dart doesn't let you return anything from void-returning methods: ..

It is still possible to justify that based on the understanding that void is a top type whose value should be discarded: return e; in a void function is misleading, because it looks like the value of e will be used, but given that the return type is void it will be discarded (unless the call-site explicitly overrules the "should be discarded" flag using as).

It also doesn't let you read anything from arguments that are explicitly typed void:

Here, the error message explicitly spells out that a value of type void should be discarded, and there's nothing stopping that value from being anything.

For the next example, you're right that we need a correction:

Future<void> foo2() async {
  return 0; // no error (!!)
}

I added a sentence about this situation and a reference to this issue here, where return is being discussed.

You can also pass values to methods expecting void

Right, you're allowed to discard any value. We discussed this, and you could certainly claim that it's equally bad to discard a value by passing it as an actual argument to a function as it is to discard it with return e (in both cases you might mistakenly assume that the value is being used). But the argument why return e is worse is that it will discard the value every time the enclosing function is invoked, and that is a misinterpretation of how the enclosing function works. But when you pass a value like f(e) where f takes an argument of type void, it is (1) an implementation detail for f that it ignores its argument, (2) explicit for anyone looking at the declaration of f that it is intended to ignore its argument (it would be SomeType f(void x) { ... /* x not used here, by design */ ... }). So we are at most misinterpreting a call site, not a function.

you can pass "void"-expecting objects into APIs that expect other types,

This is allowed until we add a 'voidness preservation' analysis (which is mentioned in the generalized void feature specification). Voidness preservation deals with all the situations where a "derived type" can be void, e.g., because we are operating on a List<void> under the statically known type List<Object>. Your second example actually touches on voidness preservation in a slightly surprising way:

void printVoidList(List<void> list) {
  print(list); // Formal parameter type `Object`, no problem.
}

void main() {
  printVoidList(const <int>[0]); // Assign `List<int>` to `List<void>`, no problem.
}

First, note that it was decided at a very early point that void differs from Object only by carrying the "please discard!" flag during static analysis, at run time it is the same type. Also note that there are no soundness issues in using a value of type void, it is purely a developer specified intention that certain values are not meaningful and should be discarded. So there is nothing wrong with the values according to the language semantics.

Hence, it is purely a static property to be void (rather than just being an Object).

In particular, we don't want to generate code for List in such a way that it behaves differently when it has type argument void. So in print(list) we just pass an object of some type (it happens to be List<void>) for a parameter of type Object, and that is allowed. Any object (including a List<void>) has a toString method, and since a List<void> is not behaving differently than a List<Object>, it will proceed and print its elements as usual.

This illustrates that being void is not a security feature, we should not assume anything like "a void value is confidential".

Next, printVoidList(const <int>[0]) is fine, also when we add voidness preservation, because it "indirectly marks the list elements with the should-be-discarded flag", and it's always OK to forget stuff.

However, if you had done something like List<Object> xs = <void>[1]; then it would be a voidness preservation violation.

With that, the first example is also covered: addNumber(list) will be a voidness preservation violation because it involves a cast from List<void> to List<int>. Today, without voidness preservation, it is allowed, but it still fails because List<Object> is not a subtype of List<int> (which is how the type check looks at run time, because void is a static thing).

This leads to situations where you can be completely messing up the types

For the last example, pendingWork.add(work); could be marked as a voidness preservation violation because it uses an actual argument of type Completer<T> where the formal parameter has type Completer<dynamic>, and T can be void.

However, this is a rather draconian rule and in the end we may decide that we won't enforce it. If we don't enforce it then there is a known soundness hole in the treatment of void: We can pass a value of type void into a context where the type void is represented by a type variable X, and then we can assign that value to a variable of type dynamic or Object (because X <: dynamic and X <: Object), and the "void value has then escaped".

Again, we know how to plug this hole, but it's draconian in the sense that it forces type variables without bounds to be treated like void.

The other flagged statement is completeWork<void>(index, 0), and that's OK (it discards 0, and we can forget anything).

So you are pointing out several sharp bends on the road to void, including the fact that we have chosen to introduce a first-order treatment of void now, and add the higher-order part (voidness preservation) later, but I hope that you can use the intuition I mentioned ("a void value can be anything, but you should discard it") to make the whole thing make sense. ;-)

MichaelRFairhurst commented 6 years ago

+Everything that Erik just said!

I may be confused, but my understanding and intuition is that void means nothing, and something of type void cannot take on any values.

I think you're thinking that void is kind of like Haskell's undefined? In that case, the type you want here is Null. Now, there is one value that can satisfy Null, but its null of course, and that is pretty much the right way of saying "cannot have a value."

import 'dart:async';

void main() {
  Completer<int> completer = new Completer<int>();
  int index = addWork<Null>(completer); // Runtime error: Completer<int> not assignable to Completer<Null>
  completeWork<Null>(index, 0); // static error: 0 is not assignable to Null
  completer.future.then(print); // prints "null"                                                                                                                                                                                                                  
}

This means you can do the following:

List<Completer<Null>> work;

Future<void> valuelessWork() {
  work.add(new Completer<Null>());
  return work.last.future;
}

void main() {
  var x = valuelessWork();
  work.pop().complete(null); // this is the only legal way to complete the work
  x.then((value) {
    print(value); // value is of type void and cannot be used
  });
}
Hixie commented 6 years ago

I think I'm mostly saying two things here:

  1. the current state is confusing (sounds like this is a known issue and it'll be resolved in due course)
  2. I wish the analyzer and debug/checked mode could help me more with preventing mistakes like the last example of my original comment above. Explicitly assigning a value to a void-declared field or explicitly passing a value to a void argument (whether statically detectable, or catching it at runtime) is very often a subtle bug, and our code will be more reliable if we can catch such cases.

IMHO what mockito is doing is wildly bogus and i have no problem saying that to do that you need lots of explicit casts or pragmas or //ignores or whatever.

MichaelRFairhurst commented 6 years ago

Note: I edited this a bunch since first posting...

How about a lint for assigning to a void value? And a lint for inferring a type parameter of type void?

The latter will require you to use //ignore in many cases with mockito and be hard to write, but the former would be very easy. And these makes sense as a lint, because it isn't a soundness problem, but more like a "you seem to be holding this wrong" problem.

here's what we could/couldn't catch with these two lints.

int returnInt() => 4;
void voidFn() => returnInt(); // won't be caught; its assumed returnInt() must have side-effects.

class Uses<T> {
  void use(T t) => print(t);
}

new Uses<int>(voidFn()); // caught today.
new Uses<void>(returnInt()); // can be caught with lint #1, otherwise will print 4
new Uses<void>(voidFn()); // won't be caught; will print 4

void use<T>(T t) => print(t);

use<int>(voidFn()); // caught today
use(voidFn()); // can be caught with lint #2, otherwise will print 4

...

IMHO what mockito is doing is wildly bogus

But I do respectfully disagree here. I'd invite you to consider that what mockito is doing has no soundness issues, produces no bugs, is not surprising to users, is cleaner than the Java version, and is relied upon to work like this in thousands of places. You can say it relies upon something bad, but that does not mean that it itself is bad.

I still think that the type you want here is Null:

void main() {
  Completer<int> completer = new Completer<int>();
  int index = addWork<Null>(completer); // runtime error, +static error with --no-implicit-downcasts
  completeWork<Null>(index, 0); // error, 0 not assignable to Null
}

and I think you should be proud for being one of those rare types of users that is using the type system sufficiently enough to need to use Bottom instead of Top. But those have strong theoretical grounding, so we can't really "fix" them for you if you find it unintuitive (which it is) to know which one to use.

As a general rule of thumb, data coming in will use Bottom when data going out uses Top, and vice versa. Void is a form of Top, and Null is Bottom. So if you would be returning a void value, then you want to accept a Null value.

MichaelRFairhurst commented 6 years ago

my understanding and intuition is that void means nothing, and something of type void cannot take on any values.

As another example of void meaning "anything" and Null meaning "nothing" -- even though void can't be used as anything and Null can -- consider a function that throws. Usually people type it like so:

void throwException() { ... }

This seems like a good idea until you realize that its typesafe to abort the program anywhere.

print(throwException());

^^ this is fine. It won't print, it will throw.

To do this, we use the Type indicating "Nothing." Namely, Null, or Bottom. This is the subtype of all types.

Null throwException() { ... }

Granted, this makes more sense in lazy languages:

return [1, 2, 3, throwException()];

but it still can be useful in dart for the same type of reason:

cache.getOrUse(key, throwException);

Here, if we use "() -> void" then that's not a subtype of "() -> T". But "() -> Null" is for any T.

my understanding and intuition is that void means nothing, and something of type void cannot take on any values.

You can see how this is a better example of "Nothing" than "void." By using Bottom, we can say "this value is usable anywhere" and the reason we can safely say it is because it will never take on any values -- you type a function as return Null to say it never returns.

This is the opposite of saying it returns any value: "void." In that case, it does take/return a value, we just make zero promises about what it is.

So if it never takes/returns a value -> we can promise it does everything. Its a subtype of all types. Because it won't happen. If it does take/return a value -> we can give no promises about what it does. It is a supertype of all types. And its there, you just can't use it.

Note that if dart gets non-nullable types, then Null will no longer be the Bottom type. We'll probably add a new type named Nothing or something.

Hixie commented 6 years ago

I understand the theory (mostly), but I think you're fighting an uphill battle. "void" simply looks like it means "nothing" (also void. n. a completely empty space.). Writing code like "void foo() {}" feels natural. Writing code like "Null foo() {}" does not. As soon as "void" was a valid keyword, we started using Future<void> over Future<Null> because it just feels more idiomatic and correct, even if it isn't.

We should go with the flow here and just have void work the way people will assume it works anyway.

MichaelRFairhurst commented 6 years ago

I think the lint is still a good approach, because values being thrown away isn't a type error but it is a code smell.

Its still tricky to know where to draw the line, and I think Null just is a useful type whether void does or doesn't do something.

It will be better in the future if we make non-nullable types, and Bottom is called Nothing instead of Null. Then I think people will feel proud of themselves for using the type system to the point of distinguishing Completer<Nothing> from Future<void>, and learning the theory differences, rather than feeling grossed out by having to use the type Null, which I must admit, just feels like a step backwards.

eernstg commented 6 years ago

"void" simply looks like it means "nothing" (also void. n. a completely empty space.)

I think it might be easier to reconcile common intuitions and language semantics by focusing on the other words spelled void: verb 1. NORTH AMERICAN declare that (something) is not valid or legally binding. "the Supreme court voided the statute"; or: void adjective 1. not valid or legally binding.

So when we encounter a void value, we know that it shouldn't be considered valid, and hence there is no other appropriate action than discarding it.

Hixie commented 6 years ago

If we were able to take each developer aside and show them each dictionary definition and have a discussion with them about why they were wrong to think void meant what they've always thought it meant, then sure.

In practice, people will look at Dart code and be confused, then they'll either write buggy code, or they'll give up and use another language.

ianloic commented 6 years ago

First, note that it was decided at a very early point that void differs from Object only by carrying the "please discard!" flag during static analysis, at run time it is the same type.

The other difference is that it is an error for a function that returns Object to not return anything.

But, I have an actual practical question: I'm designing an API and need to return a Future that has no value associated with completion. Should I use Future<void> or Future<Null>?

eernstg commented 6 years ago

The other difference is that it is an error for a function that returns Object to not return anything.

A Dart function will always return something, but return; and reaching the end without executing a return statement will cause the null object to be returned, i.e., those things work like return null;. There is no problem with that, but it is considered error-prone to return the null object implicitly (that is, in those two ways) in a function whose return type is not void (and a couple of other corner cases are allowed as well). You could say that this is the other side of the coin: When "returning to void" you shouldn't pass a useful value (because we think you've forgotten that the value will be discarded), and when "returning to Object" you shouldn't choose the returned value implicitly (because we think that you've forgotten that the value is not in general going to be discarded). In both cases it's part of the static analysis, and the actual return type at run time will be Object.

.. return .. Future<void> or Future<Null>?

Use Future<void> if the API is intended to express that it is potentially useful to synchronize with the completion of the underlying task, but the value associated with the completion should be discarded. So you're basically telling the caller that they can do foo(); as a statement (fire-and-forget) and they can do await foo(); as a statement (synchronize only), but they shouldn't use the result from evaluating the expression await foo(). You get tool support to enforce this because it is a compile-time error to evaluate an expression of type void, except in a relatively short list of situations (search for 'situations:' here).

Future<Null> has been used previously as a replacement for Future<void> (when void wasn't yet supported by the analyzer and compilers as a type argument). It does in fact express the intention that there is no useful value at completion (Null has zero information content because we already know that the value will be null), but it is error prone because the null object can be passed on as any type (currently, and even if we add non-nullable types it can still be passed as anything-nullable): It would be a strange exception to make String s = await e; an error because e has type Future<Null>—would you then also make String s; an error because that will also initialize s to have the value null? So Future<Null> does not work well to communicate to callers of your API that they should ignore the completion value.

bwilkerson commented 5 years ago

@MichaelRFairhurst Is there still work to be done here? If so, can you add this to the 2.1 milestone? If not, please close it.

Hixie commented 5 years ago

As far as I can tell the issues mentioned in the very first comment in this bug still exist.

eernstg commented 5 years ago

As far as I can tell the issues mentioned in the very first comment in this bug still exist.

I suspect that they may be covered by invalid_returns.md, in which case we're just awaiting implementations to catch up (and that's handled in other issues).

The following is a verbatim copy of the code in the [initial text of this issue](), with my comments placed after each snippet of code.

void foo1() {
  return 0; // "The return type 'int' isn't a 'void', as defined by the method 'foo1'"
}

This an error, and that's also the requested behavior as far as I understand: Done. (I believe all implementations behave in that way as well.)

import 'dart:async';

void main() {
  final Completer<void> completer = new Completer<void>();
  completer.complete(0);
  completer.future.then((void value) {
    print(value); // "The expression here has a type of 'void', and therefore cannot be used"
  });
}

Again an error (and implemented as such by the analyzer as well as the vm today), which is also what this issue requested: Done.

Future<void> foo2() async {
  return 0; // no error (!!)
}

True, invalid_returns.md does not make this an error.

It's a tricky case: In an asynchronous function, return e; means evaluate e to an object o, await o if it is a future of a suitable type, otherwise use o as the "returned value", then complete the future which was returned from this function invocation with o.

So we are not returning o, we are using it to complete a future. This makes the situation similar to f(0) where f accepts a parameter declared as void x, and different from return e; in a synchronous function.

So this may be a controversial choice, but it's not going to be realistic to change it now (that Dart 2 has been released).

void main() {
  final List<void> a = <void>[];
  a.add(0); // no error at analysis time or runtime (!!)
  print(a); // prints [0] (!!)
}

This is again as intended: Any non-void value can be discarded (except, as mentioned, via return e in a void function).

Note that it was explicitly part of the design discussions that a function like X seq<X>(void v, X x) => x; should be possible to declare, and possible to use (without errors/warnings) to evaluate sequences of expressions, discarding all but the last one: seq(foo(42), seq(print('Whatever')), baz())) gets us the side-effects from foo, print, and baz, but discards the results from foo and bar. Note that we wouldn't be able to discard the result of an expression of type void unless the first parameter has type void.

So I'll consider that as done as well.

void addNumber(List<int> list) {
  list.add(0);
}

void main() {
  final List<void> list = <void>[];
  addNumber(list); // no analyzer error, though the Dart2 runtime does throw here
  print(list);
}

During addNumber(list) we are discarding 0, which is OK as usual. During print(list) we invoke a method on list, and I've explained here that we don't want to generate different code for an instance of List<void> (void is Object at run time, after all!), so there's nothing wrong in printing the elements of the list, either.

There is a dynamic error (as mentioned: 'does throw'), but that's just because a List<Object> cannot be passed to addNumber, so the implicit downcast fails.

Another matter is that the downcast is also a voidness preservation violation, but we are not checking that at this point.

void printVoidList(List<void> list) {
  print(list); // prints [0] (!!)
}

void main() {
  printVoidList(const <int>[0]); // no error at analysis time or runtime (!!)
}

It is not an error to pass a List<int> to a parameter of type List<void>: That's simply an upcast (and it won't be a voidness preservation error, either, because we are "indirectly discarding values", which is no worse than discarding values directly: (void v) {} (42) is OK even though we are discarding an integer).

This leads to situations where you can be completely messing up the types, but neither the analyzer nor the compiler will complain. This code is based on actual code in the Flutter framework that tries to use generics to catch programmer errors, but currently fails to catch any errors because of this:

import 'dart:async';

List<Completer<dynamic>> pendingWork = <Completer<dynamic>>[];

int addWork<T>(Completer<T> work) {
  pendingWork.add(work);
  return pendingWork.length - 1;
}

void completeWork<T>(int index, T value) {
  pendingWork[index].complete(value);
}

void main() {
  Completer<int> completer = new Completer<int>();
  int index = addWork<void>(completer); // no error (!!)                                                                                                                                                                                                       
  completeWork<void>(index, 0); // no error (!!)                                                                                                                                                                                                               
  completer.future.then(print); // prints "0"                                                                                                                                                                                                                  
}

The issue I can see here (presumably motivating the claim that 'you can be completely messing up the types') is that we do not enforce the constraints of void expressions on the scope of a type variable whose corresponding actual argument could be void.

So we can do this: void foo<T>(T t) => print(t); and then foo<void>(42), which will cause print(t) to be invoked in a situation where t has type T and T has the run-time value void (which is of course not true, the run-time value is then Object, but the syntax at the call site was void).

That's just a fact of life: The treatment of void does not have a soundness property (like "the value of an expression of static type void is guaranteed to be discarded"), and the treatment of type variables (as being non-void, even though the ones that have no bound could be passed as void) is a gaping hole in the would-be soundness property. We could easily plug that hole, but we consider that choice to be less useful than the current choice, where you will indeed be able to use some values that were obtained from an expression of type void, without any errors or other heads up at compile time.

Other than that, I don't really see any types going wrong: You would just replace void by Object everywhere in order to see what the code means (except for the void specific errors about using rather than discarding some values), and then the whole thing is rather normal (except that the usages of Object are somewhat aggressively taking away type information that we'd otherwise preserve).

So what you are showing here, I suppose, is that (1) when we treat type variables like non-void types, and (2) when we (still) don't check for voidness preservation, even a short snippet of code will allow us to "break the rules" around void. Maybe we could say it as "the unsoundness of void matters". That's definitely worth discussing.

We might consider adding more strictness, e.g., by flagging situations where an expression e whose type is a type variable with no bound is subject to an upcast to a top type (like dynamic foo<X>(X x) => x). As long as we keep the value "inside the type X" we can maintain the discipline that this value should not be passed back to the caller (nor should it be assigned to a global variable etc.) under a different type (which would then have to be a top type because X has no other supertypes). So we can use it, as in x.toString(), but we can't leak it "out of void" at the call site.

The other topic that remains on the table is that we don't prevent Future<void> f() async => 42;, which seems to contradict the fact that void f() => 42; is an error, but as I mentioned: "It is not really a return when you do return e in an async function".

So I don't think we're talking about bugs, and it might well be a useful clarification to submit the "discussion issues" separately, in new issues, and this issue could then be closed.

@Hixie, would it work for you to proceed in that manner?

Hixie commented 5 years ago

I don't mind if this is refiled as a new issue, I'll leave that up to you.

If having these issues flagged as errors in the analyzer is not possible, would it be possible instead to just have a lint that catches these issues? e.g. a lint that treats an async return the same way a regular return would be treated, and so on? My goal here isn't to litigate what should be an error or not, I just wish to have the tooling help us catch bugs sooner.

eernstg commented 5 years ago

Sounds good! I just created #34455.

I think we can certainly cover these things with lints: (1) The special case on return e in an asynchronous function, (2) voidness preservation (and that won't be proper errors because Dart 2 has been released, and we don't want to add new compile-time errors that would break existing code), and (3) restrictions on type variables whose value could be void.

The reason why it's no problem to make them all lints (from a technical point of view) is that all these restrictions are outside the core notion of soundness of Dart: We can access all properties of an instance just fine even though we obtained it from an expression of static type void, it's more like a "business rule" that such values should be discarded.

srawlins commented 1 year ago

I think null safety cleaned up some of this. I'll take @Hixie 's original 4 examples of "there should be a static error here":

For example, you can return a value from an async method that returns Future<void>:

Future<void> foo2() async {
  return 0; // no error (!!)
}

This now does result in an error. ๐ŸŽ‰

You can also pass values to methods expecting void with no ill effects:

void main() {
  final List<void> a = <void>[];
  a.add(0); // no error at analysis time or runtime (!!)
  print(a); // prints [0] (!!)
}

The void_checks linter rule (in the recommended set, and live at dartpad) now reports on this code. ๐ŸŽ‰

According to the analyzer, you can pass "void"-expecting objects into APIs that expect other types, though that is a runtime error in Dart2 (not Dart1):

void addNumber(List<int> list) {
  list.add(0);
}

void main() {
  final List<void> list = <void>[];
  addNumber(list); // no analyzer error, though the Dart2 runtime does throw here
  print(list);
}

This is now a compile-time error. ๐ŸŽ‰

You can pass non-void expecting objects into APIs that expect void objects, with unexpected results:

void printVoidList(List<void> list) {
  print(list); // prints [0] (!!)
}

void main() {
  printVoidList(const <int>[0]); // no error at analysis time or runtime (!!)
}

This continues to pass static analysis, and has no errors at runtime. ๐Ÿ˜ฆ

eernstg commented 1 year ago

Note that we have used the phrase 'voidness preservation' to describe a feature which is concerned with the detection of higher-order versions of errors like the error at expressionOfTypeVoid.someMethod(), or any other void-based errors. This actually like the converse of printVoidList(const <int>[0]): Voidness preservation will flag situations like List<Object?> xs = <void>[]; and Object? Function() f = g; where g has static type void Function().

Some efforts were made in this direction, cf. https://github.com/dart-lang/linter/issues/1124.

On the other hand, we never had any proposals (as far as I know) which would make it an error/warning/lint to execute print(xs) where xs has a static type like List<void>. In that case we just detect that List<void> is a subtype of the parameter type Object? (so the actual argument is type correct), and neither the body of print nor the methods of List will know that the type argument is void. In other words, protection against the use of a value of type void has always been completely absent in the case where the type is a type variable whose value is void.

I think it would be difficult to do anything meaningful in this area. For instance, for an xs with static type List<void>, xs.length and xs.clear() should presumably be OK ("conceptually") because those operations aren't using any of the elements, but it's technically difficult to make a distinction between these operations and others like xs.contains(0).