generate suggestions during analysis

editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/PubVerifier.java

+/*
+ * Copyright (c) 2013, the Dart project authors.
+ * 
+ * Licensed under the Eclipse Public License v1.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ * 
+ * http://www.eclipse.org/legal/epl-v10.html
+ * 
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.google.dart.engine.internal.verifier;
+
+import com.google.dart.engine.ast.CompilationUnit;
+import com.google.dart.engine.ast.ImportDirective;
+import com.google.dart.engine.ast.StringLiteral;
+import com.google.dart.engine.ast.visitor.RecursiveASTVisitor;
+import com.google.dart.engine.element.CompilationUnitElement;
+import com.google.dart.engine.error.PubSuggestionCode;
+import com.google.dart.engine.internal.error.ErrorReporter;
+import com.google.dart.engine.source.Source;
+
+import java.io.File;
+
+/**
+ * Instances of the class {@code PubVerifier} traverse an AST structure looking for deviations from
+ * pub best practices.
+ */
+public class PubVerifier extends RecursiveASTVisitor<Void> {
+
+  /**
+   * The error reporter by which errors will be reported.
+   */
+  private final ErrorReporter errorReporter;
+
+  public PubVerifier(ErrorReporter errorReporter) {
+    this.errorReporter = errorReporter;
+  }
+
+  @Override
+  public Void visitImportDirective(ImportDirective directive) {
+
+    // Don't bother showing a suggestion if there is a more important issue to be solved.
+    StringLiteral uriLiteral = directive.getUri();
+    if (uriLiteral == null) {
+      return null;
+    }
+    String uriContent = uriLiteral.getStringValue();
+    if (uriContent == null) {
+      return null;
+    }
+    uriContent = uriContent.trim();
+
+    // Analyze the URI
+    int index = uriContent.indexOf(':');
+    String scheme;
+    String path;
+    if (index > -1) {
+      scheme = uriContent.substring(0, index);
+      path = uriContent.substring(index + 1);
+    } else {
+      scheme = "file";

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

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).

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

Done.

+      path = uriContent;
+    }
+
+    if (scheme.equals("file")) {
+      verifyFilePath(directive, uriLiteral, path);
+    } else if (scheme.equals("package")) {

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

Ditto with PackageUriResolver fields and methods.

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

Done.

+      verifyPackagePath(uriLiteral, path);
+    }
+    return null;
+  }
+
+  /**
+   * Verify that the file import follows best practices.
+   * 
+   * @param directive

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

nit: incomplete comment

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

Done.

+   * @param uriLiteral the node being analyzed (not {@code null})
+   * @param path the file path extracted from the uriLiteral
+   */
+  private void verifyFilePath(ImportDirective directive, StringLiteral uriLiteral, String path) {

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

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.

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

Done.

+    CompilationUnit unit = directive.getAncestor(CompilationUnit.class);
+    if (unit == null) {
+      return;
+    }
+    CompilationUnitElement element = unit.getElement();
+    if (element == null) {
+      return;
+    }
+    Source librarySource = element.getSource();
+    if (librarySource == null) {
+      return;
+    }
+    String fullName = librarySource.getFullName();
+    if (fullName == null) {
+      return;
+    }
+    fullName = fullName.replace(File.separatorChar, '/');
+    if (path.startsWith("lib/") || path.contains("/lib/")) {
+      if (!fullName.contains("/lib/")) {
+        // Files outside the lib directory hierarchy should not reference files inside
+        // ... use package: url instead
+        errorReporter.reportError(
+            PubSuggestionCode.FILE_IMPORT_OUTSIDE_LIB_REFERENCES_FILE_INSIDE,
+            uriLiteral);
+      }
+    } else {
+      int pathIndex = 0;
+      int fullNameIndex = fullName.length();
+      while (pathIndex < path.length() && path.substring(pathIndex).startsWith("../")) {

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

It would be more efficient to replace
"path.substring(pathIndex).startsWith("../")" with "path.startsWith("../",
pathIndex)".

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

Good suggestion! Done.

+        fullNameIndex = fullName.lastIndexOf('/', fullNameIndex);
+        if (fullNameIndex == -1) {
+          return;
+        }
+        if (fullName.substring(0, fullNameIndex).endsWith("/lib")) {

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

It isn't as clear, but would be more efficient the condition to be
  fullName.startsWith("/lib", fullNameIndex - 4)

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

Yes. Thanks!

+          // Files inside the lib directory hierarchy should not reference files outside
+          errorReporter.reportError(
+              PubSuggestionCode.FILE_IMPORT_INSIDE_LIB_REFERENCES_FILE_OUTSIDE,
+              uriLiteral);
+        }
+        pathIndex += 3;
+      }
+    }
+  }
+
+  private void verifyPackagePath(StringLiteral uriLiteral, String path) {
+    if (path.contains("/../")) {
+      // Package import should not to contain ".."
+      errorReporter.reportError(PubSuggestionCode.PACKAGE_IMPORT_CONTAINS_DOT_DOT, uriLiteral);
+    }
+  }
+}