fix #25280, treat setters as returning void in strong mode

fix #25280, treat setters as returning void in strong mode
this gives better checking for an incorrect "return" in a setter

R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/bc7843b02520fc1ccb4576fc0d0133cea7e3c6c5

[Jennifer Messerly, 2015-12-17 17:04:16]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-12-17 17:04:58]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Brian Wilkerson, 2015-12-17 17:58:00]

LGTM

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:12531:
element.returnType.isDynamic) {
We should check that the type 'dynamic' was not explicitly declared (using
hasImplicitReturnType):

    dynamic set x(_) {}

It seems like it would be strange to override an explicit type.

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:417: //
TypeResolverVisitor will have set this for top-level functions.
"this" --> "the return type"

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13223: void
test_setterWithNoVoidType() {
In addition to the explicit 'dynamic' case, we should also test an explicit
non-dynamic case.

[Bob Nystrom, 2015-12-17 18:52:08]

rnystrom@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2015-12-17 18:52:14]

Woo!

[Jennifer Messerly, 2016-01-05 00:42:44]

PTAL. I fixed setter elements to track hasImplicitReturnType, use that flag in
inference, added a few more tests, and tweaked a few other tests that were
expecting setters to always say "false" for hasImplicitReturnType.

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:12531:
element.returnType.isDynamic) {
On 2015/12/17 17:57:59, Brian Wilkerson wrote:
> We should check that the type 'dynamic' was not explicitly declared (using
> hasImplicitReturnType):
> 
>     dynamic set x(_) {}
> 
> It seems like it would be strange to override an explicit type.

Good catch! I thought it was okay because it's invalid to use explicit dynamic).
But yeah, as currently written it would have the effect of suppressing the
warning, so that's not good. Fixed and added test. After fix it should preserve
this warning behavior:


$ cat test.dart
dynamic set f(x) {}

$ dartanalyzer test.dart
Analyzing [test.dart]...
[warning] The return type of the setter must be 'void' (test.dart, line 1, col
1)

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:417: //
TypeResolverVisitor will have set this for top-level functions.
On 2015/12/17 17:57:59, Brian Wilkerson wrote:
> "this" --> "the return type"

Done & added a bit more clarity as well.

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1531313002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13223: void
test_setterWithNoVoidType() {
On 2015/12/17 17:58:00, Brian Wilkerson wrote:
> In addition to the explicit 'dynamic' case, we should also test an explicit
> non-dynamic case.

Good catch. Done & done.

[Jennifer Messerly, 2016-01-11 22:42:33]

Going to land this based on LGTM & addressed comments with new tests+code
changes as requested. But let me know if I missed anything, happy to fix.

[Jennifer Messerly, 2016-01-11 22:42:55]

Description was changed from

==========
fix #25280, treat setters as returning void in strong mode
this gives better checking for an incorrect "return" in a setter
==========

to

==========
fix #25280, treat setters as returning void in strong mode
this gives better checking for an incorrect "return" in a setter

R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bc7843b02520fc1ccb4576fc0d0133cea7e3c6c5
==========

[Jennifer Messerly, 2016-01-11 22:42:56]

Committed patchset #2 (id:40001) manually as
bc7843b02520fc1ccb4576fc0d0133cea7e3c6c5 (presubmit successful).