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

dart/frog/leg/scanner/scanner_task.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // 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';
 
   void scan(CompilationUnitElement compilationUnit) {
     measure(() {
       if (compilationUnit.kind === ElementKind.LIBRARY) {
         compiler.log("scanning library ${compilationUnit.script.name}");
       }
       scanElements(compilationUnit);
       if (compilationUnit.kind === ElementKind.LIBRARY) {
         processScriptTags(compilationUnit);
       }
     });
   }
 
   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());
     for (ScriptTag tag in library.tags.reverse()) {
       StringNode argument = tag.argument;
       // TODO(lrn): Support interpolations here. We need access to the
       // 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);
       }
     }
     // TODO(ahe): During Compiler.scanBuiltinLibraries,
     // compiler.coreLibrary is null. Clean this up when there is a
     // better way to access "dart:core".
     bool implicitlyImportCoreLibrary = compiler.coreLibrary !== null;
     for (ScriptTag tag in imports.toLink()) {
       // Now that we have processed all the source tags, it is safe to
@@ skipping 41 matching lines @@
         });
     if (newLibrary) {
       compiler.withCurrentElement(library, () => scan(library));
       compiler.onLibraryLoaded(library, uri);
     }
     return library;
   }
 
   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());
       Element e = library.find(prefix);
       if (e === null) {
         e = new PrefixElement(prefix, library, tag.getBeginToken());
         library.define(e, compiler);
       }
       if (e.kind !== ElementKind.PREFIX) {
         compiler.withCurrentElement(e, () {
@@ skipping 30 matching lines @@
   final String name = 'Diet Parser';
 
   dietParse(CompilationUnitElement compilationUnit, Token tokens) {
     measure(() {
       ElementListener listener = new ElementListener(compiler, compilationUnit);
       PartialParser parser = new PartialParser(listener);
       parser.parseUnit(tokens);
     });
   }
 }