Add an alwaysThrows annotation to indicate that a function always throws.

Add an alwaysThrows annotation to indicate that a function always throws.

BUG=https://github.com/dart-lang/sdk/issues/17999
R=brianwilkerson@google.com

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

[srawlins, 2016-07-21 14:54:19]

srawlins@google.com changed reviewers:
+ brianwilkerson@google.com, kevmoo@google.com

[srawlins, 2016-07-21 14:54:20]

Better late than never! Bug is 2+ years old.

[Brian Wilkerson, 2016-07-21 15:39:25]

https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart
File pkg/meta/lib/meta.dart (right):

https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart#newc...
pkg/meta/lib/meta.dart:41: /// always return a / value. The annotation shows
that `fn` does always exit. In
Not sure why there's a slash on either this line or the next.

https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart#newc...
pkg/meta/lib/meta.dart:44: const _AlwaysThrows alwaysThrows = const
_AlwaysThrows();
There are two ways in which this annotation can add value. The first, which you
describe, is to tell us whether a given function always throws an exception.
However, most of the time we could automatically determine that by using static
analysis (there are edge cases where a throw appears inside a try-catch and we
cannot determine the type of the thrown object, but they're probably rare in
practice), which would be more convenient for users.

The second is to declare the developer's intent that the method ought to always
throw an exception and to allow tools to provide a hint when we know that isn't
true (the same edge cases could result in false negatives, but again, they're
probably rare). Intent is something we can't determine from static analysis. :-)
There's nothing in the above description about this second use case. Was that
intentional, or an oversight?

Also, how does this annotation interact with inheritance? For example:

class A {
  @alwaysThrows void fail() { throw 'something'; }
}

class B extends A {
  void fail() { /* do nothing */ } // Should we hint here that the contract is
being violated?
}

int f(B b) {
  b.fail(); // Should we hint here because B.fail isn't guaranteed to throw?
}

I assume we should be able to add this restriction in a subclass, but not remove
it.

[srawlins, 2016-07-22 17:21:00]

On 2016/07/21 15:39:25, Brian Wilkerson wrote:
> https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart
> File pkg/meta/lib/meta.dart (right):
> 
>
https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart#newc...
> pkg/meta/lib/meta.dart:41: /// always return a / value. The annotation shows
> that `fn` does always exit. In
> Not sure why there's a slash on either this line or the next.
> 
>
https://codereview.chromium.org/2170643003/diff/1/pkg/meta/lib/meta.dart#newc...
> pkg/meta/lib/meta.dart:44: const _AlwaysThrows alwaysThrows = const
> _AlwaysThrows();
> There are two ways in which this annotation can add value. The first, which
you
> describe, is to tell us whether a given function always throws an exception.
> However, most of the time we could automatically determine that by using
static
> analysis (there are edge cases where a throw appears inside a try-catch and we
> cannot determine the type of the thrown object, but they're probably rare in
> practice), which would be more convenient for users.
> 
> The second is to declare the developer's intent that the method ought to
always
> throw an exception and to allow tools to provide a hint when we know that
isn't
> true (the same edge cases could result in false negatives, but again, they're
> probably rare). Intent is something we can't determine from static analysis.
:-)
> There's nothing in the above description about this second use case. Was that
> intentional, or an oversight?

I like this idea. Should we write it in the comments now? Is it important to put
it in the comments?
> 
> Also, how does this annotation interact with inheritance? For example:
> 
> class A {
>   @alwaysThrows void fail() { throw 'something'; }
> }
> 
> class B extends A {
>   void fail() { /* do nothing */ } // Should we hint here that the contract is
> being violated?
> }
> 
> int f(B b) {
>   b.fail(); // Should we hint here because B.fail isn't guaranteed to throw?
> }
> 
> I assume we should be able to add this restriction in a subclass, but not
remove
> it.

I agree with this sentiment: if it was inherited, you couldn't back out, but it
seems like the developer should be able to back out. So if we want to leave a
comment about this, we'd write that it's not inherited.

[Brian Wilkerson, 2016-07-22 21:57:02]

> > The second is to declare the developer's intent that the method ought to
always
> > throw an exception and to allow tools to provide a hint when we know that
isn't
> > true (the same edge cases could result in false negatives, but again,
they're
> > probably rare). Intent is something we can't determine from static analysis.
:-)
> > There's nothing in the above description about this second use case. Was
that
> > intentional, or an oversight?
> 
> I like this idea. Should we write it in the comments now? Is it important to
put
> it in the comments?

If the intention of the annotation is to mark this kind of intention, and if
tools are allowed / expected to show violations of that intention, then yes, we
should document it.

