Better formalize module selector diagnostics

• The same method is now always used to parse module selectors, whether or not they are valid
• A parameter to this method describes the reason for invalidity, and is used to generate its diagnostics.
• This also helps set up future changes

Author: beccadax

include/swift/AST/DiagnosticsParse.def

@@ -93,11 +93,17 @@ ERROR(forbidden_extended_escaping_string,none,
 
 ERROR(expected_identifier_in_module_selector,none,
       "expected identifier in module selector", ())
-ERROR(module_selector_not_allowed_here,none,
-      "name cannot be qualified with module selector here", ())
-ERROR(module_selector_not_allowed_in_decl,none,
-      "%select{%1|name of %1 declaration}0 cannot be qualified with module "
-      "selector", (bool, StringRef))
+ERROR(module_selector_not_allowed,none,
+      "%select{%error|name in %1 declaration|captured variable name|"
+      "parameter name|generic parameter name|argument label|SPI group|"
+      "Objective-C name|name of sibling declaration|precedence group name"
+      "}0 cannot be qualified with a module selector",
+      (uint8_t, StringRef))
+NOTE(fixit_remove_module_selector,none,
+     "remove module selector from this name", ())
+NOTE(fixit_capture_with_explicit_name,none,
+     "explicitly capture into a variable named '%0'",
+     (StringRef))
 
 //------------------------------------------------------------------------------
 // MARK: Lexer diagnostics

include/swift/Parse/Parser.h

@@ -644,21 +644,6 @@ class Parser {
              Context.getIdentifier(tok.getText()));
   }
 
-  /// If the next two tokens appear to form a module selector, consume them and
-  /// diagnose an error. Call this method before consuming an identifier that
-  /// should *not* have a module selector before it.
-  ///
-  /// \param KindName A string describing the name or declaration being written.
-  /// \param IsDef If \c true, the identifier defines a declaration's name, and
-  /// \p KindName should be a string describing the declaration; if \c false,
-  /// the identifier does something else, and \p KindName should be a more
-  /// complete string.
-  ///
-  /// \returns true if a module selector was consumed and an error was diagnosed;
-  /// false otherwise.
-  bool
-  diagnoseAndConsumeIfModuleSelector(StringRef KindName, bool IsDef = true);
-
   /// Retrieve the location just past the end of the previous
   /// source location.
   SourceLoc getEndOfPreviousLoc() const;
