[SemaObjC] Properly diagnose type arguments and protocols mix in parameterized classes

[bruno]

Summary

Under certain conditions clang currently fails to properly diagnostic ObjectC
parameter list when type args and protocols are mixed in the same list. This
happens when the first item in the parameter list is a (1) protocol, (2)
unknown type or (3) a list of protocols/unknown types up to the first type
argument. Fix the problem to report the proper error, example:

NSArray<M, NSValue *, NSURL, NSArray <id <M>>> *foo = @[@"a"];
NSNumber *bar = foo[0];
NSLog(@"%@", bar);

$ clang ...
x.m:7:13: error: angle brackets contain both a type ('NSValue') and a protocol ('M')

NSArray<M, NSValue *, NSURL, NSArray <id <M>>> *foo = @[@"a"];
        ~  ^

[manmanren]

lib/Parse/ParseObjc.cpp

1697

I am a little confused about the original comment. Have we syntactically matched a type argument? All we have done is trying to parse the single identifiers, right?

1720

Should we set foundValidTypeId here too?

1722

Should we emit diagnostics for all cases we set invalid to true?

1726

Please add comments here: not a type argument, not a protocol, treat it as unknown type.

1729

Is it necessary to have a "continue" here? Can we restructure this to get rid of the "continue"?

[bruno]

Hi Manman,

lib/Parse/ParseObjc.cpp

1697

Yes, you're right! Going to update the entire comment to be more accurate

1720

See the comment below

1722

The initial version of the patch was doing this. However, I could not come up with a testcase to exercise it, so I left it out. Gonna try harder!

1726

Ok

1729

Did this to remove some level of nested "if"s which I personally dislike, but can easily change that

[bruno]

New patch after Manman's review.

[manmanren]

LGTM.

Manman

[bruno]

Committed r266245

include/clang/Sema/Sema.h

   DeclGroupPtrTy ActOnForwardProtocolDeclaration(SourceLocation AtProtoclLoc,
                                         ArrayRef<IdentifierLocPair> IdentList,
                                         AttributeList *attrList);
 
   void FindProtocolDeclaration(bool WarnOnDeclarations, bool ForObjCContainer,
                                ArrayRef<IdentifierLocPair> ProtocolId,
                                SmallVectorImpl<Decl *> &Protocols);
 
+  void DiagnoseTypeArgsAndProtocols(IdentifierInfo *ProtocolId,
+                                    SourceLocation ProtocolLoc,
+                                    IdentifierInfo *TypeArgId,
+                                    SourceLocation TypeArgLoc,
+                                    bool SelectProtocolFirst = false);
+
   /// Given a list of identifiers (and their locations), resolve the
   /// names to either Objective-C protocol qualifiers or type
   /// arguments, as appropriate.
   void actOnObjCTypeArgsOrProtocolQualifiers(
          Scope *S,
          ParsedType baseType,
          SourceLocation lAngleLoc,
          ArrayRef<IdentifierInfo *> identifiers,

lib/Parse/ParseObjc.cpp

Actions.actOnObjCTypeArgsOrProtocolQualifiers(getCurScope(),
                                                   protocolLAngleLoc,
                                                   protocols,
                                                   protocolRAngleLoc,
                                                   warnOnIncompleteProtocols);
     return;
   }
 
   // We syntactically matched a type argument, so commit to parsing
-  // type arguments.
+  // type arguments. Finding protocol arguments after here is an error.
 
   // Convert the identifiers into type arguments.
   bool invalid = false;
+  IdentifierInfo *foundProtocolId = nullptr;
+  SourceLocation foundProtocolSrcLoc;
+  SmallVector<IdentifierInfo *, 2> unknownTypeArgs;
+  SmallVector<SourceLocation, 2> unknownTypeArgsLoc;
+
   for (unsigned i = 0, n = identifiers.size(); i != n; ++i) {
     ParsedType typeArg
       = Actions.getTypeName(*identifiers[i], identifierLocs[i], getCurScope());
     if (typeArg) {
       DeclSpec DS(AttrFactory);
       const char *prevSpec = nullptr;
       unsigned diagID;
       DS.SetTypeSpecType(TST_typename, identifierLocs[i], prevSpec, diagID,
                          typeArg, Actions.getASTContext().getPrintingPolicy());
 
       // Form a declarator to turn this into a type.
       Declarator D(DS, Declarator::TypeNameContext);
       TypeResult fullTypeArg = Actions.ActOnTypeName(getCurScope(), D);
       if (fullTypeArg.isUsable())
         typeArgs.push_back(fullTypeArg.get());
       else
         invalid = true;
     } else {
       invalid = true;
+      auto *P = Actions.LookupProtocol(identifiers[i], identifierLocs[i]);
+      if (!P) {
+        unknownTypeArgs.push_back(identifiers[i]);
+        unknownTypeArgsLoc.push_back(identifierLocs[i]);
+        continue;
+      }
+      if (!foundProtocolId) {
+        foundProtocolId = identifiers[i];
+        foundProtocolSrcLoc = identifierLocs[i];
+      }
     }
   }
 
   // Continue parsing type-names.
