generate suggestions during analysis

generate suggestions during analysis
During analysis, detect imports that do not follow pub best practices
and generate suggestions to be displayed in the editor.
This CL generates the suggestions while a subsequent CL will display them.
See dartbug.com/9734

R=brianwilkerson@google.com

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

[Brian Wilkerson, 2013-05-31 15:28:58]

LGTM

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorSeverity.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorSeverity.java:33:
SUGGESTION("s", "suggestion"),
nit: For consistency, "\"s\"" --> "\"S\""

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorType.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorType.java:29:
* Bad practices representing situations where the user has deviated from
recommended pub
nit: Perhaps "Bad practices representing" --> "Suggestions made in"

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/PubSuggestionCode.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/PubSuggestionCode.java:17:
* The enumeration {@code PubSuggestionCode} defines the suggestions used for
reporting reporting
"reporting reporting" --> "reporting"

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:64:
scheme = "file";
Consider using FileUriResolver.FILE_SCHEME (yes, we'd need to make it public,
but I'm OK with that) or even FileUriResolver.isFileUri(URI) (although that
would require creating a URI from the uriContent which, while less efficient,
might be good anyway to catch "malformed URI" errors that have already been
reported so that we don't report this suggestion in those cases).

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:70:
} else if (scheme.equals("package")) {
Ditto with PackageUriResolver fields and methods.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:79:
* @param directive
nit: incomplete comment

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:83:
private void verifyFilePath(ImportDirective directive, StringLiteral uriLiteral,
String path) {
nit: The convention in ErrorVerifier is for the private methods to return a
boolean value indicating whether an error was reported (which allows us to not
report multiple errors for the same invalid code) and to be named
"checkFor<error code name>" or "checkForAll<common prefix of error codes>". It
might be good to follow the same convention here so that maintainers need to
learn fewer conventions.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:112:
while (pathIndex < path.length() && path.substring(pathIndex).startsWith("../"))
{
It would be more efficient to replace
"path.substring(pathIndex).startsWith("../")" with "path.startsWith("../",
pathIndex)".

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:117:
if (fullName.substring(0, fullNameIndex).endsWith("/lib")) {
It isn't as clear, but would be more efficient the condition to be
  fullName.startsWith("/lib", fullNameIndex - 4)

[jwren, 2013-05-31 17:38:12]

Pending Brian's comments LGTM.

[danrubel, 2013-05-31 19:43:45]

PTAL....

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorSeverity.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorSeverity.java:33:
SUGGESTION("s", "suggestion"),
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> nit: For consistency, "\"s\"" --> "\"S\""

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorType.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/ErrorType.java:29:
* Bad practices representing situations where the user has deviated from
recommended pub
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> nit: Perhaps "Bad practices representing" --> "Suggestions made in"

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/PubSuggestionCode.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/PubSuggestionCode.java:17:
* The enumeration {@code PubSuggestionCode} defines the suggestions used for
reporting reporting
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> "reporting reporting" --> "reporting"

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java
(right):

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:64:
scheme = "file";
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> Consider using FileUriResolver.FILE_SCHEME (yes, we'd need to make it public,
> but I'm OK with that) or even FileUriResolver.isFileUri(URI) (although that
> would require creating a URI from the uriContent which, while less efficient,
> might be good anyway to catch "malformed URI" errors that have already been
> reported so that we don't report this suggestion in those cases).

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:70:
} else if (scheme.equals("package")) {
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> Ditto with PackageUriResolver fields and methods.

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:79:
* @param directive
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> nit: incomplete comment

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:83:
private void verifyFilePath(ImportDirective directive, StringLiteral uriLiteral,
String path) {
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> nit: The convention in ErrorVerifier is for the private methods to return a
> boolean value indicating whether an error was reported (which allows us to not
> report multiple errors for the same invalid code) and to be named
> "checkFor<error code name>" or "checkForAll<common prefix of error codes>". It
> might be good to follow the same convention here so that maintainers need to
> learn fewer conventions.

Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:112:
while (pathIndex < path.length() && path.substring(pathIndex).startsWith("../"))
{
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> It would be more efficient to replace
> "path.substring(pathIndex).startsWith("../")" with "path.startsWith("../",
> pathIndex)".

Good suggestion! Done.

https://codereview.chromium.org/15736020/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:117:
if (fullName.substring(0, fullNameIndex).endsWith("/lib")) {
On 2013/05/31 15:28:58, Brian Wilkerson wrote:
> It isn't as clear, but would be more efficient the condition to be
>   fullName.startsWith("/lib", fullNameIndex - 4)

Yes. Thanks!

[Brian Wilkerson, 2013-05-31 19:56:20]

LGTM

https://codereview.chromium.org/15736020/diff/7001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java
(right):

https://codereview.chromium.org/15736020/diff/7001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java:72:
if (checkForFileImportOutsideLibReferencesFileInside(directive, path)) {
Sorry, I probably wasn't very clear. The error reporting generally happens in
the "checkFor" methods, they return a boolean indicating whether an error was
reported, not whether an error should be reported. The idea, though, was to be
able to know whether one check produced an error so that you could make other
checks conditional on it. Either way, we can address it later if you'd prefer.

[danrubel, 2013-06-03 19:54:56]

Committed patchset #4 manually as r23562 (presubmit successful).