@@ -1630,6 +1615,80 @@ class Parser {
   /// \param loc The location of the label (empty if it doesn't exist)
   void parseOptionalArgumentLabel(Identifier &name, SourceLoc &loc);
 
+  /// The reason we are trying to parse a module selector. Other than
+  /// \c Allowed, all reasons indicate an error should be emitted.
+  enum class ModuleSelectorReason : uint8_t {
+    /// Use of a module selector is allowed.
+    Allowed,
+
+    /// Not allowed; this is the name of a declaration. The string parameter
+    /// describes the declaration in question (e.g. a class, struct, etc.).
+    ///
+    /// If a Decl subclass also cannot have its name prefixed at its use sites
+    /// (like ParamDecls), it will have a separate reason from this one.
+    NameInDecl,
+
+    /// Not allowed; this the name of a variable being captured. Has a special
+    /// fix-it transforming `Mod::foo` into `foo = Mod::foo`.
+    Capture,
+
+    /// Not allowed; this is the name of a parameter being declared.
+    ParamDecl,
+
+    /// Not allowed; this is the name of a generic parameter being declared.
+    GenericParamDecl,
+
+    /// Not allowed; this is an argument label at either a declaration or
+    /// use site.
+    ArgumentLabel,
+
+    /// Not allowed; this is the name of an SPI group for the @_spi attribute.
+    SPIGroup,
+
+    /// Not allowed; this is an Objective-C selector or class name, so it will
+    /// be used at runtime where the module selector would have no effect.
+    ObjCName,
+
+    /// Not allowed; this is the name of a sibling to the current declaration
+    /// and has some kind of special contextual lookup.
+    ///
+    /// It may be possible to support module selectors on these in the future,
+    /// but at the moment these are all compiler-internal attributes and it
+    /// doesn't seem worth the effort.
+    SiblingDeclName,
+
+    /// Not allowed; this is the name of a precedence group, either at its
+    /// declaration site (where it makes no sense to use a module selector) or
+    /// at a use site (where it might make sense, but is not currently
+    /// implemented).
+    PrecedenceGroup,
+  };
+
+  /// Attempts to parse a \c module-selector if one is present.
+  ///
+  /// \verbatim
+  ///   module-selector: identifier '::'
+  /// \endverbatim
+  ///
+  /// At most use sites, this is actually called to parse a module selector
+  /// which should \em not be present in the source code, but which users might
+  /// plausibly write incorrectly. The \p reason argument indicates what the
+  /// name that is about to be parsed is, and therefore why there should not be
+  /// a module selector there.
+  ///
+  /// \param reason If not \c Allowed, gives a reason why a \c module-selector
+  ///        should not be present in the source here.
+  /// \param declKindName For \c ModuleSelectorReason::NameInDecl, the kind of
+  ///        declaration whose name we are parsing. Otherwise unused.
+  ///
+  /// \return \c None if no selector is present or a selector is present but
+  ///         is not allowed; an instance with an empty \c Identifier if a
+  ///         selector is present but has no valid identifier; an instance with
+  ///         a valid \c Identifier if a selector is present and includes a
+  ///         module name.
+  Optional<Located<Identifier>>
+  parseModuleSelector(ModuleSelectorReason reason, StringRef declKindName = "");
+
   enum class DeclNameFlag : uint8_t {
     /// If passed, operator basenames are allowed.
     AllowOperators = 1 << 0,
@@ -1642,10 +1701,6 @@ class Parser {
     /// not ordinary identifiers.
     AllowKeywordsUsingSpecialNames = AllowKeywords | 1 << 2,
 
-    /// If passed, a module selector (ModuleName::) is permitted before the base
-    /// name.
-    AllowModuleSelector = 1 << 3,
-
     /// If passed, compound names with argument lists are allowed, unless they
     /// have empty argument lists.
     AllowCompoundNames = 1 << 4,
@@ -1675,8 +1730,9 @@ class Parser {
   ///   unqualified-decl-name:
   ///     unqualified-decl-base-name
   ///     unqualified-decl-base-name '(' ((identifier | '_') ':') + ')'
-  DeclNameRef parseDeclNameRef(DeclNameLoc &loc, const Diagnostic &diag,
-                               DeclNameOptions flags);
+  DeclNameRef parseDeclNameRef(
+       DeclNameLoc &loc, const Diagnostic &diag, DeclNameOptions flags,
+       ModuleSelectorReason modSelReason = ModuleSelectorReason::Allowed);
 
   ParserResult<Expr> parseExprIdentifier();
   Expr *parseExprEditorPlaceholder(Token PlaceholderTok,

lib/Parse/ParseDecl.cpp

@@ -670,7 +670,8 @@ bool Parser::parseSpecializeAttributeArguments(
               targetFunctionLoc, diag::attr_specialize_expected_function,
               DeclNameFlag::AllowZeroArgCompoundNames |
                   DeclNameFlag::AllowKeywordsUsingSpecialNames |
-                  DeclNameFlag::AllowOperators);
+                  DeclNameFlag::AllowOperators,
+              ModuleSelectorReason::SiblingDeclName);
         }
       }
       if (ParamLabel == "spiModule") {
@@ -686,7 +687,7 @@ bool Parser::parseSpecializeAttributeArguments(
           consumeToken();
           return false;
         }
-        diagnoseAndConsumeIfModuleSelector("SPI group");
+        parseModuleSelector(ModuleSelectorReason::SPIGroup);
         auto text = Tok.getText();
         spiGroups.push_back(Context.getIdentifier(text));
         consumeToken();
@@ -815,7 +816,8 @@ Parser::parseImplementsAttribute(SourceLoc AtLoc, SourceLoc Loc) {
       MemberName = parseDeclNameRef(MemberNameLoc,
           diag::attr_implements_expected_member_name,
           DeclNameFlag::AllowZeroArgCompoundNames |
-          DeclNameFlag::AllowOperators);
+          DeclNameFlag::AllowOperators,
+          ModuleSelectorReason::SiblingDeclName);
       if (!MemberName) {
         Status.setIsParseError();
       }
@@ -1193,8 +1195,7 @@ static bool parseQualifiedDeclName(Parser &P, Diag<> nameParseError,
         original.Loc, nameParseError,
         Parser::DeclNameFlag::AllowZeroArgCompoundNames |
             Parser::DeclNameFlag::AllowKeywordsUsingSpecialNames |
-            Parser::DeclNameFlag::AllowOperators |
-            Parser::DeclNameFlag::AllowModuleSelector);
+            Parser::DeclNameFlag::AllowOperators);
     // The base type is optional, but the final unqualified declaration name is
     // not. If name could not be parsed, return true for error.
     if (!original.Name)
@@ -1862,7 +1863,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
       return false;
     }
 
-    diagnoseAndConsumeIfModuleSelector("SPI group");
+    parseModuleSelector(ModuleSelectorReason::SPIGroup);
 
     auto text = Tok.getText();
     spiGroups.push_back(Context.getIdentifier(text));
@@ -1974,10 +1975,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
       return false;
     }
 
