Chaining not just element requests but also summaries access.

Chaining not just element requests but also summaries access.

We need to do this when during resynthesizing a package library we
need to create a Location referencing something in the Dart SDK.

R=paulberry@google.com
BUG=

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

[Paul Berry, 2016-01-15 16:37:38]

https://codereview.chromium.org/1585843009/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1585843009/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:134: LinkedLibrary
getLinkedSummary(String uri) {
I'm concerned about the interface contract implied by this code.  Since the
class isn't abstract, and the methods getLinkedSummary, getUnlinkedSummary, and
hasLibrarySummary have implementations, it's not obvious to the reader that the
intended use is to derive from this class and provide implementations of these
three methods.

Also, the behavior of checking the parent first is implemented in these three
methods, but since they are intended to be overridden, clients will have to be
careful to re-implement correct behavior.  This seems like it will be easy to
mess up.

How about doing something like this instead:

abstract class SummaryResynthesizer extends ElementResynthesizer {
  ...
  /**
   * Return the [LinkedLibrary] for the given [uri] or `null` if it
   * could not be found.  Caller has already checked that
   * `parent.hasLibrarySummary(uri)` returns false.
   */
  LinkedLibrary getLinkedSummary(String uri);

  /**
   * ...similar comment here...
   */
  UnlinkedUnit getUnlinkedSummary(String uri);

  /**
   * ...similar comment here...
   */
  bool hasLibrarySummary(String uri);

  LinkedLibrary _getLinkedSummaryOrThrow(String uri) {
    if (parent.hasLibrarySummary(uri)) {
      return parent._getLinkedSummaryOrThrow(uri);
    }
    LinkedLibrary summary = getLinkedSummary(uri);
    if (summary != null) {
      return summary;
    }
    throw new StateError(...);
  }

  UnlinkedUnit _getUnlinkedSummaryOrThrow(String uri) {
    ...similar implementation...
  }
}

This way it's obvious what needs to be implemented by clients, and there's no
way clients can screw up the behavior of checking the parent first.

https://codereview.chromium.org/1585843009/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:159: * Return `true` if this
resynthesizer get provide summaries of the libraries
s/get/can/

[scheglov, 2016-01-15 16:54:50]

PTAL

https://codereview.chromium.org/1585843009/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1585843009/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:134: LinkedLibrary
getLinkedSummary(String uri) {
On 2016/01/15 16:37:38, Paul Berry wrote:
> I'm concerned about the interface contract implied by this code.  Since the
> class isn't abstract, and the methods getLinkedSummary, getUnlinkedSummary,
and
> hasLibrarySummary have implementations, it's not obvious to the reader that
the
> intended use is to derive from this class and provide implementations of these
> three methods.
> 
> Also, the behavior of checking the parent first is implemented in these three
> methods, but since they are intended to be overridden, clients will have to be
> careful to re-implement correct behavior.  This seems like it will be easy to
> mess up.
> 
> How about doing something like this instead:
> 
> abstract class SummaryResynthesizer extends ElementResynthesizer {
>   ...
>   /**
>    * Return the [LinkedLibrary] for the given [uri] or `null` if it
>    * could not be found.  Caller has already checked that
>    * `parent.hasLibrarySummary(uri)` returns false.
>    */
>   LinkedLibrary getLinkedSummary(String uri);
> 
>   /**
>    * ...similar comment here...
>    */
>   UnlinkedUnit getUnlinkedSummary(String uri);
> 
>   /**
>    * ...similar comment here...
>    */
>   bool hasLibrarySummary(String uri);
> 
>   LinkedLibrary _getLinkedSummaryOrThrow(String uri) {
>     if (parent.hasLibrarySummary(uri)) {
>       return parent._getLinkedSummaryOrThrow(uri);
>     }
>     LinkedLibrary summary = getLinkedSummary(uri);
>     if (summary != null) {
>       return summary;
>     }
>     throw new StateError(...);
>   }
> 
>   UnlinkedUnit _getUnlinkedSummaryOrThrow(String uri) {
>     ...similar implementation...
>   }
> }
> 
> This way it's obvious what needs to be implemented by clients, and there's no
> way clients can screw up the behavior of checking the parent first.

Done.

[Paul Berry, 2016-01-15 17:05:41]

lgtm

[scheglov, 2016-01-15 17:07:00]

Description was changed from

==========
Chaining not just element requests but also summaries access.

We need to do this when during resynthesizing a package library we
need to create a Location referencing something in the Dart SDK.

R=paulberry@google.com
BUG=
==========

to

==========
Chaining not just element requests but also summaries access.

We need to do this when during resynthesizing a package library we
need to create a Location referencing something in the Dart SDK.

R=paulberry@google.com
BUG=

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

[scheglov, 2016-01-15 17:07:06]

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