Optimize parsing of qualified identifiers.

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 3470)
+++ runtime/vm/parser.cc	(working copy)
@@ -269,13 +269,11 @@
     Clear();
   }
   void Clear() {
-    is_local_scope_ident = false;
     lib_prefix = NULL;
     qualifier = NULL;
     ident_pos = 0;
     ident = NULL;
   }
-  bool is_local_scope_ident;
   LibraryPrefix* lib_prefix;
   String* qualifier;
   intptr_t ident_pos;
@@ -1909,26 +1907,25 @@
 void Parser::ParseQualIdent(QualIdent* qual_ident) {
   ASSERT(IsIdentifier());
   if (!is_top_level_) {
-    AstNode* var_or_field = NULL;
-    bool is_local_ident = ResolveIdentInLocalScope(token_index_,
-                                                   *CurrentLiteral(),
-                                                   &var_or_field);
     qual_ident->ident_pos = token_index_;
     qual_ident->ident = CurrentLiteral();
     qual_ident->lib_prefix = NULL;
     qual_ident->qualifier = NULL;
-    qual_ident->is_local_scope_ident = is_local_ident;
     ConsumeToken();
-    if (!is_local_ident && (CurrentToken() == Token::kPERIOD)) {
-      LibraryPrefix& lib_prefix = LibraryPrefix::ZoneHandle();
-      lib_prefix = current_class().LookupLibraryPrefix(*(qual_ident->ident));
-      if (!lib_prefix.IsNull()) {
-        // We have a library prefix qualified identifier.
-        ConsumeToken();  // Consume the kPERIOD token.
-        qual_ident->lib_prefix = &lib_prefix;
-        qual_ident->qualifier = qual_ident->ident;
-        qual_ident->ident_pos = token_index_;
-        qual_ident->ident = ExpectIdentifier("identifier expected after '.'");
+    if (CurrentToken() == Token::kPERIOD) {
+      if (!ResolveIdentInLocalScope(qual_ident->ident_pos,
+                                    *(qual_ident->ident),
+                                    NULL)) {
+        LibraryPrefix& lib_prefix = LibraryPrefix::ZoneHandle();
+        lib_prefix = current_class().LookupLibraryPrefix(*(qual_ident->ident));
+        if (!lib_prefix.IsNull()) {
+          // We have a library prefix qualified identifier.
+          ConsumeToken();  // Consume the kPERIOD token.
+          qual_ident->lib_prefix = &lib_prefix;
+          qual_ident->qualifier = qual_ident->ident;
+          qual_ident->ident_pos = token_index_;
+          qual_ident->ident = ExpectIdentifier("identifier expected after '.'");
+        }
       }
     }
   } else {
@@ -1936,7 +1933,6 @@
     qual_ident->ident = CurrentLiteral();
     qual_ident->lib_prefix = NULL;
     qual_ident->qualifier = NULL;
-    qual_ident->is_local_scope_ident = false;
     ConsumeToken();
     if (CurrentToken() == Token::kPERIOD) {
       ConsumeToken();  // Consume the kPERIOD token.
@@ -2643,7 +2639,7 @@
   bool is_alias_name = false;
   if (IsIdentifier() && (LookaheadToken(1) == Token::kLT)) {
     ConsumeToken();
-    if (IsTypeParameter() && (CurrentToken() == Token::kLPAREN)) {
+    if (TryParseTypeParameter() && (CurrentToken() == Token::kLPAREN)) {
       is_alias_name = true;
     }
   }
@@ -2796,11 +2792,6 @@
     const intptr_t factory_pos = token_index_;
     QualIdent factory_name;
     ParseQualIdent(&factory_name);
-    if (factory_name.is_local_scope_ident) {
-      ErrorMsg(factory_pos,
-               "using '%s' in this context is invalid",
-               factory_name.ident->ToCString());
-    }
     String& qualifier = String::Handle();
     if (factory_name.qualifier != NULL) {
       qualifier ^= factory_name.qualifier->raw();
@@ -3916,7 +3907,7 @@
 
 // Returns true if the current and next tokens can be parsed as type
 // parameters. Current token position is not saved and restored.
-bool Parser::IsTypeParameter() {
+bool Parser::TryParseTypeParameter() {
   if (CurrentToken() == Token::kLT) {
     // We are possibly looking at type parameters. Find closing ">".
     int nesting_level = 0;
@@ -3956,18 +3947,13 @@
 
 // Returns true if the next tokens can be parsed as a type with optional
 // type parameters. Current token position is not restored.
-bool Parser::IsOptionalType() {
+bool Parser::TryParseOptionalType() {
   if (CurrentToken() == Token::kIDENT) {
     QualIdent type_name;
     ParseQualIdent(&type_name);
-    // Check if the type_name has been defined as a variable in a local scope,
-    // hiding the type.
-    if (type_name.is_local_scope_ident) {
+    if (CurrentToken() == Token::kLT && !TryParseTypeParameter()) {

[hausner, 2012/01/23 18:31:24]

I've been conditioned to expect extra parens here.

[regis, 2012/01/23 19:56:23]

Done.

       return false;
     }
-    if (CurrentToken() == Token::kLT && !IsTypeParameter()) {
-      return false;
-    }
   }
   return true;
 }
@@ -3976,12 +3962,12 @@
 // Returns true if the next tokens can be parsed as a type with optional
 // type parameters, or keyword "void".
 // Current token position is not restored.
-bool Parser::IsReturnType() {
+bool Parser::TryParseReturnType() {
   if (CurrentToken() == Token::kVOID) {
     ConsumeToken();
     return true;
   } else if (CurrentToken() == Token::kIDENT) {
-    return IsOptionalType();
+    return TryParseOptionalType();
   }
   return false;
 }
@@ -4002,7 +3988,7 @@
   }
   const intptr_t saved_pos = token_index_;
   bool is_var_decl = false;
-  if (IsOptionalType()) {
+  if (TryParseOptionalType()) {
     if (IsIdentifier()) {
       ConsumeToken();
       if ((CurrentToken() == Token::kSEMICOLON) ||
@@ -4037,7 +4023,7 @@
     return true;
   }
   const intptr_t saved_pos = token_index_;
-  if (IsReturnType()) {
+  if (TryParseReturnType()) {
     if ((CurrentToken() == Token::kGET) || (CurrentToken() == Token::kSET)) {
       if (Token::IsIdentifier(LookaheadToken(1))) {  // Accessor name.
         SetPosition(saved_pos);
@@ -4058,7 +4044,7 @@
   bool is_function_literal = false;
   if (IsIdentifier() && (LookaheadToken(1) == Token::kLPAREN)) {
     ConsumeToken();  // Consume function identifier.
-  } else if (IsReturnType()) {
+  } else if (TryParseReturnType()) {
     if (!IsIdentifier()) {
       SetPosition(saved_pos);
       return false;
@@ -4089,7 +4075,7 @@
   if (IsIdentifier()) {
     if (LookaheadToken(1) == Token::kIN) {
       result = true;
-    } else if (IsOptionalType()) {
+    } else if (TryParseOptionalType()) {
       if (IsIdentifier()) {
         ConsumeToken();
       }
@@ -6437,13 +6423,14 @@
 bool Parser::ResolveIdentInLocalScope(intptr_t ident_pos,
                                       const String &ident,
                                       AstNode** node) {
-  ASSERT(node != NULL);
   TRACE_PARSER("ResolveIdentInLocalScope");
   Isolate* isolate = Isolate::Current();
   // First try to find the identifier in the nested local scopes.
   LocalVariable* local = LookupLocalScope(ident);
   if (local != NULL) {
-    *node = new LoadLocalNode(ident_pos, *local);
+    if (node != NULL) {
+      *node = new LoadLocalNode(ident_pos, *local);
+    }
     return true;
   }
 
@@ -6455,11 +6442,13 @@
     // First check if a field exists.
     field = cls.LookupField(ident);
     if (!field.IsNull()) {
-      if (!field.is_static()) {
-        CheckInstanceFieldAccess(ident_pos, ident);
-        *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
-      } else {
-        *node = GenerateStaticFieldLookup(field, ident_pos);
+      if (node != NULL) {
+        if (!field.is_static()) {
+          CheckInstanceFieldAccess(ident_pos, ident);
+          *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
+        } else {
+          *node = GenerateStaticFieldLookup(field, ident_pos);
+        }
       }
       return true;
     }
@@ -6468,8 +6457,10 @@
     func = cls.LookupFunction(ident);
     if (!func.IsNull() &&
         (func.IsDynamicFunction() || func.IsStaticFunction())) {
-      *node = new PrimaryNode(ident_pos,
-                              Function::ZoneHandle(isolate, func.raw()));
+      if (node != NULL) {
+        *node = new PrimaryNode(ident_pos,
+                                Function::ZoneHandle(isolate, func.raw()));
+      }
       return true;
     }
 
@@ -6480,42 +6471,52 @@
       if (func.IsDynamicFunction()) {
         CheckInstanceFieldAccess(ident_pos, ident);
         ASSERT(AbstractType::Handle(func.result_type()).IsResolved());

[siva, 2012/01/23 18:11:38]

The CheckInstance and ASSERT could also be inside the
conditional (if (node != NULL))?

[regis, 2012/01/23 19:56:23]

Done.

-        *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
+        if (node != NULL) {
+          *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
+        }
         return true;
       } else if (func.IsStaticFunction()) {
         ASSERT(AbstractType::Handle(func.result_type()).IsResolved());

[regis, 2012/01/23 19:56:23]

Done here too.

-        *node = new StaticGetterNode(ident_pos,
-                                     Class::ZoneHandle(isolate, cls.raw()),
-                                     ident);
+        if (node != NULL) {
+          *node = new StaticGetterNode(ident_pos,
+                                       Class::ZoneHandle(isolate, cls.raw()),
+                                       ident);
+        }
         return true;
       }
     }
     func = cls.LookupSetterFunction(ident);
     if (!func.IsNull()) {
       if (func.IsDynamicFunction()) {
-        // We create a getter node even though a getter doesn't exist as
-        // it could be followed by an assignment which will convert it to
-        // a setter node. If there is no assignment we will get an error
-        // when we try to invoke the getter.
-        CheckInstanceFieldAccess(ident_pos, ident);
-        ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
-        *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
+        if (node != NULL) {
+          // We create a getter node even though a getter doesn't exist as
+          // it could be followed by an assignment which will convert it to
+          // a setter node. If there is no assignment we will get an error
+          // when we try to invoke the getter.
+          CheckInstanceFieldAccess(ident_pos, ident);
+          ASSERT(AbstractType::Handle(func.result_type()).IsResolved());
+          *node = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
+        }
         return true;
       } else if (func.IsStaticFunction()) {
-        // We create a getter node even though a getter doesn't exist as
-        // it could be followed by an assignment which will convert it to
-        // a setter node. If there is no assignment we will get an error
-        // when we try to invoke the getter.
-        *node = new StaticGetterNode(ident_pos,
-                                     Class::ZoneHandle(isolate, cls.raw()),
-                                     ident);
+        if (node != NULL) {
+          // We create a getter node even though a getter doesn't exist as
+          // it could be followed by an assignment which will convert it to
+          // a setter node. If there is no assignment we will get an error
+          // when we try to invoke the getter.
+          *node = new StaticGetterNode(ident_pos,
+                                       Class::ZoneHandle(isolate, cls.raw()),
+                                       ident);
+        }
         return true;
       }
     }
 
     cls = cls.SuperClass();
   }
-  *node = NULL;
+  if (node != NULL) {
+    *node = NULL;
+  }
   return false;  // Not an unqualified identifier.
 }
 
@@ -6658,7 +6659,8 @@
     SkipQualIdent();
   } else {
     ParseQualIdent(&type_name);
-    if (type_name.is_local_scope_ident) {
+    if (!is_top_level_ && (type_name.qualifier == NULL) &&
+        ResolveIdentInLocalScope(type_pos, *(type_name.ident), NULL)) {

[siva, 2012/01/23 18:11:38]

Should we be reporting this as an error?

[hausner, 2012/01/23 18:31:24]

The parens around type_name.ident seem weird.

[regis, 2012/01/23 19:56:23]

They were sometimes added, sometimes not. I now removed them at every location I
could find them.

[regis, 2012/01/23 19:56:23]

Yes, we have to. The fact that the qualident is declared in the local scope
indicates that it is not a type as expected.

       ErrorMsg(type_pos, "using '%s' in this context is invalid",
                type_name.ident->ToCString());
     }
@@ -7139,7 +7141,9 @@
   const intptr_t  type_pos = token_index_;
   QualIdent type_name;
   ParseQualIdent(&type_name);
-  if (type_name.is_local_scope_ident) {
+  ASSERT(!is_top_level_);

[hausner, 2012/01/23 18:31:24]

Not sure whether you can assert this here. What if we parse code that contains a
"new List()" statement in the initializer of a top-level variable? While that is
not valid Dart code, it should not crash the compiler.

[regis, 2012/01/23 19:56:23]

As discussed, we already have top level initializers using const parsed by this
code. ParseNewOperator() is only called from ParsePrimary(), in which I added an
ASSERT(!is_top_level_);

+  if ((type_name.qualifier == NULL) &&
+      ResolveIdentInLocalScope(type_pos, *(type_name.ident), NULL)) {

[siva, 2012/01/23 18:11:38]

In the cases where current token is "." and the qualifying token is not a
library prefix we end up calling ResolveIdentInLocalScope twice.

[regis, 2012/01/23 19:56:23]

Consider this example calling a named constructor:

  var Test;
  var i = new Test.named(10);  // This should be an error.

ParseQualIdent will stop parsing at the period, because Test is a local ident,
but not a prefix. Although we see a period, we have to repeat the test to report
the error.

To make the code more efficient and robust, we would need to   remember one of 3
possible states: unchecked, local, non-local. After submitting this cl, I'll
measure if it is worth it or not.

     ErrorMsg(type_pos, "using '%s' in this context is invalid",
              type_name.ident->ToCString());
   }
@@ -7390,26 +7394,24 @@
   } else if (IsIdentifier()) {
     QualIdent qual_ident;
     ParseQualIdent(&qual_ident);
-    if (qual_ident.is_local_scope_ident) {
-      ResolveIdentInLocalScope(qual_ident.ident_pos,
-                               *qual_ident.ident,
-                               &primary);
-    } else {
-      if (qual_ident.qualifier == NULL) {
-        // This is an unqualified identifier so resolve the identifier
+    if (qual_ident.qualifier == NULL) {
+      if (!ResolveIdentInLocalScope(qual_ident.ident_pos,
+                                    *(qual_ident.ident),

[hausner, 2012/01/23 18:31:24]

The parens are unnecessary.

[regis, 2012/01/23 19:56:23]

Done.

+                                    &primary)) {
+        // This is a non-local unqualified identifier so resolve the identifier
         // locally in the main app library and all libraries imported by it.
         primary = ResolveIdentInLibraryScope(library_,
                                              qual_ident,
                                              kResolveIncludingImports);
-      } else {
-        // This is a qualified identifier with a library prefix so resolve
-        // the identifier locally in that library (we do not include the
-        // libraries imported by that library).
-        const Library& lib = Library::Handle(qual_ident.lib_prefix->library());
-        primary = ResolveIdentInLibraryScope(lib,
-                                             qual_ident,
-                                             kResolveLocally);
       }
+    } else {
+      // This is a qualified identifier with a library prefix so resolve
+      // the identifier locally in that library (we do not include the
+      // libraries imported by that library).
+      const Library& lib = Library::Handle(qual_ident.lib_prefix->library());
+      primary = ResolveIdentInLibraryScope(lib,
+                                           qual_ident,
+                                           kResolveLocally);
     }
     ASSERT(primary != NULL);
   } else if (CurrentToken() == Token::kTHIS) {