Skip to content

Analyzer: Support generalized void #30177

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

Closed
eernstg opened this issue Jul 17, 2017 · 69 comments
Closed

Analyzer: Support generalized void #30177

eernstg opened this issue Jul 17, 2017 · 69 comments
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Milestone

Comments

@eernstg
Copy link
Member

eernstg commented Jul 17, 2017

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

@eernstg eernstg added the legacy-area-analyzer Use area-devexp instead. label Jul 17, 2017
@eernstg eernstg self-assigned this Jul 17, 2017
@eernstg eernstg mentioned this issue Jul 17, 2017
8 tasks
@bwilkerson bwilkerson added P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jul 20, 2017
@eernstg
Copy link
Member Author

eernstg commented Jul 26, 2017

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
Copy link
Member Author

eernstg commented Aug 7, 2017

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

@eernstg eernstg removed their assignment Dec 20, 2017
@anders-sandholm anders-sandholm added this to the 2.0-beta1 milestone Dec 20, 2017
@bwilkerson bwilkerson added P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P2 A bug or feature request we're likely to work on labels Dec 20, 2017
@bwilkerson
Copy link
Member

@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
Copy link
Member Author

eernstg commented Jan 11, 2018

(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
Copy link
Member

/cc @MichaelRFairhurst

@MichaelRFairhurst MichaelRFairhurst self-assigned this Jan 11, 2018
@MichaelRFairhurst
Copy link
Contributor

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
Copy link
Contributor

MichaelRFairhurst commented Jan 11, 2018

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
Copy link
Member

leafpetersen commented Jan 12, 2018

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 #31869

@eernstg
Copy link
Member Author

eernstg commented Jan 12, 2018

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
Copy link
Contributor

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
Copy link
Member Author

eernstg commented Jan 12, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

eernstg commented Jan 15, 2018

@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
Copy link
Member Author

eernstg commented Jan 15, 2018

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
Copy link
Contributor

Thanks Erik for the quick clarification!

@MichaelRFairhurst
Copy link
Contributor

But yeah, I'm likely shaking up the nest for no reason. The implementation of void here is great and I love the simplicity. Little quirks are expected in any complex system.

For solutions not related to the void typesystem.......I suppose that if we knew the variance of X in T<X>, we could say that only contravariant (not covariant or invariant) generics should be able to be applied by void. This makes Future<void> a valid type, but not Sink<void> (because why?) nor List<void> (and therefore not List<Future<void>>).

@lrhn
Copy link
Member

lrhn commented Jan 22, 2018

Seems reasonable ... except that Future<void> is covariant and a primary reason for allowing void as a type argument in the first place. Another example was Listener<void>, where the type variable is used as return type for a generic listener pattern implementation.
It really is cases where we want to generalize over return types that made us allow void as a type parameter, and all the other uses of it are just unavoidable complications on top of that ... and return values are covariant.

We want Future<void> because it really is the correct type for an asynchronous void function. If we have that, we need to allow (void x) { ... } as argument to Future.then. Then we need to be able to handle void typed variables and expressions everywhere. And with inference, the moment we have void somewhere, it can end up anywhere.

We don't particularaly want a void collection like List<void>. It's just that we can't have the things we want without also allowing that.

@devoncarew devoncarew modified the milestones: 2.0-beta1, 2.0-alpha2 Jan 26, 2018
@eernstg
Copy link
Member Author

eernstg commented Feb 1, 2018

In this comment I mentioned that I had created a CL that allows conditional expressions to contain branches of type void (so we can have void foo() => b ? e1 : e2; where e1 and e2 can have the type void).

Proposals for an extended version of the same idea have come up since then, such that we would generally allow data flow where the value of an expression of type void is allowed to flow to a sink of type void (that is, be assigned to a variable of type void, passed as an actual argument where the corresponding statically known formal parameter has type void, returned in a void function). Here's a CL for that: https://dart-review.googlesource.com/c/sdk/+/38060. As mentioned in that CL, it subsumes the above-mentioned CL about conditionals, and we would land at most one of them.

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Feb 8, 2018

some remaining todos:

  • land the CL that makes usages of void types errors, for a starting point that works internally
  • triage FutureOr
  • update analyzer to be fully compliant with the spec, confirm passes in g3
  • update internal/external code to no longer return void in dynamic functions
  • update analyzer/spec to disallow returning void in dynamic functions
  • experiment with making void the only top type

@MichaelRFairhurst
Copy link
Contributor

@leafpetersen @eernstg @devoncarew I think this is done enough for dart2/the purpose of this ticket. WDYT?

Much of the rest is spec work, and the void return case which could probably be its own ticket, from what I can tell.

@devoncarew
Copy link
Member

Yup, would love to split the remaining work out into a new issue, for work post the alpha2 / march 1st timeline.

@leafpetersen
Copy link
Member

The main issue will be flutter. Anything remaining will have to be run through their breaking change process, which means breakage will have to be fairly limited and well-justified.

@eernstg
Copy link
Member Author

eernstg commented Feb 14, 2018

Much of the rest is spec work ..

That would belong to #30597 (which lists all the relevant feature spec CLs, including the ones mentioned here), and it should be possible to handle this issue independently. For the record, I've added a comment on #30597 noting that some discussions here are relevant to said CLs.

@MichaelRFairhurst
Copy link
Contributor

Assuming FutureOr and Top changes are part of #30597, I made #32161 to address the work I'm still looking at as I have availability. This ticket can now be closed

@leafpetersen
Copy link
Member

There are still failing tests around this, re-opening.

@leafpetersen leafpetersen reopened this Jul 24, 2018
@leafpetersen
Copy link
Member

void_type_function_types_test/none does not parse. Small repro which shows the incorrect parse error:

typedef G2 = void Function([void]);

@leafpetersen
Copy link
Member

Also failing:
void_type_usage_test/param_boolean_and_right: MissingCompileTimeError
void_type_usage_test/param_boolean_negation: MissingCompileTimeError
void_type_usage_test/param_boolean_or_right: MissingCompileTimeError

@leafpetersen
Copy link
Member

Also failing:
void_type_usage_test/param_use_in_string_interpolation: MissingCompileTimeError # #30177

@MichaelRFairhurst
Copy link
Contributor

Closing. All tests are either passing except the issues filed in #35508, #34319, both of which are unrelated to generalized void.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

7 participants