No errors in third-party packages (issue 22170)

No errors in third-party packages (issue 22170)

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

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

[Brian Wilkerson, 2015-03-05 18:27:58]

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

[Brian Wilkerson, 2015-03-05 18:27:58]

I'm not thrilled about having to make the default be to produce errors, but to
do otherwise would be a breaking change, the scope of which is unclear to me.

Also, this might have some performance impact, so it would be good to check
after this lands.

[scheglov, 2015-03-05 18:54:04]

LGTM

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

https://codereview.chromium.org/983733002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2601: continue;
Can this code be moved outside of the libraries cycle?

[Paul Berry, 2015-03-05 19:16:53]

lgtm assuming issue below is addressed

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

https://codereview.chromium.org/983733002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:3864: if ((!source.isInSystemLibrary
|| _generateSdkErrors)
I had to write out a truth table to determine if this was correct, and I think
it's wrong.  In the case where:
- _generateSdkErrors is true
- source.isInSystemLibrary is true
- dartEntry.explicitlyAdded is false
- _generateImplicitErrors is false
I think we should generate errors for the file (because it is in the SDK, and
the client requested SDK errors).

But the logic above evaluates to false in this case.

Suggestion: rather than trying to invent a clever boolean expression to get the
behavior we want, how about just writing out the logic we want as a sequence of
if tests, e.g.:

bool _shouldErrorsBeAnalyzed(Source source, DartEntry dartEntry) {
  if (source.isInSystemLibrary) {
    return _generateSdkErrors;
  } else if (!dartEntry.explicitlyAdded) {
    return _generateImplicitErrors;
  } else {
    return true;
  }
}

[Brian Wilkerson, 2015-03-05 19:47:58]

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

https://codereview.chromium.org/983733002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:2601: continue;
> Can this code be moved outside of the libraries cycle?

I don't think so, although it would be nice. The problem is that
'libraryDescriptors' returns descriptors beyond the three we're checking on
here. Moving the test outside the loop would therefore disable too much.

[Brian Wilkerson, 2015-03-05 19:55:42]

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

https://codereview.chromium.org/983733002/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/engine.dart:3864: if ((!source.isInSystemLibrary
|| _generateSdkErrors)
Done

[Brian Wilkerson, 2015-03-05 20:18:08]

Committed patchset #1 (id:1) manually as 44270 (presubmit successful).