Add visitors that throw by default

Add visitors that throw by default

R=paulberry@google.com

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

[Brian Wilkerson, 2017-01-09 17:16:49]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com

[Brian Wilkerson, 2017-01-09 17:16:50]

I need to add new AstNode subclasses, which means adding new visit methods to
AstVisitor, but that breaks any clients that implement AstVisitor. AstVisitor is
typically implemented only when a subclass is required to implement all of its
methods and the author wants to know when a new visit method is added that
hasn't yet been implemented. Unfortunately, this makes it difficult for everyone
when the client is pulled into the SDK via third_party.

In order to fix this problem, I want to introduce a new visitor that throws a
run-time exception when a visit method has not been overridden. It's nicer (from
the authors' perspective) to get a compilation error than a run-time exception,
but I think this will ease our backward compatibility story.

[Paul Berry, 2017-01-09 17:36:51]

lgtm

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:559: * Clients may not extend, implement or
mix-in this class.
Consider adding a reference to ThrowingAstVisitor here, so that clients who
previously extended or implemented this class will have a hint as to what they
should do instead.

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/visitor.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/visitor.dart:1585: * visit methods.
It would also be useful for classes that implement the visitor pattern and don't
need to override all of the visit methods, because by design they don't visit
all kinds of nodes (e.g. a visitor that traverses declarations but doesn't
recurse into method bodies).

A similar comment applies to ThrowingElementVisitor.

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/element.dart:722: /*=T*/
accept/*<T>*/(ElementVisitor<dynamic/*=T*/ > visitor);
Unintentional change?

[Brian Wilkerson, 2017-01-09 18:08:07]

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/ast.dart:559: * Clients may not extend, implement or
mix-in this class.
Done

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/visitor.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/visitor.dart:1585: * visit methods.
Updated comment.

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/element.dart:722: /*=T*/
accept/*<T>*/(ElementVisitor<dynamic/*=T*/ > visitor);
Yes. The formatter appears to be inserting it, but I don't know why because it
doesn't do so elsewhere. I manually removed it, but it might reappear the next
time the file is formatted.

[Brian Wilkerson, 2017-01-09 18:08:51]

Description was changed from

==========
Add visitors that throw by default
==========

to

==========
Add visitors that throw by default

R=paulberry@google.com

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

[Brian Wilkerson, 2017-01-09 18:08:52]

Committed patchset #2 (id:20001) manually as
d1fcb1e986f48708270eef786b8e6f602db5af1a (presubmit successful).

[Paul Berry, 2017-01-09 18:17:56]

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/element.dart:722: /*=T*/
accept/*<T>*/(ElementVisitor<dynamic/*=T*/ > visitor);
On 2017/01/09 18:08:07, Brian Wilkerson wrote:
> Yes. The formatter appears to be inserting it, but I don't know why because it
> doesn't do so elsewhere. I manually removed it, but it might reappear the next
> time the file is formatted.

FYI, Bob: possible formatter bug.

[Bob Nystrom, 2017-01-11 01:19:38]

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

[Bob Nystrom, 2017-01-11 01:19:39]

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/element.dart:722: /*=T*/
accept/*<T>*/(ElementVisitor<dynamic/*=T*/ > visitor);
On 2017/01/09 18:17:56, Paul Berry wrote:
> On 2017/01/09 18:08:07, Brian Wilkerson wrote:
> > Yes. The formatter appears to be inserting it, but I don't know why because
it
> > doesn't do so elsewhere. I manually removed it, but it might reappear the
next
> > time the file is formatted.
> 
> FYI, Bob: possible formatter bug.

Hmm, yeah. It tries to handle most of the generic method comment stuff
correctly. (They are not formatted like regular /* */ comments in some ways.)
But it's not a heavily tested code path, and it's effectively deprecated now, so
I'm not inclined to improve it.

We have real syntax for generic methods. :)

[Paul Berry, 2017-01-11 04:17:09]

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/2623453003/diff/20001/pkg/analyzer/lib/dart/e...
pkg/analyzer/lib/dart/element/element.dart:722: /*=T*/
accept/*<T>*/(ElementVisitor<dynamic/*=T*/ > visitor);
On 2017/01/11 01:19:38, Bob Nystrom wrote:
> On 2017/01/09 18:17:56, Paul Berry wrote:
> > On 2017/01/09 18:08:07, Brian Wilkerson wrote:
> > > Yes. The formatter appears to be inserting it, but I don't know why
because
> it
> > > doesn't do so elsewhere. I manually removed it, but it might reappear the
> next
> > > time the file is formatted.
> > 
> > FYI, Bob: possible formatter bug.
> 
> Hmm, yeah. It tries to handle most of the generic method comment stuff
> correctly. (They are not formatted like regular /* */ comments in some ways.)
> But it's not a heavily tested code path, and it's effectively deprecated now,
so
> I'm not inclined to improve it.
> 
> We have real syntax for generic methods. :)

Fair enough.  Thanks for checking!