+  IdentifierInfo *foundValidTypeId = nullptr;
+  SourceLocation foundValidTypeSrcLoc;
   do {
+    Token CurTypeTok = Tok;
     TypeResult typeArg = ParseTypeName();
 
     // Consume the '...' for a pack expansion.
     SourceLocation ellipsisLoc;
     TryConsumeToken(tok::ellipsis, ellipsisLoc);
     if (typeArg.isUsable() && ellipsisLoc.isValid()) {
       typeArg = Actions.ActOnPackExpansion(typeArg.get(), ellipsisLoc);
     }
 
     if (typeArg.isUsable()) {
       typeArgs.push_back(typeArg.get());
+      if (!foundValidTypeId) {
+        foundValidTypeId = CurTypeTok.getIdentifierInfo();
+        foundValidTypeSrcLoc = CurTypeTok.getLocation();
+      }
     } else {
       invalid = true;
     }
   } while (TryConsumeToken(tok::comma));
 
+  // Diagnose the mix between type args and protocols.
+  if (foundProtocolId && foundValidTypeId)
+    Actions.DiagnoseTypeArgsAndProtocols(foundProtocolId, foundProtocolSrcLoc,
+                                         foundValidTypeId,
+                                         foundValidTypeSrcLoc);
+
+  // Diagnose unknown arg types.
+  ParsedType T;
+  if (unknownTypeArgs.size())
+    for (unsigned i = 0, e = unknownTypeArgsLoc.size(); i < e; ++i)
+      Actions.DiagnoseUnknownTypeName(unknownTypeArgs[i], unknownTypeArgsLoc[i],
+                                      getCurScope(), nullptr, T);
+
   // Parse the closing '>'.
   SourceLocation rAngleLoc;
   (void)ParseGreaterThanInTemplateList(rAngleLoc, consumeLastToken,
                                        /*ObjCGenericList=*/true);
 
   if (invalid) {
     typeArgs.clear();
     return;

lib/Sema/SemaDeclObjC.cpp

if (LookupKind != Sema::LookupObjCProtocolName) {
       return false;
     }
 
     return false;
   }
 };
 } // end anonymous namespace
 
+void Sema::DiagnoseTypeArgsAndProtocols(IdentifierInfo *ProtocolId,
+                                        SourceLocation ProtocolLoc,
+                                        IdentifierInfo *TypeArgId,
+                                        SourceLocation TypeArgLoc,
+                                        bool SelectProtocolFirst) {
+  Diag(TypeArgLoc, diag::err_objc_type_args_and_protocols)
+      << SelectProtocolFirst << TypeArgId << ProtocolId
+      << SourceRange(ProtocolLoc);
+}
+
 void Sema::actOnObjCTypeArgsOrProtocolQualifiers(
        Scope *S,
        ParsedType baseType,
        SourceLocation lAngleLoc,
        ArrayRef<IdentifierInfo *> identifiers,
        ArrayRef<SourceLocation> identifierLocs,
        SourceLocation rAngleLoc,
        SourceLocation &typeArgsLAngleLoc,
if (protocols[i] || typeDecls[i]) {
 
       // If we want types and we have a type declaration, there's
       // nothing more to do.
       if (lookupKind == LookupOrdinaryName && typeDecls[i])
         continue;
 
       // We have a conflict: some names refer to protocols and others
       // refer to types.
-      Diag(identifierLocs[i], diag::err_objc_type_args_and_protocols)
-        << (protocols[i] != nullptr)
-        << identifiers[i]
-        << identifiers[0]
-        << SourceRange(identifierLocs[0]);
+      DiagnoseTypeArgsAndProtocols(identifiers[0], identifierLocs[0],
+                                   identifiers[i], identifierLocs[i],
+                                   protocols[i] != nullptr);
 
       protocols.clear();
       typeArgs.clear();
       return;
     }
 
     // Perform typo correction on the name.
     TypoCorrection corrected = CorrectTypo(

test/SemaObjC/parameterized_classes.m

 
 // Typo correction: bias set by correction itself to a type.
 typedef PC4<NSObject, NSObject, ObjCStringref> typeArgs5; // expected-error{{unknown type name 'ObjCStringref'; did you mean 'ObjCStringRef'?}}
 // expected-error@-1{{type argument 'NSObject' must be a pointer (requires a '*')}}
 // expected-error@-2{{type argument 'NSObject' must be a pointer (requires a '*')}}
 
 // Type/protocol conflict.
 typedef PC4<NSCopying, ObjCStringRef> typeArgsProtocolQualsConflict1; // expected-error{{angle brackets contain both a type ('ObjCStringRef') and a protocol ('NSCopying')}}
+typedef PC4<NSCopying, NSString *> typeArgsProtocolQualsConflict2; // expected-error{{angle brackets contain both a type ('NSString') and a protocol ('NSCopying')}}
+typedef PC4<NSCopying, UnknownType, NSString *> typeArgsProtocolQualsConflict3; // expected-error{{angle brackets contain both a type ('NSString') and a protocol ('NSCopying')}} expected-error{{unknown type name 'UnknownType'}}
+typedef PC4<UnknownType, NSString *> typeArgsProtocolQualsConflict4; // expected-error{{unknown type name 'UnknownType'}}
 
 // Handling the '>>' in type argument lists.
 typedef PC4<id<NSCopying>, NSObject *, id<NSObject>> typeArgs6;
 
 // --------------------------------------------------------------------------
 // Checking type arguments.
 // --------------------------------------------------------------------------