Make source be a target

Make source be a target

R=paulberry@google.com, scheglov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43355

[Brian Wilkerson, 2015-01-29 18:24:43]

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

[Brian Wilkerson, 2015-01-29 18:24:44]

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/test/services/t...
File pkg/analyzer/test/services/test_utils.dart (right):

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/test/services/t...
pkg/analyzer/test/services/test_utils.dart:5: library test_utils;
Sorry. Looks like this wasn't sorted/formatted before. The only real change is
on line 146/169.

[Paul Berry, 2015-01-29 18:37:55]

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/source.dart:30: abstract class AbstractSource
implements Source {
I'm confused as to why this class is needed, since the preexisting Source class
is already abstract.  Couldn't we just do one of the following?

1. Add the getter "Source get source => this" to the Source object
2. Modify the new task manager infrastructure so that instead of calling
".source" on a source target, it just uses it directly?

Also, I'm confused about the @override annotation on the "source" getter--I
don't see any "source" getter in the Source class to be overridden.

[Brian Wilkerson, 2015-01-30 16:39:19]

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/source.dart:30: abstract class AbstractSource
implements Source {
> I'm confused as to why this class is needed, since the preexisting Source
class
> is already abstract.

I think my intention would have been a lot more clear in Java where there's a
distinction between classes and interfaces. (Perhaps that shows that I need to
adjust my thinking even more when designing for Dart). It would also have been
more clear if there were a distinction between public API and internal
implementation details. So, while it's clear in my head, I'm not surprised, in
retrospect, that it's confusing in this CL.

I consider Source to be a public interface; something that should be implemented
but not extended. AbstractSource is an internal abstract class; something
designed to be extended (in order to get a default implementation of 'source')
but not implemented.

> Couldn't we just do one of the following?
> 
> 1. Add the getter "Source get source => this" to the Source object

Only if we also either (a) changed all subtypes of Source to extend Source
rather than implement it, or (b) duplicated the implementation of the getter in
every subtype. The latter would result in duplicated code. The former would blur
the line between interfaces and classes, which I'm not convinced is good from an
API design perspective.

> 2. Modify the new task manager infrastructure so that instead of calling
> ".source" on a source target, it just uses it directly?

It isn't the task manager that needs this getter. The task manager and analysis
driver should (I think) only need to deal with targets and shouldn't need to
know anything about sources. It's (a subset of) the individual tasks that need
this getter.

In my mind, all sources are targets, but not all targets are sources (an
individual method will be a target; while it has a source it isn't itself a
source). Therefore, we need some way, within a task, to convert a target into a
source.

> Also, I'm confused about the @override annotation on the "source" getter--I
> don't see any "source" getter in the Source class to be overridden.

The getter being overridden is in AnalysisTarget.

[Paul Berry, 2015-01-30 19:33:42]

On 2015/01/30 16:39:19, Brian Wilkerson wrote:
>
https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
> File pkg/analyzer/lib/src/generated/source.dart (right):
> 
>
https://codereview.chromium.org/882813007/diff/1/pkg/analyzer/lib/src/generat...
> pkg/analyzer/lib/src/generated/source.dart:30: abstract class AbstractSource
> implements Source {
> > I'm confused as to why this class is needed, since the preexisting Source
> class
> > is already abstract.
> 
> I think my intention would have been a lot more clear in Java where there's a
> distinction between classes and interfaces. (Perhaps that shows that I need to
> adjust my thinking even more when designing for Dart). It would also have been
> more clear if there were a distinction between public API and internal
> implementation details. So, while it's clear in my head, I'm not surprised, in
> retrospect, that it's confusing in this CL.
> 
> I consider Source to be a public interface; something that should be
implemented
> but not extended. AbstractSource is an internal abstract class; something
> designed to be extended (in order to get a default implementation of 'source')
> but not implemented.
> 
> > Couldn't we just do one of the following?
> > 
> > 1. Add the getter "Source get source => this" to the Source object
> 
> Only if we also either (a) changed all subtypes of Source to extend Source
> rather than implement it, or (b) duplicated the implementation of the getter
in
> every subtype. The latter would result in duplicated code. The former would
blur
> the line between interfaces and classes, which I'm not convinced is good from
an
> API design perspective.
> 
> > 2. Modify the new task manager infrastructure so that instead of calling
> > ".source" on a source target, it just uses it directly?
> 
> It isn't the task manager that needs this getter. The task manager and
analysis
> driver should (I think) only need to deal with targets and shouldn't need to
> know anything about sources. It's (a subset of) the individual tasks that need
> this getter.
> 
> In my mind, all sources are targets, but not all targets are sources (an
> individual method will be a target; while it has a source it isn't itself a
> source). Therefore, we need some way, within a task, to convert a target into
a
> source.
> 
> > Also, I'm confused about the @override annotation on the "source" getter--I
> > don't see any "source" getter in the Source class to be overridden.
> 
> The getter being overridden is in AnalysisTarget.

Ok, thanks for the explanation.  I think I now understand the distinction you're
making.

Personally I'm not fond of the change--I'm not bothered by the blurring between
interfaces and classes, and I find the additional layer in the class hierarchy
jarring.  And I feel ok about the state of the code without this CL as well. 
But I'm not sure there's much more to discuss beyond personal preference at this
point, and I don't want to slow you down unnecessarily (especially since I've
been so excited to start trying out this code).  So in the name of progress,
lgtm.

[Brian Wilkerson, 2015-01-30 20:02:29]

> Personally I'm not fond of the change--I'm not bothered by the blurring
between
> interfaces and classes, and I find the additional layer in the class hierarchy
> jarring.

I'll try getting rid of the extra class and see how it looks. Like I said,
perhaps this distinction shouldn't be as important as it currently is.

> And I feel ok about the state of the code without this CL as well.

My biggest concern without this CL (or something like it) is that we would need
to create a wrapper object for every Source that has no behavior other than
being able to return the wrapped source, and that sounds like both a performance
issue and a code smell.

> But I'm not sure there's much more to discuss beyond personal preference at
this
> point, and I don't want to slow you down unnecessarily (especially since I've
> been so excited to start trying out this code).  So in the name of progress,
> lgtm.

It's not going to slow me down. Presumably discussions will bring us to a joint
understanding and improved architecture.

[Brian Wilkerson, 2015-01-31 00:26:09]

Getting rid of AbstractSource isn't too bad. PTAL.

[Paul Berry, 2015-02-02 15:51:09]

lgtm.  Thanks.

[scheglov, 2015-02-02 16:01:22]

LGTM

[Brian Wilkerson, 2015-02-02 17:08:44]

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