"Implement" #resource tag and various restrictions on library definitions.

dart/frog/leg/scanner/scanner_task.dart

Index: dart/frog/leg/scanner/scanner_task.dart
diff --git a/dart/frog/leg/scanner/scanner_task.dart b/dart/frog/leg/scanner/scanner_task.dart
index 74b0a97e27d114505d18464a7326522e3bd3f830..3997acdeae4131b40b815ca70cefba882927fe3d 100644
--- a/dart/frog/leg/scanner/scanner_task.dart
+++ b/dart/frog/leg/scanner/scanner_task.dart
@@ -3,6 +3,11 @@
 // BSD-style license that can be found in the LICENSE file.
 
 class ScannerTask extends CompilerTask {
+  static final int LIBRARY = 1;

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

Add comment saying that these are numbered in the order they must occur in the
source file.

+  static final int IMPORT = 2;
+  static final int SOURCE = 3;

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

The spec uses the word "include" about the production. Perhaps change SOURCE to
INCLUDE.

[ahe, 2012/03/23 08:44:35]

I prefer the word "source" because I find it confusing to have both "include"
and "import". I think this was why the keywords were originally chosen and I
consider it a problem that the spec attempts to rename the concept
inconsistently. However, "source" is still terminology from the specification
simply because that is the keyword we're using.

+  static final int RESOURCE = 4;
+
   ScannerTask(Compiler compiler) : super(compiler);
   String get name() => 'Scanner';
 
@@ -19,6 +24,16 @@ class ScannerTask extends CompilerTask {
   }
 
   void processScriptTags(LibraryElement library) {
+    int tagState = 0;

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

Name is undescriptive. It is being used to remember the currently expected tag
type, so perhaps call it something like "expectedTag".

Could start at LIBRARY?

[ahe, 2012/03/23 08:44:35]

There is something about starting a zero that helps me understand the code
better. I think the code will make more sense if I create a class named TagState
and elaborate a little in its documentation comment.

+
+    int checkTag(int currentState, ScriptTag tag) {

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

Add comment explaining that checkTag returns the [currentState] argument on
success, and otherwise the current expected tag (i.e., on error, don't change
the expectation).

Alternatively, you could return the [currentState] in both cases, so a sequence
of
#source("aaa")
#library("bbb")
#import("bbb")
would only warn about the library, and not both library and import. The real
warning could even be saying that #source and #import occur out of order (the
bug here is likely to be the source, not the rest).

+      if (tagState > currentState) {

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

Rename currentState to, e.g., newState. It's too easy to confuse it with the
current tagState.

[ahe, 2012/03/23 17:15:22]

I agree. I can't come up with anything better so I have used "value".

+        compiler.reportError(tag, 'out of order');

[ngeoffray, 2012/03/23 07:54:30]

That's news to me. Not sure I like it.

[ahe, 2012/03/23 08:44:35]

This is the same problem we have with final, static, and abstract. We wrote a
simple grammar in ANTLR and kinda forgot that we specified an explicit order.

Long term, I think we need to relax these restrictions, but right now, I think
it is acceptable to follow the strict interpretation of the grammar. We have far
more important things to trouble the VM team about.

+        return tagState;
+      }
+      return currentState;
+    }
+
     LinkBuilder<ScriptTag> imports = new LinkBuilder<ScriptTag>();
     Uri cwd = new Uri(scheme: 'file', path: compiler.currentDirectory);
     Uri base = cwd.resolve(library.script.name.toString());
@@ -28,22 +43,28 @@ class ScannerTask extends CompilerTask {
       // special constants that can be inserted into script tag strings.
       Uri resolved = base.resolve(argument.dartString.slowToString());
       if (tag.isImport()) {
+        tagState = checkTag(IMPORT, tag);
         // It is not safe to import other libraries at this point as
         // another library could then observe the current library
         // before it fully declares all the members that are sourced
         // in.
         imports.addLast(tag);
       } else if (tag.isLibrary()) {
+        tagState = checkTag(LIBRARY, tag) + 1;

[Lasse Reichstein Nielsen, 2012/03/23 08:10:59]

This is too cute.

If the check fails, you increment the expectation, so

#import()
#library()
#import()

will give an error on the second import.
You might want 
  tagState = Math.min(checkTag(LIBRARY, tag), IMPORT));

[ahe, 2012/03/23 17:15:22]

Done.

         if (library.libraryTag !== null) {
           compiler.cancel("duplicated library declaration", node: tag);
         } else {
           library.libraryTag = tag;
         }
       } else if (tag.isSource()) {
+        tagState = checkTag(SOURCE, tag);
         Script script = compiler.readScript(resolved, tag);
         CompilationUnitElement unit =
           new CompilationUnitElement(script, library);
         compiler.withCurrentElement(unit, () => scan(unit));
+      } else if (tag.isResource()) {
+        tagState = checkTag(RESOURCE, tag);
+        compiler.reportWarning(tag, 'ignoring resource tag');
       } else {
         compiler.cancel("illegal script tag: ${tag.tag}", node: tag);
       }
@@ -105,6 +126,12 @@ class ScannerTask extends CompilerTask {
 
   void importLibrary(LibraryElement library, LibraryElement imported,
                      ScriptTag tag) {
+    if (imported.tags.isEmpty()) {

[ngeoffray, 2012/03/23 07:54:30]

Please add a comment that since we're checking the order of the tags, if the
tags are not empty, we are sure there is a library tag.

You could also add a helper method in LibraryElement: hasLibraryTag.

[ahe, 2012/03/23 17:15:22]

This is actually not correct. I forgot to add the else branch.

+      compiler.withCurrentElement(library, () {
+        compiler.reportError(tag,
+                             'no #library tag found in ${imported.script.uri}');
+      });
+    }
     if (tag !== null && tag.prefix !== null) {
       SourceString prefix =
           new SourceString(tag.prefix.dartString.slowToString());