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

@@ -95,11 +95,18 @@ 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|"
+      "attribute parameter value|%error}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

@@ -543,21 +543,6 @@ class Parser {
   /// identifiers in addition to a standalone '$'.
   void diagnoseDollarIdentifier(const Token &tok, bool diagnoseDollarPrefix);
 
-  /// 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;
@@ -1821,6 +1806,84 @@ class Parser {
   void parseOptionalArgumentLabel(Identifier &name, SourceLoc &loc,
                                   bool isAttr = false);
 
+  /// 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,
+
+    /// Not allowed; this is a specially-handled keyword or identifier in an
+    /// attribute's argument list.
+    AttrParameter,
+  };
+
+  /// 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.
+  std::optional<Located<Identifier>>
+  parseModuleSelector(ModuleSelectorReason reason, StringRef declKindName = "");
+
   enum class DeclNameFlag : uint8_t {
     /// If passed, operator basenames are allowed.
     AllowOperators = 1 << 0,
@@ -1833,10 +1896,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,
@@ -1871,8 +1930,9 @@ class Parser {
   ///   unqualified-decl-name:
   ///     unqualified-decl-base-name
   ///     unqualified-decl-base-name '(' ((identifier | '_') ':') + ')'
-  DeclNameRef parseDeclNameRef(DeclNameLoc &loc, DiagRef diag,
-                               DeclNameOptions flags);
+  DeclNameRef parseDeclNameRef(
+       DeclNameLoc &loc, DiagRef diag, DeclNameOptions flags,
+       ModuleSelectorReason modSelReason = ModuleSelectorReason::Allowed);
 
   /// Parse macro expansion.
   ///

lib/Parse/ParseDecl.cpp

@@ -671,7 +671,8 @@ bool Parser::parseSpecializeAttributeArguments(
               DeclNameFlag::AllowZeroArgCompoundNames |
                   DeclNameFlag::AllowKeywordsUsingSpecialNames |
                   DeclNameFlag::AllowOperators |
-                  DeclNameFlag::AllowLowercaseAndUppercaseSelf);
+                  DeclNameFlag::AllowLowercaseAndUppercaseSelf,
+              ModuleSelectorReason::SiblingDeclName);
         }
       }
       if (ParamLabel == "spiModule") {
@@ -687,7 +688,7 @@ bool Parser::parseSpecializeAttributeArguments(
           consumeToken();
           return false;
         }
-        diagnoseAndConsumeIfModuleSelector("SPI group");
+        parseModuleSelector(ModuleSelectorReason::SPIGroup);
         auto text = Tok.getText();
         spiGroups.push_back(Context.getIdentifier(text));
         consumeToken();
@@ -1117,7 +1118,8 @@ Parser::parseImplementsAttribute(SourceLoc AtLoc, SourceLoc Loc) {
           MemberNameLoc, diag::attr_implements_expected_member_name,
           DeclNameFlag::AllowZeroArgCompoundNames |
               DeclNameFlag::AllowOperators |
-              DeclNameFlag::AllowLowercaseAndUppercaseSelf);
+              DeclNameFlag::AllowLowercaseAndUppercaseSelf,
+          ModuleSelectorReason::SiblingDeclName);
       if (!MemberName) {
         Status.setIsParseError();
       }
@@ -1588,8 +1590,7 @@ static bool parseQualifiedDeclName(Parser &P, Diag<> nameParseError,
         Parser::DeclNameFlag::AllowZeroArgCompoundNames |
             Parser::DeclNameFlag::AllowKeywordsUsingSpecialNames |
             Parser::DeclNameFlag::AllowOperators |
-            Parser::DeclNameFlag::AllowLowercaseAndUppercaseSelf |
-            Parser::DeclNameFlag::AllowModuleSelector);
+            Parser::DeclNameFlag::AllowLowercaseAndUppercaseSelf);
     // 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)
@@ -2498,8 +2499,7 @@ static std::optional<Identifier> parseSingleAttrOptionImpl(
     P.consumeToken(tok::code_complete);
   }
 
