Don't allow a #source directive in a library unit include itself

compiler/java/com/google/dart/compiler/DartCompiler.java

Index: compiler/java/com/google/dart/compiler/DartCompiler.java
diff --git a/compiler/java/com/google/dart/compiler/DartCompiler.java b/compiler/java/com/google/dart/compiler/DartCompiler.java
index 118608d70cc682108c481eb663782f3c9cc66344..c187109b7772a307f29125c235c9ab77789138ee 100644
--- a/compiler/java/com/google/dart/compiler/DartCompiler.java
+++ b/compiler/java/com/google/dart/compiler/DartCompiler.java
@@ -12,6 +12,7 @@ import com.google.dart.compiler.LibraryDeps.Dependency;
 import com.google.dart.compiler.UnitTestBatchRunner.Invocation;
 import com.google.dart.compiler.ast.DartDirective;
 import com.google.dart.compiler.ast.DartLibraryDirective;
+import com.google.dart.compiler.ast.DartSourceDirective;
 import com.google.dart.compiler.ast.DartNode;
 import com.google.dart.compiler.ast.DartUnit;
 import com.google.dart.compiler.ast.LibraryNode;
@@ -489,6 +490,7 @@ public class DartCompiler {
     private void setEntryPoint() {
       LibraryUnit lib = context.getAppLibraryUnit();
       lib.setEntryNode(new LibraryNode(MAIN_ENTRY_POINT_NAME));
+      // this ensures that if we find it, it's a top-level static element 
       Element element = lib.getElement().lookupLocalElement(MAIN_ENTRY_POINT_NAME);
       switch (ElementKind.of(element)) {
         case NONE:
@@ -498,20 +500,18 @@ public class DartCompiler {
         case METHOD:
           MethodElement methodElement = (MethodElement) element;
           Modifiers modifiers = methodElement.getModifiers();
-          if (!modifiers.isGetter() && !modifiers.isSetter()
-              && (methodElement.getParameters() == null
-                  || methodElement.getParameters().size() == 0)) {
-            lib.getElement().setEntryPoint(methodElement);
-          } else if (modifiers.isGetter()) {
+          if (modifiers.isGetter()) {

[jbrosenberg, 2011/10/11 21:32:31]

The changes in this method are some refactoring suggested awhile ago by Miguel
in an older review....

             context.compilationError(new DartCompilationError(Location.NONE,
                 DartCompilerErrorCode.ENTRY_POINT_METHOD_MAY_NOT_BE_GETTER, MAIN_ENTRY_POINT_NAME));
           } else if (modifiers.isSetter()) {
             context.compilationError(new DartCompilationError(Location.NONE,
                 DartCompilerErrorCode.ENTRY_POINT_METHOD_MAY_NOT_BE_SETTER, MAIN_ENTRY_POINT_NAME));
-          } else {
+          } else if (methodElement.getParameters().size() > 0) {
             context.compilationError(new DartCompilationError(Location.NONE,
                 DartCompilerErrorCode.ENTRY_POINT_METHOD_CANNOT_HAVE_PARAMETERS,
                 MAIN_ENTRY_POINT_NAME));
+          } else {
+            lib.getElement().setEntryPoint(methodElement);
           }
           break;
 
@@ -522,17 +522,33 @@ public class DartCompiler {
       }
     }
 
-    private boolean checkUnitForLibraryDirective(DartUnit unit) {
+    private void checkLibraryDirectives(LibraryUnit libUnit, DartUnit unit, boolean isAppLibUnit) {
+      boolean foundLibraryDirective = false;
+      boolean foundSelfSourceDirective = false;
       List<DartDirective> directives = unit.getDirectives();
-      if (directives == null || directives.size() == 0) {
-        return false;
-      }
-      for (DartDirective directive : directives) {
-        if (directive instanceof DartLibraryDirective) {
-          return true;
+      if (directives != null && directives.size() > 0) {
+        for (DartDirective directive : directives) {
+          if (directive instanceof DartLibraryDirective) {
+            foundLibraryDirective = true;
+          } else if (directive instanceof DartSourceDirective) {
+            DartSourceDirective sourceDirective = (DartSourceDirective) directive;
+            if (libUnit.getSelfSourcePath().getText()
+                .equals(sourceDirective.getSourceUri().getValue())) {
+              foundSelfSourceDirective = true;
+            }
+          }
         }
       }
-      return false;
+
+      if (!foundLibraryDirective && !isAppLibUnit) {
+        context.compilationError(new DartCompilationError(Location.NONE,

[Brian Wilkerson, 2011/10/11 22:29:16]

Please, please, please! Always pass in a non-null source when creating a
compilation error (and consider removing the one constructor that doesn't take a
source). The editor uses the source to associate the error with the right file.
Without a source we cannot open the right file when the user double-clicks on an
error in the Problems view.

+            DartCompilerErrorCode.MISSING_LIBRARY_DIRECTIVE, unit.getSourceName()));

[danrubel, 2011/10/11 22:13:22]

From the user's point of view, should the error be reported on the *.dart file
being imported indicating that it is missing a #library directive? Or is it
better to report the error on the *.dart file with the #import statement
indicating that it is trying to import a file without a #library directive? I'm
guessing that the latter approach is better because (1) the user may have
imported the wrong file rather than the file needing a #library directive, and
(2) the user can hyperlink/jump from the #import to the file being imported, but
the reverse is much more difficult.

[Brian Wilkerson, 2011/10/11 22:29:16]

As I have argued several times before, the second is *much* better from a user's
perspective. The error is not with the way the file is being declared, it is
with the way the file is being used. We would use the same criteria for
assignments to final fields. It would be hard to argue that the right error
message is "you should not declare this as final because someone wants to assign
to it".

[jbrosenberg, 2011/10/12 14:52:51]

No disagreement from me. Perhaps we have a bit more time now to update this
error reporting, I'll take a look.  Since I expect there are other places where
we don't report errors clearly, there should probably be a separate followon
effort to deprecate the one method that doesn't take a Source arg, etc.

+      }
+      if (foundSelfSourceDirective) {
+        context.compilationError(new DartCompilationError(Location.NONE,
+            DartCompilerErrorCode.ILLEGAL_DIRECTIVES_IN_SOURCED_UNIT, unit.getSourceName(), libUnit
+                .getSelfSourcePath().getText()));
+      }
     }
 
     private void compileLibraries() throws IOException {
@@ -561,21 +577,17 @@ public class DartCompiler {
               continue;
             }
 
-            if (!isAppLibUnit) {
-              // See if this unit was imported from another unit, and if so,
-              // it's required to have a #library directive
-              if (libSelfUnit == unit) {
-                if (!checkUnitForLibraryDirective(unit)) {
-                  context.compilationError(new DartCompilationError(Location.NONE,
-                      DartCompilerErrorCode.MISSING_LIBRARY_DIRECTIVE, unit.getSourceName()));
-                }
-              } else {
-                // Else it's required not to have any directives
-                if (unit.getDirectives() != null) {
-                  context.compilationError(new DartCompilationError(Location.NONE,
-                      DartCompilerErrorCode.ILLEGAL_DIRECTIVES_IN_SOURCED_UNIT, libSelfUnit
-                          .getSourceName(), unit.getSourceName()));
-                }
+            if (libSelfUnit == unit) {
+              // if it has a self unit, then it is the dart unit that
+              // corresponds to the library itself
+              checkLibraryDirectives(lib, unit, isAppLibUnit);
+            } else {
+              // it was included with a source directive, and can't have any
+              // directives
+              if (unit.getDirectives() != null) {
+                context.compilationError(new DartCompilationError(Location.NONE,
+                    DartCompilerErrorCode.ILLEGAL_DIRECTIVES_IN_SOURCED_UNIT, libSelfUnit
+                        .getSourceName(), unit.getSourceName()));
               }
             }