-    diagnoseAndConsumeIfModuleSelector(
-        "Objective-C class name in @_swift_native_objc_runtime_base",
-        /*IsDef=*/true);
-
+    parseModuleSelector(ModuleSelectorReason::ObjCName);
     if (Tok.isNot(tok::identifier)) {
       diagnose(Loc, diag::swift_native_objc_runtime_base_must_be_identifier);
       return false;
@@ -2495,8 +2493,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
           diag::attr_dynamic_replacement_expected_function,
           DeclNameFlag::AllowZeroArgCompoundNames |
           DeclNameFlag::AllowKeywordsUsingSpecialNames |
-          DeclNameFlag::AllowOperators |
-          DeclNameFlag::AllowModuleSelector);
+          DeclNameFlag::AllowOperators);
     }
 
     // Parse the matching ')'.
@@ -2599,14 +2596,14 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
   }
 
   case DAK_ProjectedValueProperty: {
-    diagnoseAndConsumeIfModuleSelector("@_projectedValueProperty attribute");
-
     if (!consumeIf(tok::l_paren)) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return false;
     }
 
+    parseModuleSelector(ModuleSelectorReason::SiblingDeclName);
+
     if (Tok.isNot(tok::identifier)) {
       diagnose(Loc, diag::projection_value_property_not_identifier);
       return false;
@@ -3142,8 +3139,7 @@ bool Parser::parseConventionAttributeInternal(
 
     DeclNameLoc unusedLoc;
     convention.WitnessMethodProtocol = parseDeclNameRef(unusedLoc,
-        diag::convention_attribute_witness_method_expected_protocol,
-        DeclNameFlag::AllowModuleSelector);
+        diag::convention_attribute_witness_method_expected_protocol, {});
   }
 
   // Parse the ')'.  We can't use parseMatchingToken if we're in
@@ -4822,27 +4818,11 @@ ParserStatus Parser::parseInheritance(
   return Status;
 }
 
-bool Parser::diagnoseAndConsumeIfModuleSelector(StringRef KindName,
-                                               bool IsDefinition) {
-  if (!Context.LangOpts.EnableExperimentalModuleSelector ||
-      peekToken().isNot(tok::colon_colon))
-    return false;
-
-  // Diagnose an error and consume the module selector so we can continue.
-  SourceLoc start = consumeToken();
-  SourceLoc end = consumeToken(tok::colon_colon);
-  diagnose(end, diag::module_selector_not_allowed_in_decl, IsDefinition,
-           KindName)
-      .fixItRemove({start, end});
-
-  return true;
-}
-
 static ParserStatus
 parseIdentifierDeclName(Parser &P, Identifier &Result, SourceLoc &Loc,
                         StringRef DeclKindName,
                         llvm::function_ref<bool(const Token &)> canRecover) {
-  P.diagnoseAndConsumeIfModuleSelector(DeclKindName);
+  P.parseModuleSelector(Parser::ModuleSelectorReason::NameInDecl, DeclKindName);
 
   if (P.Tok.is(tok::identifier)) {
     Loc = P.consumeIdentifier(Result, /*diagnoseDollarPrefix=*/true);
@@ -5759,7 +5739,7 @@ static ParameterList *parseOptionalAccessorArgument(SourceLoc SpecifierLoc,
     SyntaxParsingContext ParamCtx(P.SyntaxContext, SyntaxKind::AccessorParameter);
     StartLoc = P.consumeToken(tok::l_paren);
 
-    P.diagnoseAndConsumeIfModuleSelector("accessor parameter");
+    P.parseModuleSelector(Parser::ModuleSelectorReason::ParamDecl);
 
     if (P.Tok.isNot(tok::identifier)) {
       P.diagnose(P.Tok, diag::expected_accessor_parameter_name,
@@ -7245,8 +7225,6 @@ Parser::parseDeclEnumCase(ParseDeclOptions Flags,
       break;
     }
 
-    diagnoseAndConsumeIfModuleSelector("enum 'case'");
-
     if (Tok.is(tok::identifier)) {
       Status |= parseIdentifierDeclName(
           *this, Name, NameLoc, "enum 'case'", [](const Token &next) {
@@ -8164,11 +8142,10 @@ parsePrecedenceGroupNameList(Parser &P, Fn takeGroupName) {
 
     // TODO: We could support module selectors for precedence groups if we
     //       implemented the lookup for it.
-    P.diagnoseAndConsumeIfModuleSelector("precedence group specifier",
-                                         /*isDef=*/false);
     DeclNameLoc nameLoc;
     auto name = P.parseDeclNameRef(nameLoc,
-        diag::expected_group_name_in_precedencegroup_list, {});
+        diag::expected_group_name_in_precedencegroup_list, {},
+        Parser::ModuleSelectorReason::PrecedenceGroup);
     if (!name)
       return makeParserError();
 
@@ -8307,7 +8284,7 @@ Parser::parseDeclPrecedenceGroup(ParseDeclOptions flags,
     return nullptr;
   }
 
-  diagnoseAndConsumeIfModuleSelector("precedence group");
+  parseModuleSelector(ModuleSelectorReason::PrecedenceGroup);
 
   Identifier name;
   SourceLoc nameLoc;