Diagnose invalid module selectors in more places

beccadax · beccadax · commit a8c5be08f611 · 2021-08-06T12:37:29.000-07:00
When a module selector does not start with an identifier, we often end up trying to parse whatever token it *does* have there as something else, which doesn't handle the module selector well. Correct this by speculatively parsing invalid selectors in various places.
diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def
@@ -94,7 +94,7 @@ ERROR(forbidden_extended_escaping_string,none,
 ERROR(expected_identifier_in_module_selector,none,
       "expected identifier in module selector", ())
 ERROR(module_selector_not_allowed,none,
-      "%select{%error|name in %1 declaration|captured variable name|"
+      "%select{%error|%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",
diff --git a/include/swift/Parse/Parser.h b/include/swift/Parse/Parser.h
@@ -1616,11 +1616,16 @@ class Parser {
   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.
+  /// \c Allowed and \c InvalidOnly, all reasons indicate an error should be
+  /// emitted.
   enum class ModuleSelectorReason : uint8_t {
     /// Use of a module selector is allowed.
     Allowed,
 
+    /// Only parse (and diagnose) invalid module selectors here; if the module
+    /// selector is valid, return \c None and leave it for later.
+    InvalidOnly,
+
     /// Not allowed; this is the name of a declaration. The string parameter
     /// describes the declaration in question (e.g. a class, struct, etc.).
     ///
diff --git a/lib/Parse/Lexer.cpp b/lib/Parse/Lexer.cpp
@@ -733,9 +733,12 @@ static bool isLeftBound(const char *tokBegin, const char *bufferBegin) {
 static bool isRightBound(const char *tokEnd, bool isLeftBound,
                          const char *codeCompletionPtr) {
   switch (*tokEnd) {
+  case ':':     // ':' is an expression separator; '::' is not
+    return tokEnd[1] == ':';
+
   case ' ': case '\r': case '\n': case '\t': // whitespace
   case ')': case ']': case '}':              // closing delimiters
-  case ',': case ';': case ':':              // expression separators
+  case ',': case ';':                        // expression separators
     return false;
 
   case '\0':
diff --git a/lib/Parse/ParseExpr.cpp b/lib/Parse/ParseExpr.cpp
@@ -520,6 +520,9 @@ static Expr *formUnaryArgument(ASTContext &context, Expr *argument) {
 ///     '&' expr-unary(Mode)
 ///
 ParserResult<Expr> Parser::parseExprUnary(Diag<> Message, bool isExprBasic) {
+  // If we have an invalid module selector, consume that first.
+  parseModuleSelector(ModuleSelectorReason::InvalidOnly);
+
   SyntaxParsingContext UnaryContext(SyntaxContext, SyntaxContextKind::Expr);
   UnresolvedDeclRefExpr *Operator;
   switch (Tok.getKind()) {
@@ -1124,6 +1127,8 @@ Parser::parseExprPostfixSuffix(ParserResult<Expr> Result, bool isExprBasic,
       Tok.setKind(tok::period);
       consumeToken();
 
+      parseModuleSelector(ModuleSelectorReason::InvalidOnly);
+
       // Handle "x.42" - a tuple index.
       if (Tok.is(tok::integer_literal)) {
         DeclNameRef name(Context.getIdentifier(Tok.getText()));
@@ -1552,6 +1557,9 @@ ParserResult<Expr> Parser::parseExprPostfix(Diag<> ID, bool isExprBasic) {
 ///     expr-selector
 ///
 ParserResult<Expr> Parser::parseExprPrimary(Diag<> ID, bool isExprBasic) {
+  // If we have an invalid module selector, consume that first.
+  parseModuleSelector(ModuleSelectorReason::InvalidOnly);
+
   SyntaxParsingContext ExprContext(SyntaxContext, SyntaxContextKind::Expr);
   switch (Tok.getKind()) {
   case tok::integer_literal: {
@@ -2280,6 +2288,13 @@ parseModuleSelector(ModuleSelectorReason reason, StringRef declKindName) {
   if (peekToken().isNot(tok::colon_colon) && Tok.isNot(tok::colon_colon))
     return None;
 
+  // Leave these paired tokens to be diagnosed by a future call to
+  // `parseModuleSelector()` at the next token, rather than consuming them as
+  // malformed module names and causing more errors later in the parser.
+  if (Tok.isAny(tok::l_paren, tok::l_brace, tok::l_angle, tok::l_square,
+                tok::r_paren, tok::r_brace, tok::r_angle, tok::r_square))
+    return None;
+
   // We will parse the selector whether or not it's allowed, then early return
   // if it's disallowed, then diagnose any other errors in what we parsed. This
   // will make sure we always consume the module selector's tokens, but don't
@@ -2288,8 +2303,15 @@ parseModuleSelector(ModuleSelectorReason reason, StringRef declKindName) {
 
   SourceLoc nameLoc;
   Identifier moduleName;
-  if (Tok.is(tok::identifier))
+  if (Tok.is(tok::identifier)) {
+    // If we are only supposed to consume invalid selectors, leave this for
+    // later.
+    if (reason == ModuleSelectorReason::InvalidOnly) {
+      return None;
+    }
+
     nameLoc = consumeIdentifier(moduleName, /*diagnoseDollarPrefix=*/true);
+  }
   else if (Tok.is(tok::colon_colon))
     // Let nameLoc and colonColonLoc both point to the tok::colon_colon.
     nameLoc = Tok.getLoc();
@@ -2298,7 +2320,8 @@ parseModuleSelector(ModuleSelectorReason reason, StringRef declKindName) {
 
   auto colonColonLoc = consumeToken(tok::colon_colon);
 
-  if (reason != ModuleSelectorReason::Allowed) {
+  if (reason != ModuleSelectorReason::Allowed &&
+      reason != ModuleSelectorReason::InvalidOnly) {
     diagnose(colonColonLoc, diag::module_selector_not_allowed,
              (uint8_t)reason, declKindName);
 
diff --git a/lib/Parse/ParseType.cpp b/lib/Parse/ParseType.cpp
@@ -332,6 +332,9 @@ ParserResult<TypeRepr> Parser::parseSILBoxType(GenericParamList *generics,
 ///
 ParserResult<TypeRepr> Parser::parseType(
     Diag<> MessageID, ParseTypeReason reason) {
+  // If we have an invalid module selector, consume that first.
+  parseModuleSelector(ModuleSelectorReason::InvalidOnly);
+
   // Start a context for creating type syntax.
   SyntaxParsingContext TypeParsingContext(SyntaxContext,
                                           SyntaxContextKind::Type);
diff --git a/test/NameLookup/module_selector.swift b/test/NameLookup/module_selector.swift
@@ -11,6 +11,7 @@
 // * Cross-import overlays
 // * Key paths
 // * Key path dynamic member lookup
+// * Custom type attributes (and coverage of type attrs generally is sparse)
 //
 // It also might not cover all combinations of name lookup paths and inputs.
 
@@ -491,6 +492,131 @@ func badModuleNames() {
   // expected-error@-1 {{'NonexistentModule::MyChildType' is not a member type of struct 'ModuleSelectorTestingKit.A'}}
 }
 
+struct BadModuleSelectorSyntax { // expected-note {{in declaration of 'BadModuleSelectorSyntax'}}
+  var a: ::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var b: (::Int)
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var c: *::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var d: _::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var e: Self::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var f: self::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var g: inout::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var h: Any::Int
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var aArray: [::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var bArray: [(::Int)]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var cArray: [*::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var dArray: [_::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var eArray: [Self::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var fArray: [self::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var gArray: [inout::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var hArray: [Any::Int]
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var aIndex: String.::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  // FIXME: This gets interpreted as a single `.*` operator; may not be ideal.
+  var cIndex: String.*::Index
+  // expected-error@-1 {{consecutive declarations on a line must be separated by ';'}}
+  // expected-error@-2 {{expected declaration}}
+
+  var dIndex: String._::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var eIndex: String.Self::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var fIndex: String.self::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var gIndex: String.inout::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  var hIndex: String.Any::Index
+  // expected-error@-1 {{expected identifier in module selector}}
+
+  func inExpr() {
+    ::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    (::print())
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    *::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    Self::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    self::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    inout::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    Any::print()
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = 1.::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = (1.::magnitude)
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    // FIXME: This gets interpreted as a single `.*` operator; may not be ideal.
+    _ = 1.*::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+    // expected-error@-2 {{cannot find operator '.*' in scope}}
+
+    _ = 1._::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = 1.Self::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = 1.self::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = 1.inout::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+
+    _ = 1.Any::magnitude
+    // expected-error@-1 {{expected identifier in module selector}}
+  }
+}
+
 @_spi(main::Private)
 // expected-error@-1 {{SPI group cannot be qualified with a module selector}} expected-note@-1 {{remove module selector from this name}} {{7-13=}}
 public struct BadImplementsAttr: CustomStringConvertible {
