|
|
Created:
4 years, 5 months ago by srawlins Modified:
3 years, 6 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd 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
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comments about inheritence and enforcement #Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Alphabetize #Messages
Total messages: 14 (2 generated)
srawlins@google.com changed reviewers: + brianwilkerson@google.com, kevmoo@google.com
Better late than never! Bug is 2+ years old.
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.
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.
> > 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.
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.
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.
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.
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.
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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as c6d6a69a7876c2b3d637472f4f8594f18de82263 (presubmit successful).
Message was sent while issue was closed.
Is there work planned so that calls to such a method are treated by analysis as a throw?
Message was sent while issue was closed.
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. |