> > I assume we should be able to add this restriction in a subclass, but not
remove
> > it.
> 
> I agree with this sentiment: if it was inherited, you couldn't back out, but
it
> seems like the developer should be able to back out. So if we want to leave a
> comment about this, we'd write that it's not inherited.

Actually, my expectation is exactly the opposite, so I think we disagree. If I
declare that invocations of the method `m` will throw an exception, then a
subclass shouldn't violate that intention. It seems analogous to me to declaring
that a method will return an `int` and then allowing a subclass to return a
`String`. I think the annotation *should* be inherited.

[srawlins, 2016-07-28 13:04:31]

On 2016/07/22 21:57:02, Brian Wilkerson wrote:
> > > The second is to declare the developer's intent that the method ought to
> always
> > > throw an exception and to allow tools to provide a hint when we know that
> isn't
> > > true (the same edge cases could result in false negatives, but again,
> they're
> > > probably rare). Intent is something we can't determine from static
analysis.
> :-)
> > > There's nothing in the above description about this second use case. Was
> that
> > > intentional, or an oversight?
> > 
> > I like this idea. Should we write it in the comments now? Is it important to
> put
> > it in the comments?
> 
> If the intention of the annotation is to mark this kind of intention, and if
> tools are allowed / expected to show violations of that intention, then yes,
we
> should document it.
> 
> > > I assume we should be able to add this restriction in a subclass, but not
> remove
> > > it.
> > 
> > I agree with this sentiment: if it was inherited, you couldn't back out, but
> it
> > seems like the developer should be able to back out. So if we want to leave
a
> > comment about this, we'd write that it's not inherited.
> 
> Actually, my expectation is exactly the opposite, so I think we disagree. If I
> declare that invocations of the method `m` will throw an exception, then a
> subclass shouldn't violate that intention. It seems analogous to me to
declaring
> that a method will return an `int` and then allowing a subclass to return a
> `String`. I think the annotation *should* be inherited.

I see. I like the reasoning. I've added comments about enforcement, and about
inheritance.

[srawlins, 2017-06-13 17:57:01]

On 2016/07/28 13:04:45, srawlins wrote:

Hi Brian, I think this is up-to-date. Would you mind reviewing again? This came
up for me again.

[Brian Wilkerson, 2017-06-13 19:35:19]

lgtm

https://codereview.chromium.org/2170643003/diff/40001/pkg/meta/lib/meta.dart
File pkg/meta/lib/meta.dart (right):

https://codereview.chromium.org/2170643003/diff/40001/pkg/meta/lib/meta.dart#...
pkg/meta/lib/meta.dart:73: /// addition, the / annotation reveals that any
statements following a call to
Please remove the '/'s in the middle of the lines.

[srawlins, 2017-06-13 19:42:05]

https://codereview.chromium.org/2170643003/diff/40001/pkg/meta/lib/meta.dart
File pkg/meta/lib/meta.dart (right):

https://codereview.chromium.org/2170643003/diff/40001/pkg/meta/lib/meta.dart#...
pkg/meta/lib/meta.dart:73: /// addition, the / annotation reveals that any
statements following a call to
On 2017/06/13 19:35:19, Brian Wilkerson wrote:
> Please remove the '/'s in the middle of the lines.

Done.

[srawlins, 2017-06-13 19:42:29]

Description was changed from

==========
Add an alwaysThrows annotation to indicate that a function always throws.

BUG=https://github.com/dart-lang/sdk/issues/17999
==========

to

==========
Add an alwaysThrows annotation to indicate that a function always throws.

BUG=https://github.com/dart-lang/sdk/issues/17999
R=brianwilkerson@google.com

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

[srawlins, 2017-06-13 19:42:30]

Committed patchset #4 (id:60001) manually as
c6d6a69a7876c2b3d637472f4f8594f18de82263 (presubmit successful).

[kevmoo, 2017-06-13 20:14:35]

Is there work planned so that calls to such a method are treated by analysis as
a throw?

[srawlins, 2017-06-13 20:50:55]

On 2017/06/13 20:14:35, kevmoo wrote:
> Is there work planned so that calls to such a method are treated by analysis
as
> a throw?

I plan to support this in the DEAD_CODE detection in analyzer. Specifically:

String someQuestion(something) {
  if (something.whatever) {
    fail("whatever was a thing");
  } else {
    return something.someString;
  }
}

Right now this gets a DEAD_CODE. Even though package:test's fail() always
throws.

Someone could also write a check that a method does, in fact, always throw. I
hadn't really planned to write this, though.