Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fasta: Generalized void #30470

Closed
eernstg opened this issue Aug 17, 2017 · 57 comments

Comments

Projects
@eernstg
Copy link
Member

commented Aug 17, 2017

This is the Fasta specific issue for issue #30176, which contains the details.

@eernstg eernstg referenced this issue Aug 17, 2017

Open

Support generalized void #30176

7 of 8 tasks complete

@eernstg eernstg changed the title Common front end: Generalized void Fasta: Generalized void Aug 17, 2017

@anders-sandholm anders-sandholm added this to the 2.0-alpha milestone Dec 20, 2017

@anders-sandholm

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

From talking with @lrhn it sounds like this will block the next wave of library changes.

@kmillikin, @stefantsov, just wondering if there is a way we could/should move this forward in time.

@lrhn

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

To be more specific, the next wave will include changes to asynchronous classes like Future and Stream. Some methods currently return Future where they should return Future<void>. We'd like to make those changes at the same time as all other changes to those the classes. In practice, it's a small change since void and dynamic are equivalent wrt. subtyping, so Future<void> and Future<dynamic> are too, but it would be great to have it early so we can see if some use-places are affected (you'd not be allowed to use a value with static type void, you would when it has static type dynamic).

Some methods return Future<Null> where they should return Future<void>. Those changes are bigger changes - it's an error to change a subclass before the superclass/interface has been updated because Future<void> is not a subtype of Future<Null>, it's the other way around.
Luckily we don't have any of those in the public parts of the platform libraries.

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Do we have any tests of this?

@lrhn

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

A quick search shows language_2/generalized_void_syntax_test.dart, language_2/void_type_callbacks_test.dart, language_2/void_type_override_test.dart and language_2/void_type_usage_test.dart as containing <void>.

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

I need a list of the actual tests that the language teams has implemented to test this feature. I know how to do a quick search.

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Proposed fix: change 30760.

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2017

I added a positive test here: https://dart-review.googlesource.com/c/sdk/+/30840. It's simply checking that expressions of type void are allowed to occur in the situations where we want them to be allowed (I ran the test with many configurations and tools, and none of them failed. Hurrah! ;-).

Edit: Landed in commit aa97656.

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2017

NB: Change 30760 allows the void type to occur as a type, but it does not ensure, e.g., that the following compile-time error occurs:

void get x => null;

void main() {
  var y = x; //# 01: compile-time error
}

So it would be a fix for part of this issue, not all of it. But a partial fix is also useful, of course!

@kmillikin kmillikin added this to Incoming in Dart Front End Jan 3, 2018

@jensjoha jensjoha added the p2-medium label Jan 10, 2018

@jensjoha jensjoha moved this from Incoming Untriaged to Triaged in Dart Front End Jan 10, 2018

@a-siva a-siva added p1-high and removed p2-medium labels Jan 11, 2018

@mraleph

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

Note: this is important for being able to run Flutter. That's why we are bumping priority.

For the record there is a patch from @peter-ahe-google https://dart-review.googlesource.com/c/sdk/+/30760

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

I'm still short on tests of the new features, I do have tests of existing features that have been in Dart for years.

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2018

Here are some tests in sdk/tests/language_2 that contain constructs using void in a way that used to be unsupported, and whose expectations match the semantics when the 'generalized void' feature is implemented. In other words, they should run successfully when void is generalized, and they do test things that are relevant for the generalized void feature.

  • generalized_void_syntax_test.dart: This test uses uses void in positions that used to be compile-time errors (typically: syntax errors) which are supported when void is generalized.

  • generalized_void_usage_test.dart: This test uses a void typed expression in the 6 situations where an expression of type void must be accepted when void is generalized.

  • void_type_callbacks_test.dart: This test contains expressions of type void in various contexts, along with test outcomes like compile-time error when needed for the new semantics.

  • void_type_function_types_test.dart: Tests function types with void also as a parameter type.

  • void_type_override_test.dart: Contains tests involving the void type and overriding.

There are other tests involving void, of course, but I could not find any others that are testing the generalized void feature. One test is obsolete in the sense that it should fail under the new semantics: void_check_test.dart. The problem is that it contains the declaration

callFoo(A a) => a.foo();

where a.foo() is an expression of type void, and that should incur a compile-time error. I suppose the best approach to fix this test would be to change the declaration to

callFoo(A a) => a.foo() as dynamic;

in which case it should still run successfully under the new rules.

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2018

I believe that the most productive approach, for any given tool/configuration, would be to make generalized_void_syntax_test.dart run (maybe it runs fine already) and then concentrate on void_type_usage_test.dart. The latter contains 239 multi-test comments with compile-time error and 36 with ok. These two tests touch on a lot of different contexts where void and void-typed expressions can occur, with all outcomes based on having generalized void.

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2018

@eernstg thank you, I'll give that a try.

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2018

@peter-ahe-google Thank you! Note also #31883 which explains why some subtests in void_type_usage_test.dart can be somewhat misleading. That shouldn't matter much, though, because there are so many other subtests in that test.

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

I have landed the CL, @peter-ahe-google you can close this issue.

@peter-ahe-google peter-ahe-google added p2-medium and removed p1-high labels Jan 24, 2018

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

Thank you, @a-siva. Unfortunately, work still remains on producing the correct compile-time errors.

@vsmenon vsmenon added this to the Dart2.1 milestone Jul 24, 2018

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

In progress CL sorting out language tests here: https://dart-review.googlesource.com/c/sdk/+/66521 for reference. Tests under language_2/void have been triaged.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

@vsmenon

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Is there anything further we need to do here?

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

I'm still seeing CFE errors on void to void transfers, and this seems to be causing trouble with rolling flutter into fuschia. This example produces an incorrect error:

void foo() {}

void main () {
  void v = foo();
}

The tests I landed don't seem to have been triaged yet.

cc @tvolkert

@tvolkert

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

FYI, this is producing spurious warnings in the Fuchsia build (it causes issues with mockito's verify() call), but it's not blocking a roll.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

FYI: I made a mistake while revising the spec for void related returns that implied an undesired breaking change. I fixed the spec and the analyzer implementation, and adjusted the tests here: https://dart-review.googlesource.com/c/sdk/+/70462 .

@MichaelRFairhurst

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

another user hitting this: dart-lang/mockito#163

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

For planning purposes, what's the status of this issue? Is a revert of https://dart-review.googlesource.com/c/sdk/+/66160 likely? Is the analyzer likely to change? Are the compiler warnings here to stay? Is the status quo likely to remain for the foreseeable future, or is something likely to change in the near term?

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Some of the compiler warnings should be errors, and some of them are bugs in the implementation (they are false positive warnings where there is no problem).

2a36502 fixed many of the false positive warnings, but not all of them. They were left as warnings. This CL should fix the remaining false positives and turn the genuine error cases back into static errors.

I will ensure that I verify that the errors do not break anything in Flutter that I can see, and ensure the behavior is as intended according to the language team, before landing the fix.

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Thanks. Will there be an effort to make sure the analyzer reports at least all the errors that the compiler reports? One of the weird things we're dealing with right now is that the compiler complains about things that the analyzer is happy with.

@MichaelRFairhurst

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

@Hixie

Yes, and I'll be the one to do that.

From what I know, the missing errors in the analyzer here are extreme edge cases (like !print(...)). So its not currently my top priority but I can see about fixing them sooner, for the sake of making this situation less confusing.

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Great! We see a lot of these messages when running our tests (from mockito, I assume), so we are looking forward to these changes making those go away. Let me know if there's anything we can do to help.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@leafpetersen @eernstg

By my understanding of https://github.com/dart-lang/sdk/blob/master/docs/language/informal/invalid_returns.md this should be invalid:

FutureOr<void> f() => print('Hello');

It's a synchronous function and the return type is not void so we need to have that the expression would be a valid return expression in a block-bodied synchronous function. None of the three cases for valid returns of an expression from a block-bodied synchronous function make this valid.

Is that correct?

@eernstg

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

Discussing this IRL, Kevin and I concluded that invalid_returns.md makes it an error: We go from 'Expression bodied functions', synchronous, to 'Block bodied functions', 'Synchronous', with T == FutureOr<void> and S == void, which fails to match any of the specified cases.

In the case where we decide that it "should" have been a valid return, I believe the relevant adjustment would be to require 'S is voidy' rather than 'S is void' in bullet 2, where a type is voidy iff it is void or it is of the form FutureOr<S2> where S2 is voidy. We could also say 'S is FutureOr^k<void> for some k >= 0'.

The conceptual consideration here is that we may consider FutureOr<void> values to be either a value that we should discard, or a value which is a Future<void>; allowing this case means allowing that we discard such a Future<void> silently. I would consider that to be a reasonable treatment, because I prefer to think of FutureOr<void> as just another way to spell void: When the value "could be void", we should discard it.

The opposite side of that coin would be to say that it is an error to perform case analysis on a value of type FutureOr<void> to see whether it's a Future, because that might be a future which should be ignored (say, because it was returned from a fire-and-forget function, i.e., we received it "with the type void" and hence we should discard it). I know that I won't get support for that, but it's worth keeping in mind that we will have a certain amount of dissonance if we do both: Allow a FutureOr<void> value to be discarded silently, and allow a FutureOr<void> value to be subject to case analysis.

@lrhn

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

I agree with the interpretation. We did not go for "voidy" in the asynchronous cases either, and I'm not sure we should for the synchronous ones alone. This is a pure void expression occurring in a position where the value is going to be used (to check if it's a future). There is no need to make the return type FutureOr<void> in this case, void is sufficient and the function type will then be a proper subtype of the function type with FutureOr<void> as return type.

So, all in all, I'm not sure we need to change anything.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Something seems off about that. I believe it's OK to write this (Dart 1 programmers do it all the time):

Future<int> future;
...future.then((i) => print('done'))...

If I notice that this same function is used multiple times, I might name it as a local function and I might declare a type for it. (In fact, I will declare a type for it because I want the type checker to validate my understanding and I want the type to serve as documentation.) I will just use the inferred type which is FutureOr<void> Function(int). So:

FutureOr<void> done(int i) => print('done');
...future.then(done)...

Here the implementation will tell me that I have a static error which is an invalid return. But something's fishy about that, because all I did was move some code around and annotate it with its inferred type. So either (1) there's nothing wrong with this return or (2) there is, but not both.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@Hixie the current status is that fixing the remaining false positive warnings for void requires us to have an implementation of the spec for invalid returns. And implementing that specification (as specified) will give us even more false positive errors (which is undesirable). So we need to sort out how to avoid those errors in a consistent way.

ETA is hopefully the end of this week. There doesn't seem to be any implementation problems to sort out.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

The front end is now passing the language_2 void and invalid returns tests. This was landed in 9bdf248.

@kmillikin kmillikin closed this Sep 5, 2018

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I will just use the inferred type which is FutureOr Function(int). So:

Well, actually the inferred type is void Function(int) since we use the principal type for functions, and if you use that type, it's ok (this is why there wasn't an error in the lambda to start with). But agreed that it's not obvious to the user that this is the inferred type.

These voidy warnings were something of a rabbit hole, so I'm hesitant to revisit them without some evidence that there is a real problem here.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Until a few days ago the inferred type was FutureOr<void> Function(int) but that was a bug and I fixed it. Now we do have void Function(int):

static method main() → dynamic {
  asy::Future<core::int> future;
  future.{asy::Future::then}<void>((core::int i) → void => core::print("done"));
}

@kmillikin kmillikin removed this from Triaged in Dart Front End Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.