[Modules] Don't swallow errors when parsing optional attributes

[davide]

Summary

I found this by visual inspection while trying to get up-to-speed on modules, so I'm still uncertain how to test it. If the current one is really the actual behaviour, then parseOptionalAttribute() can return void.

[silvas]

makes sense. can you include a testcase? I assume we are forgetting some sort of diagnostic without this patch.

Also, if learning about Clang's modules, I would start with addHeaderInclude in lib/Frontend/FrontendActions.cpp. The module map stuff ultimately boils down to that (essentially, it textually forms a header which it then more-or-less compiles as a PCH; the submodule stuff is only relevant for setting Decl::Hidden and some extra checking when importing).

[davide]

Added a test. Yes, with the patch we stop immediately if we fail to parse attributes, while the old code kept emitting diagnostics. I like the new behaviour better, but I'll defer the decision to you.

[silvas]

This makes sense. LGTM.

lib/Lex/ModuleMap.cpp

     }
   }
   
   StringRef ModuleName = Id.back().first;
   SourceLocation ModuleNameLoc = Id.back().second;
   
   // Parse the optional attribute list.
   Attributes Attrs;
-  parseOptionalAttributes(Attrs);
+  if (parseOptionalAttributes(Attrs))
+    return;
+
   
   // Parse the opening brace.
   if (!Tok.is(MMToken::LBrace)) {
     Diags.Report(Tok.getLocation(), diag::err_mmap_expected_lbrace)
       << ModuleName;
     HadError = true;
     return;
   }  

 
   // Only top-level modules can have configuration macros.
   if (ActiveModule->Parent) {
     Diags.Report(ConfigMacrosLoc, diag::err_mmap_config_macro_submodule);
   }
 
   // Parse the optional attributes.
   Attributes Attrs;
-  parseOptionalAttributes(Attrs);
+  if (parseOptionalAttributes(Attrs))
+    return;
+
   if (Attrs.IsExhaustive && !ActiveModule->Parent) {
     ActiveModule->ConfigMacrosExhaustive = true;
   }
 
   // If we don't have an identifier, we're done.
   // FIXME: Support macros with the same name as a keyword here.
   if (!Tok.is(MMToken::Identifier))
     return;

         consumeToken();
     }
     HadError = true;
     return;
   }
 
   // Parse optional attributes.
   Attributes Attrs;
-  parseOptionalAttributes(Attrs);
+  if (parseOptionalAttributes(Attrs))
+    return;
 
   if (ActiveModule) {
     // Note that we have an inferred submodule.
     ActiveModule->InferSubmodules = true;
     ActiveModule->InferredSubmoduleLoc = StarLoc;
     ActiveModule->InferExplicitSubmodules = Explicit;
   } else {
     // We'll be inferring framework modules for this directory.

test/Modules/parse-attributes.modulemap

+// RUN: rm -rf %t.modules
+// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t.modules \
+// RUN:   -fmodule-map-file=%s -I%S -include "Inputs/empty.h" \
+// RUN:   -fsyntax-only -x c++ /dev/null 2>&1 | FileCheck %s
+
+// CHECK: error: expected ']' to close attribute
+// CHECK-NOT: error: expected '{' to start module 'A'
+
+module A [system {
+  header "Inputs/empty.h"
+  private header "Inputs/empty.h"
+}