[clang][Modules] Reporting Errors for Duplicating Link Declarations in modulemaps

clang/include/clang/Basic/DiagnosticLexKinds.td

@@ -912,6 +912,11 @@ def err_mmap_nested_submodule_id : Error<
   "qualified module name can only be used to define modules at the top level">;
 def err_mmap_expected_feature : Error<"expected a feature name">;
 def err_mmap_expected_attribute : Error<"expected an attribute name">;
+def warn_mmap_link_redeclaration : Warning<"redeclaration of link library '%0'">,
+  InGroup<DiagGroup<"module-link-redeclaration">>, DefaultError;
+def note_mmap_prev_link_declaration : Note<"previously declared here">;
+def err_mmap_submodule_link_decl
+    : Error<"link declaration is not allowed in submodules">;
 def warn_mmap_unknown_attribute : Warning<"unknown attribute '%0'">,
   InGroup<IgnoredAttributes>;
 def warn_mmap_mismatched_private_submodule : Warning<

clang/lib/Lex/ModuleMapFile.cpp

@@ -118,7 +118,8 @@ struct ModuleMapFileParser {
   std::optional<ExcludeDecl> parseExcludeDecl(clang::SourceLocation LeadingLoc);
   std::optional<UmbrellaDirDecl>
   parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
-  std::optional<LinkDecl> parseLinkDecl();
+  std::optional<LinkDecl>
+  parseLinkDecl(llvm::StringMap<SourceLocation> &SeenLinkDecl, bool Allowed);
 
   SourceLocation consumeToken();
   void skipUntil(MMToken::TokenKind K);
@@ -325,6 +326,7 @@ std::optional<ModuleDecl> ModuleMapFileParser::parseModuleDecl(bool TopLevel) {
   SourceLocation LBraceLoc = consumeToken();
 
   bool Done = false;
+  llvm::StringMap<SourceLocation> SeenLinkDecl;
   do {
     std::optional<Decl> SubDecl;
     switch (Tok.Kind) {
@@ -405,7 +407,9 @@ std::optional<ModuleDecl> ModuleMapFileParser::parseModuleDecl(bool TopLevel) {
       break;
 
     case MMToken::LinkKeyword:
-      SubDecl = parseLinkDecl();
+      // Link decls are only allowed in top level modules or explicit
+      // submodules.
+      SubDecl = parseLinkDecl(SeenLinkDecl, TopLevel || MDecl.Explicit);
       break;
 
     default:
@@ -822,7 +826,8 @@ ModuleMapFileParser::parseUmbrellaDirDecl(clang::SourceLocation UmbrellaLoc) {
 ///
 ///   module-declaration:
 ///     'link' 'framework'[opt] string-literal
-std::optional<LinkDecl> ModuleMapFileParser::parseLinkDecl() {
+std::optional<LinkDecl> ModuleMapFileParser::parseLinkDecl(
+    llvm::StringMap<SourceLocation> &SeenLinkDecl, bool Allowed) {
   assert(Tok.is(MMToken::LinkKeyword));
   LinkDecl LD;
   LD.Location = consumeToken();
@@ -838,12 +843,33 @@ std::optional<LinkDecl> ModuleMapFileParser::parseLinkDecl() {
   if (!Tok.is(MMToken::StringLiteral)) {
     Diags.Report(Tok.getLocation(), diag::err_mmap_expected_library_name)
         << LD.Framework << SourceRange(LD.Location);
+    consumeToken();
     HadError = true;
     return std::nullopt;
   }
 
-  LD.Library = Tok.getString();
+  StringRef Library = Tok.getString();
+
+  LD.Library = Library;
   consumeToken();
+
+  // Make sure we eat all the tokens when we report the errors so parsing
+  // can continue.
+  if (!Allowed) {
+    Diags.Report(LD.Location, diag::err_mmap_submodule_link_decl);
+    HadError = true;
+    return std::nullopt;
+  }
+
+  auto [It, Inserted] =
+      SeenLinkDecl.insert(std::make_pair(Library, LD.Location));
+  if (!Inserted) {
+    Diags.Report(LD.Location, diag::warn_mmap_link_redeclaration) << Library;
+    Diags.Report(It->second, diag::note_mmap_prev_link_declaration);
+    HadError = true;
+    return std::nullopt;
+  }
+
   return std::move(LD);
 }
 

clang/test/ClangScanDeps/link-libraries-diag-dup.c

@@ -0,0 +1,57 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+//--- module.modulemap
+module A {
+  umbrella header "A.h"
+
+  module B {
+    header "B.h"
+    link "libraryB"
+  }
+
+  explicit module D {
+    header "D.h"
+    link "libraryD"
+  }
+
+  link "libraryA"
+  link "libraryA"
+}
+
+module C {
+  header "C.h"
+  link "libraryA"
+}
+
+//--- A.h
+#include "B.h"
+//--- B.h
+// empty
+//--- C.h
+// empty
+//--- D.h
+// empty
+//--- TU.c
+#include "A.h"
+#include "C.h"
+#include "D.h"
+
+//--- cdb.json.template
+[{
+  "file": "DIR/TU.c",
+  "directory": "DIR",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR -c DIR/TU.c"
+}]
+
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: not clang-scan-deps -compilation-database %t/cdb.json -format \
+// RUN:   experimental-full 2>&1 | FileCheck %s
+
+// Note that module D does not report an error because it is explicit.
+// Therefore we can use CHECK-NEXT for the redeclaration error on line 15.
+// CHECK:      module.modulemap:6:5:  error: link declaration is not allowed in submodules
+// CHECK-NEXT: module.modulemap:15:3: error: redeclaration of link library 'libraryA' [-Wmodule-link-redeclaration]
+// CHECK-NEXT: module.modulemap:14:3: note: previously declared here
+// CHECK-NOT:  module.modulemap:20:3: error: redeclaration of link library 'libraryA'