[Clang][ModuleMap] Add conditional parsing via requires block declaration

[Bigcheese]

Summary

Add a new form of requires called a requires block declaration.

A requires block declaration allows conditional inclusion of other
declarations within a module map.

The syntax is the same as a requires-declaration, except that it is
followed by a block. If the feature-list isn’t satisfied, then the
contents of the block is ignored and skipped over. If the feature-list
is satisfied, then the contents of the requires block are parsed as if
the requires block was not present.

This differs from a requires-declaration in that it is not an error to
import a module from within an unsatisfied requires-block-declaration
as long as another declaration of the module exists.

[iana]

clang/docs/Modules.rst

651

Is there any kind of else syntax here? Or do you just use !whatever for the else? Is something like this valid?

requires cplusplus11 {
  ...
}
requires cplusplus && !cplusplus11 {
  ...
}

Or would you nest the requires? If you wanted OR, would you just have to duplicate the requires block body?

[Bigcheese]

clang/docs/Modules.rst

651

There's currently no else syntax, although I don't think it would be hard to add. Nesting is allowed. The syntax in your example would be:

requires cplusplus11 {
  ...
}
requires cplusplus, !cplusplus11 {
  ...
}

OR currently requires duplication. The idea is that the feature list expressions just aren't that complex in practice.

[iana]

clang/docs/Modules.rst

651

Seems like you don't strictly need else then, probably fine without it as long as there are enough examples in the documentation?

[vsapsai]

Haven't really checked the code, at the moment thinking about various failure modes.

Cases that aren't tested but I suspect are valid ones:

• empty block, i.e., requires cplusplus {}
• nested blocks.

Is it possible to reference external module map from requires block? I mean that in one context the module has some extra requirements but in a different context doesn't have them.

It would be nice to have some mechanism to notify developers that includes are still performed regardless of requires. For example, in A.h we have

#include <A_cpp_feature.h>

and in module map

module A {
  header "A.h"
  requires cplusplus {
    header "A_cpp_feature.h"
  }
  export *
}

It doesn't mean the header A_cpp_feature.h is used only with cplusplus feature which is not obvious and with big headers can be hard to notice. But I feel like this request goes beyond the scope of your change, so not insisting on it.

Also it can be nice to diagnose strange conditions like requires cplusplus, !cplusplus but that's orthogonal to this feature.

clang/include/clang/Basic/DiagnosticLexKinds.td

722

s/rquires/requires/

Would it be useful to put requires into some quotation marks to show it's not a part of the sentence but used verbatim?

[Bigcheese]

Is it possible to reference external module map from requires block? I mean that in one context the module has some extra requirements but in a different context doesn't have them.

Can you provide an example where this would cause issues?

It would be nice to have some mechanism to notify developers that includes are still performed regardless of requires.

This would be nice, but is rather complicated to do given how the module map parser works right now. We would need to actually record what gets skipped by a requires block without including it in the Module. With the current design that's not easy to do.

clang/include/clang/Basic/DiagnosticLexKinds.td

722

Possibly, but I don't think we do that anywhere else. '' is always used to refer to user identifiers, "" is only used when referring to headers or strings, and I don't see any usage of ``. I have added a note so it shows up now as:

requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:3:1: error: expected '{' to start requires block
module Pony {
^
requires-block-errors.m.tmp/missing-opening-brace/module.modulemap:2:1: note: for requires block declared here
requires !cplusplus
^

Which makes it clear.

[Bigcheese]

Add testing of empty blocks and nested blocks and make the missing { error clearer.

[dexonsmith]

Drive-by comment on the docs; otherwise this sounds awesome; as long as else is easy to add later this SGTM (I'll let others do the code review).

(Although, if else {} and else requires {} would be easy to add now/soon, I feel like it'd be worth it. Modelling an else-requires chain by hand would be quite error-prone, and it might be annoying to stage the adoption separately...)

clang/docs/Modules.rst

473

Should this be *requires-block-declaration*? I'm not seeing the definition of *requires-block* anywhere.

[iana]

clang/include/clang/Basic/Module.h

249

Is static correct? (Sorry I know very little about C++)

[dexonsmith]

clang/include/clang/Basic/DiagnosticLexKinds.td

722

The updated text looks better; maybe good enough; but I wonder if it'd be more clear to diagnose as a requires-declaration at global scope. E.g., something like:

error: invalid requires declaration outside of a module
note: did you mean to add a '{' to open a block?

(maybe my wording isn't great but I hope it indicates the direction I'm suggesting)

(I don't feel strongly either way)

[Bigcheese]

Drive-by comment on the docs; otherwise this sounds awesome; as long as else is easy to add later this SGTM (I'll let others do the code review).
(Although, if else {} and else requires {} would be easy to add now/soon, I feel like it'd be worth it. Modelling an else-requires chain by hand would be quite error-prone, and it might be annoying to stage the adoption separately...)

I don't really expect users to actually need that, but adding it is pretty trivial.

clang/docs/Modules.rst

473

Yes.

clang/include/clang/Basic/DiagnosticLexKinds.td

722

That does seem better, although I'm not sure how difficult it is to emit a fixit from the modulemap parser, but I can take a look.

clang/include/clang/Basic/Module.h

249

Yeah, you don't need an instance of a Module to query this.

[Bigcheese]

• Fixed documentation typo.
• Made missing '{' diagnostic more clear.
• Use ModuleMapParser::skipUntil.

Emitting an actual fixup is kind of difficult from ModuleMapParser because the way it handles tokens makes it hard to get the source location of the end of the line containing the requires feature list.

[vsapsai]

In my testing I was able to find a case where we go around requires like

// module.modulemap
module Main {
  header "main.h"
  export *

  extern module A "extra.modulemap"
  requires nonsense {
    extern module B "extra.modulemap"
  }
}

// extra.modulemap
module Main.A {}
module Main.B {}

In this case we can do @import Main.A and @import Main.B despite unsatisfied requires in the module.modulemap. I'm not sure it is a bug but I'm not really in a position to predict the expectations of other developers, especially those not deeply familiar with module maps.

Other than this example, I haven't found any other strange requires/extern interactions. For example, attributes like [extern_c] are communicated through module/sub-module relationship and not through extern parsing location, so block requires does not affect the attributes (as far as I can tell).

clang/lib/Lex/ModuleMap.cpp

2315–2320

You can try using all_of for this computation. But I don't know without trying if it would improve readability, to be honest. If it looks clunky, I'm perfectly happy to keep the "for" loop.

3059

/*TopLevel=*/ would help understand the meaning of true.

Also I'm not sure if parseRequiresDecl parameter should have default value as we can update all the call places. But I don't have a strong opinion about that and leave the decision to you.

clang/test/Modules/requires-block.m

8

After reading the code it seems more like testing skipUntil, so I'm not sure it is a useful test. I was thinking about testing the closing brace as a token, i.e.,

requires checkClosingBraceNotAsSeparateToken {
  //}
}

If you think it doesn't add extra value, no need to add it.

[bruno]

It would be nice to have some mechanism to notify developers that includes are still performed regardless of requires

I'd like to see a module remark for whenever a match fails -Rmodule-requires-fail or something like that. I've been bitten before by a regular submodule requires not triggering, just to find out some specific subfeature was missing in the compiler invocation - it can take a while to spot that. Doesn't need to be a blocker for getting this in though.

clang/docs/Modules.rst

657

Nice doc and examples!

clang/include/clang/Basic/DiagnosticLexKinds.td

723

I'm not really too worried about a fixit here, but it would be nice if the "did you mean..." part came out of a note instead.

clang/lib/Lex/ModuleMap.cpp

2355

Too many curly braces?

[Bigcheese]

clang/lib/Lex/ModuleMap.cpp

2355

This is correct. It closes the block on line 2353.

clang/docs/Modules.rst

 Module map file
 ---------------
 A module map file consists of a series of module declarations:
 
 .. parsed-literal::
 
   *module-map-file*:
     *module-declaration**
+    *requires-block-declaration**
 
 Within a module map file, modules are referred to by a *module-id*, which uses periods to separate each part of a module's name:
 
 .. parsed-literal::
 
   *module-id*:
     *identifier* ('.' *identifier*)*
 

 The ``no_undeclared_includes`` attribute specifies that the module can only reach non-modular headers and headers from used modules. Since some headers could be present in more than one search path and map to different modules in each path, this mechanism helps clang to find the right header, i.e., prefer the one for the current module or in a submodule instead of the first usual match in the search paths.
 
 Modules can have a number of different kinds of members, each of which is described below:
 
 .. parsed-literal::
 
   *module-member*:
     *requires-declaration*
+    *requires-block-declaration*
     *header-declaration*
     *umbrella-dir-declaration*
     *submodule-declaration*
     *export-declaration*
     *export-as-declaration*
     *use-declaration*
     *link-declaration*
     *config-macros-declaration*
module std {
     }
 
     module type_traits {
       requires cplusplus11
       header "type_traits"
     }
   }
 
+Requires block declaration
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+A requires block declaration allows conditional inclusion of other declarations within a module map.
+
+.. parsed-literal::
+
+  *requires-block-declaration*:
+    ``requires`` *feature-list* '{' *requires-block-body* '}'
+
+  *requires-block-body*:
+    *module-declaration**
+    *module-member**
+
+The syntax is the same as a *requires-declaration*, except that it is followed by a block. If the *feature-list* isn't satisfied, then the contents of the block is ignored and skipped over. If the *feature-list* is satisfied, then the contents of the requires block are parsed as if the requires block was not present.
+
+This differs from a *requires-declaration* in that it is not an error to import a module from within an unsatisfied *requires-block-declaration* as long as another declaration of the module exists.
+
+**Example:** The ``std`` module can be extended to also include C++ and C++11 headers without creating a submodule using a *requires-block-declaration*:
+
+.. parsed-literal::
+
+ module std {
+    // C standard library...
+
+    requires cplusplus {
+      header "vector"
+    }
+    requires cplusplus11 {
+      header "type_traits"
+    }
+  }
+
 Header declaration
 ~~~~~~~~~~~~~~~~~~
 A header declaration specifies that a particular header is associated with the enclosing module.
 
 .. parsed-literal::
 
   *header-declaration*:
     ``private``:sub:`opt` ``textual``:sub:`opt` ``header`` *string-literal* *header-attrs*:sub:`opt`

clang/include/clang/Basic/DiagnosticLexKinds.td

def warn_pp_date_time : Warning<
   "expansion of date or time macro is not reproducible">,
   ShowInSystemHeader, DefaultIgnore, InGroup<DiagGroup<"date-time">>;
 
 // Module map parsing
 def err_mmap_unknown_token : Error<"skipping stray token">;
 def err_mmap_expected_module : Error<"expected module declaration">;
 def err_mmap_expected_module_name : Error<"expected module name">;
 def err_mmap_expected_lbrace : Error<"expected '{' to start module '%0'">;
+def err_mmap_invalid_requires : Error<
+  "invalid requires declaration outside of a module declaration, did you mean"
+  " to add '{' to open a requires block?">;
 def err_mmap_expected_rbrace : Error<"expected '}'">;
 def note_mmap_lbrace_match : Note<"to match this '{'">;
 def err_mmap_expected_rsquare : Error<"expected ']' to close attribute">;
 def note_mmap_lsquare_match : Note<"to match this ']'">;
 def err_mmap_expected_member : Error<
   "expected umbrella, header, submodule, or module export">;
 def err_mmap_expected_header : Error<"expected a header name after '%0'">;
 def err_mmap_expected_mmap_file : Error<"expected a module map file name">;

clang/include/clang/Basic/Module.h

public:
   using Requirement = std::pair<std::string, bool>;
 
   /// The set of language features required to use this module.
   ///
   /// If any of these requirements are not available, the \c IsAvailable bit
   /// will be false to indicate that this (sub)module is not available.
   SmallVector<Requirement, 2> Requirements;
 
+  /// Determine whether a translation unit built using the current
+  /// language options has the given feature.
+  static bool hasFeature(StringRef Feature, const LangOptions &LangOpts,
+                         const TargetInfo &Target);
+
   /// A module with the same name that shadows this module.
   Module *ShadowingModule = nullptr;
 
   /// Whether this module has declared itself unimportable, either because
   /// it's missing a requirement from \p Requirements or because it's been
   /// shadowed by another module.
   unsigned IsUnimportable : 1;
 

clang/lib/Basic/Module.cpp

static bool isPlatformEnvironment(const TargetInfo &Target, StringRef Feature) {
   // variant (2) the simulator is hardcoded as part of the platform name. Both
   // forms above should match for "iossimulator" requirement.
   if (Target.getTriple().isOSDarwin() && PlatformEnv.endswith("simulator"))
     return PlatformEnv == Feature || CmpPlatformEnv(PlatformEnv, Feature);
 
   return PlatformEnv == Feature;
 }
 
-/// Determine whether a translation unit built using the current
-/// language options has the given feature.
-static bool hasFeature(StringRef Feature, const LangOptions &LangOpts,
+bool Module::hasFeature(StringRef Feature, const LangOptions &LangOpts,
                         const TargetInfo &Target) {
   bool HasFeature = llvm::StringSwitch<bool>(Feature)
                         .Case("altivec", LangOpts.AltiVec)
                         .Case("blocks", LangOpts.Blocks)
                         .Case("coroutines", LangOpts.Coroutines)
                         .Case("cplusplus", LangOpts.CPlusPlus)
                         .Case("cplusplus11", LangOpts.CPlusPlus11)
                         .Case("cplusplus14", LangOpts.CPlusPlus14)
                         .Case("cplusplus17", LangOpts.CPlusPlus17)

clang/lib/Lex/ModuleMap.cpp

class ModuleMapParser {
     SourceLocation consumeToken();
 
     /// Skip tokens until we reach the a token with the given kind
     /// (or the end of the file).
     void skipUntil(MMToken::TokenKind K);
 
     using ModuleId = SmallVector<std::pair<std::string, SourceLocation>, 2>;
 
+    struct RequiresFeature {
+      bool RequiredState;
+      std::string Feature;
+    };
+    using RequiresFeatureList = SmallVector<RequiresFeature, 2>;
+
     bool parseModuleId(ModuleId &Id);
     void parseModuleDecl();
+    void parseModuleMembers();
     void parseExternModuleDecl();
-    void parseRequiresDecl();
+    void parseRequiresFeatureList(RequiresFeatureList &RFL);
+    void parseRequiresDecl(bool TopLevel = false);
+    void parseRequiresBlockBody(const RequiresFeatureList &RFL);
     void parseHeaderDecl(MMToken::TokenKind, SourceLocation LeadingLoc);
     void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
     void parseExportDecl();
     void parseExportAsDecl();
     void parseUseDecl();
     void parseLinkDecl();
     void parseConfigMacros();
     void parseConflict();

 ///
 ///   module-declaration:
 ///     'extern' 'module' module-id string-literal
 ///     'explicit'[opt] 'framework'[opt] 'module' module-id attributes[opt]
 ///       { module-member* }
 ///
 ///   module-member:
 ///     requires-declaration
+///     requires-block-declaration
 ///     header-declaration
 ///     submodule-declaration
 ///     export-declaration
 ///     export-as-declaration
 ///     link-declaration
 ///
 ///   submodule-declaration:
 ///     module-declaration
void ModuleMapParser::parseModuleDecl() {
   if (Map.HeaderInfo.getHeaderSearchOpts().ImplicitModuleMaps &&
       !Diags.isIgnored(diag::warn_mmap_mismatched_private_submodule,
                        StartLoc) &&
       !Diags.isIgnored(diag::warn_mmap_mismatched_private_module_name,
                        StartLoc) &&
       ActiveModule->ModuleMapIsPrivate)
     diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
 
+  parseModuleMembers();
+
+  if (Tok.is(MMToken::RBrace))
+    consumeToken();
+  else {
+    Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
+    Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
+    HadError = true;
+  }
+
+  // If the active module is a top-level framework, and there are no link
+  // libraries, automatically link against the framework.
+  if (ActiveModule->IsFramework && !ActiveModule->isSubFramework() &&
+      ActiveModule->LinkLibraries.empty()) {
+    inferFrameworkLink(ActiveModule, Directory, SourceMgr.getFileManager());
+  }
+
+  // If the module meets all requirements but is still unavailable, mark the
+  // whole tree as unavailable to prevent it from building.
+  if (!ActiveModule->IsAvailable && !ActiveModule->IsUnimportable &&
+      ActiveModule->Parent) {
+    ActiveModule->getTopLevelModule()->markUnavailable(/*Unimportable=*/false);
+    ActiveModule->getTopLevelModule()->MissingHeaders.append(
+        ActiveModule->MissingHeaders.begin(),
+        ActiveModule->MissingHeaders.end());
+  }
+
+  // We're done parsing this module. Pop back to the previous module.
+  ActiveModule = PreviousActiveModule;
+}
+
+void ModuleMapParser::parseModuleMembers() {
   bool Done = false;
   do {
     switch (Tok.Kind) {
     case MMToken::EndOfFile:
     case MMToken::RBrace:
       Done = true;
       break;
 
case MMToken::LinkKeyword:
       break;
 
     default:
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_member);
       consumeToken();
       break;
     }
   } while (!Done);
-
-  if (Tok.is(MMToken::RBrace))
-    consumeToken();
-  else {
-    Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
-    Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
-    HadError = true;
-  }
-
-  // If the active module is a top-level framework, and there are no link
-  // libraries, automatically link against the framework.
-  if (ActiveModule->IsFramework && !ActiveModule->isSubFramework() &&
-      ActiveModule->LinkLibraries.empty()) {
-    inferFrameworkLink(ActiveModule, Directory, SourceMgr.getFileManager());
-  }
-
-  // If the module meets all requirements but is still unavailable, mark the
-  // whole tree as unavailable to prevent it from building.
-  if (!ActiveModule->IsAvailable && !ActiveModule->IsUnimportable &&
-      ActiveModule->Parent) {
-    ActiveModule->getTopLevelModule()->markUnavailable(/*Unimportable=*/false);
-    ActiveModule->getTopLevelModule()->MissingHeaders.append(
-      ActiveModule->MissingHeaders.begin(), ActiveModule->MissingHeaders.end());
-  }
-
-  // We're done parsing this module. Pop back to the previous module.
-  ActiveModule = PreviousActiveModule;
 }
 
 /// Parse an extern module declaration.
 ///
 ///   extern module-declaration:
 ///     'extern' 'module' module-id string-literal
 void ModuleMapParser::parseExternModuleDecl() {
   assert(Tok.is(MMToken::ExternKeyword));
if (Feature == "excluded" &&
     return false;
   } else if (Feature == "cplusplus" && M->fullModuleNameIs({"IOKit", "avc"})) {
     return false;
   }
 
   return true;
 }
 
-/// Parse a requires declaration.
-///
-///   requires-declaration:
-///     'requires' feature-list
+/// Parse a requires or requires block feature-list.
 ///
 ///   feature-list:
 ///     feature ',' feature-list
 ///     feature
 ///
 ///   feature:
 ///     '!'[opt] identifier
-void ModuleMapParser::parseRequiresDecl() {
-  assert(Tok.is(MMToken::RequiresKeyword));
-
-  // Parse 'requires' keyword.
-  consumeToken();
-
-  // Parse the feature-list.
+void ModuleMapParser::parseRequiresFeatureList(RequiresFeatureList &RFL) {
   do {
     bool RequiredState = true;
     if (Tok.is(MMToken::Exclaim)) {
       RequiredState = false;
       consumeToken();
     }
 
     if (!Tok.is(MMToken::Identifier)) {
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_feature);
       HadError = true;
       return;
     }
 
     // Consume the feature name.
     std::string Feature = std::string(Tok.getString());
     consumeToken();
 
+    RFL.push_back({RequiredState, Feature});
+
+    if (!Tok.is(MMToken::Comma))
+      break;
+
+    // Consume the comma.
+    consumeToken();
+  } while (true);
+}
+
+/// Parse a requires or requires block declaration.
+///
+///   requires-declaration:
+///     'requires' feature-list
+///
+///   requires-block-declaration:
+///     'requires' feature-list '{' requires-block-body '}'
+///
+void ModuleMapParser::parseRequiresDecl(bool TopLevel) {
+  assert(Tok.is(MMToken::RequiresKeyword));
+
+  // Parse 'requires' keyword.
+  SourceLocation RequiresLoc = consumeToken();
+
+  // Parse the feature-list.
+  RequiresFeatureList RFL;
+  parseRequiresFeatureList(RFL);
+
+  if (Tok.is(MMToken::LBrace)) {
+    return parseRequiresBlockBody(RFL);
+  } else if (TopLevel) {
+    Diags.Report(RequiresLoc, diag::err_mmap_invalid_requires);
+    HadError = true;
+    return;
+  }
+
+  for (const RequiresFeature &F : RFL) {
     bool IsRequiresExcludedHack = false;
     bool ShouldAddRequirement =
-        shouldAddRequirement(ActiveModule, Feature, IsRequiresExcludedHack);
-
+        shouldAddRequirement(ActiveModule, F.Feature, IsRequiresExcludedHack);
     if (IsRequiresExcludedHack)
       UsesRequiresExcludedHack.insert(ActiveModule);
-
     if (ShouldAddRequirement) {
       // Add this feature.
-      ActiveModule->addRequirement(Feature, RequiredState, Map.LangOpts,
+      ActiveModule->addRequirement(F.Feature, F.RequiredState, Map.LangOpts,
                                    *Map.Target);
     }
+  }
+}
 
-    if (!Tok.is(MMToken::Comma))
-      break;
+/// Parse a requires block body.
+///
+///   requires-block-body:
+///     module-declaration*
+///     module-member*
+///
+void ModuleMapParser::parseRequiresBlockBody(const RequiresFeatureList &RFL) {
+  assert(Tok.is(MMToken::LBrace));
 
-    // Consume the comma.
+  // Check if the feature list is satisfied.
+  bool Satisfied = true;
+  for (const RequiresFeature &F : RFL) {
+    if (Module::hasFeature(F.Feature, Map.LangOpts, *Map.Target) !=
+        F.RequiredState)
+      Satisfied = false;
+  }
+
+  // Consume the {
+  SourceLocation LBraceLoc = consumeToken();
+
+  // Brace balanaced skipping of tokens until the end of the block.
+  if (!Satisfied) {
+    skipUntil(MMToken::RBrace);
+    if (Tok.Kind == MMToken::EndOfFile) {
+      Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
+      Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
+      HadError = true;
+      return;
+    }
+    // Eat the '}'
     consumeToken();
-  } while (true);
+    return;
+  }
+
+  while (true) {
+    switch (Tok.Kind) {
+    case MMToken::RBrace:
+      consumeToken();
+      return;
+    case MMToken::EndOfFile:
+      Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
+      Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
+      HadError = true;
+      return;
+    default:
+      // Parse the content.
+      if (!ActiveModule) {
+        parseModuleDecl();
+      } else {
+        parseModuleMembers();
+      }
+    }
+  }
 }
 
 /// Parse a header declaration.
 ///
 ///   header-declaration:
 ///     'textual'[opt] 'header' string-literal
 ///     'private' 'textual'[opt] 'header' string-literal
 ///     'exclude' 'header' string-literal
bool ModuleMapParser::parseOptionalAttributes(Attributes &Attrs) {
 
   return HadError;
 }
 
 /// Parse a module map file.
 ///
 ///   module-map-file:
 ///     module-declaration*
+///     requires-block-declaration*
 bool ModuleMapParser::parseModuleMapFile() {
   do {
     switch (Tok.Kind) {
     case MMToken::EndOfFile:
       return HadError;
 
     case MMToken::ExplicitKeyword:
     case MMToken::ExternKeyword:
     case MMToken::ModuleKeyword:
     case MMToken::FrameworkKeyword:
       parseModuleDecl();
       break;
 
+    case MMToken::RequiresKeyword:
+      parseRequiresDecl(true);
+      break;
+
     case MMToken::Comma:
     case MMToken::ConfigMacros:
     case MMToken::Conflict:
     case MMToken::Exclaim:
     case MMToken::ExcludeKeyword:
     case MMToken::ExportKeyword:
     case MMToken::ExportAsKeyword:
     case MMToken::HeaderKeyword:
     case MMToken::Identifier:
     case MMToken::LBrace:
     case MMToken::LinkKeyword:
     case MMToken::LSquare:
     case MMToken::Period:
     case MMToken::PrivateKeyword:
     case MMToken::RBrace:
     case MMToken::RSquare:
-    case MMToken::RequiresKeyword:
     case MMToken::Star:
     case MMToken::StringLiteral:
     case MMToken::IntegerLiteral:
     case MMToken::TextualKeyword:
     case MMToken::UmbrellaKeyword:
     case MMToken::UseKeyword:
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_module);
       HadError = true;

clang/test/Modules/requires-block-errors.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-closing-brace %t/missing-closing-brace.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-opening-brace %t/missing-opening-brace.m -verify
+
+//--- missing-closing-brace/module.modulemap
+
+requires !cplusplus {
+  module Pony {
+    header "Pony.h"
+  }
+
+//--- missing-closing-brace/Pony.h
+
+//--- missing-closing-brace.m
+// [e-mail]:7 {{expected '}'}}
+// [e-mail]:2 {{to match this '{'}}
+@import Pony // expected-error {{could not build module 'Pony'}}
+
+//--- missing-opening-brace/module.modulemap
+
+requires !cplusplus
+module Pony {
+  header "Pony.h"
+}
+
+//--- missing-opening-brace/Pony.h
+
+//--- missing-opening-brace.m
+// [e-mail]:2 {{invalid requires declaration outside of a module declaration, did you mean to add '{' to open a requires block?}}
+@import Pony // expected-error {{could not build module 'Pony'}}

clang/test/Modules/requires-block.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.mm
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.mm -verify
+
+//--- include/module.modulemap
+
+requires !cplusplus {
+  module A {
+    header "A.h"
+  }
+}
+
+requires cplusplus {
+  module A {
+    header "A.h"
+    header "A.hpp"
+  }
+}
+
+// Test empty blocks
+requires cplusplus {}
+requires !cplusplus {}
+
+// Test nested requires
+requires objc {
+  module B {
+    requires !cplusplus {
+      header "B.h"
+    }
+    requires cplusplus {
+      header "B.hpp"
+    }
+  }
+}
+
+//--- include/A.h
+
+typedef int A_t;
+
+//--- include/A.hpp
+
+using Adrgdrg_t = long;
+
+//--- include/B.h
+
+typedef int B_t;
+
+//--- include/B.hpp
+
+using Bdrgdrg_t = long;
+
+//--- A.m
+@import A;
+A_t a;
+Adrgdrg_t b; // expected-error {{unknown type name 'Adrgdrg_t'}}
+
+//--- A.mm
+@import A;
+A_t a;
+Adrgdrg_t b;
+
+//--- B.m
+@import B;
+B_t a;
+Bdrgdrg_t b; // expected-error {{unknown type name 'Bdrgdrg_t'}}
+
+//--- B.mm
+@import B;
+B_t a; // expected-error {{unknown type name 'B_t'}}
+Bdrgdrg_t b;