-  P.diagnoseAndConsumeIfModuleSelector("keyword in attribute arguments",
-                                       /*IsDef=*/true);
+  P.parseModuleSelector(Parser::ModuleSelectorReason::AttrParameter);
 
   StringRef parsedName = P.Tok.getText();
   if (!P.consumeIf(tok::identifier)) {
@@ -2975,7 +2975,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
       return makeParserSuccess();
     }
 
-    diagnoseAndConsumeIfModuleSelector("SPI group");
+    parseModuleSelector(ModuleSelectorReason::SPIGroup);
 
     auto text = Tok.getText();
     // An spi group name can be '_' as in @_spi(_), a specifier for implicit import of the SPI.
@@ -3540,8 +3540,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
           DeclNameFlag::AllowZeroArgCompoundNames |
               DeclNameFlag::AllowKeywordsUsingSpecialNames |
               DeclNameFlag::AllowOperators |
-              DeclNameFlag::AllowLowercaseAndUppercaseSelf |
-              DeclNameFlag::AllowModuleSelector);
+              DeclNameFlag::AllowLowercaseAndUppercaseSelf);
     }
 
     // Parse the matching ')'.
@@ -4517,8 +4516,7 @@ bool Parser::parseConventionAttributeInternal(SourceLoc atLoc, SourceLoc attrLoc
     DeclNameLoc unusedLoc;
     witnessMethodProtocol = parseDeclNameRef(
         unusedLoc, diag::convention_attribute_witness_method_expected_protocol,
-        DeclNameFlag::AllowLowercaseAndUppercaseSelf
-            | DeclNameFlag::AllowModuleSelector);
+        DeclNameFlag::AllowLowercaseAndUppercaseSelf);
   }
 
   // Parse the ')'.  We can't use parseMatchingToken if we're in
@@ -6695,27 +6693,11 @@ ParserStatus Parser::parseInheritance(
   return Status;
 }
 
-bool Parser::diagnoseAndConsumeIfModuleSelector(StringRef KindName,
-                                               bool IsDefinition) {
-  if (!Context.LangOpts.hasFeature(Feature::ModuleSelector) ||
-      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);
@@ -7559,7 +7541,7 @@ static ParameterList *parseOptionalAccessorArgument(SourceLoc SpecifierLoc,
   if (SpecifierLoc.isValid() && P.Tok.is(tok::l_paren)) {
     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,
@@ -9174,8 +9156,6 @@ Parser::parseDeclEnumCase(ParseDeclOptions Flags,
       break;
     }
 
-    diagnoseAndConsumeIfModuleSelector("enum 'case'");
-
     if (Tok.is(tok::identifier)) {
       Status |= parseIdentifierDeclName(
           *this, Name, NameLoc, "enum 'case'", [](const Token &next) {
@@ -10142,11 +10122,9 @@ Parser::parseDeclOperatorImpl(SourceLoc OperatorLoc, Identifier Name,
 
     // TODO: We could support module selectors for precedence groups if we
     //       implemented the lookup for it.
-    diagnoseAndConsumeIfModuleSelector("precedence group specifier",
-                                       /*isDef=*/false);
     groupName = parseDeclNameRef(groupLoc,
                                  diag::operator_decl_expected_precedencegroup,
-                                 {});
+                                 {}, ModuleSelectorReason::PrecedenceGroup);
 
     if (Context.TypeCheckerOpts.EnableOperatorDesignatedTypes) {
       // Designated types have been removed; consume the list (mainly for source
@@ -10241,7 +10219,7 @@ Parser::parseDeclPrecedenceGroup(ParseDeclOptions flags,
     return nullptr;
   }
 
-  diagnoseAndConsumeIfModuleSelector("precedence group");
+  parseModuleSelector(ModuleSelectorReason::PrecedenceGroup);
 
   Identifier name;
   SourceLoc nameLoc;
@@ -10446,7 +10424,8 @@ Parser::parseDeclPrecedenceGroup(ParseDeclOptions flags,
         auto name = parseDeclNameRef(nameLoc,
                                      { diag::expected_precedencegroup_relation,
                                        attrName },
-                                     {});
+                                     {},
+                                     ModuleSelectorReason::PrecedenceGroup);
         if (!name) {
           return abortBody();
         }