[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

1696–1699

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?

1722

Should we set foundValidTypeId here too?

1728–1731

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

1735

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

1738

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

1696–1699

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

1722

See the comment below

1728–1731

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!

1735

Ok

1738

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(),
                                                   typeArgsRAngleLoc,
                                                   protocolLAngleLoc,
                                                   protocols,
                                                   protocolRAngleLoc,
                                                   warnOnIncompleteProtocols);
     return;
   }
 
-  // We syntactically matched a type argument, so commit to parsing
-  // type arguments.
+  // We parsed an identifier list but stumbled into non single identifiers, this
+  // means we might (a) check that what we already parsed is a legitimate type
+  // (not a protocol or unknown type) and (b) parse the remaining ones, which
+  // must all be type args.
 
   // Convert the identifiers into type arguments.
   bool invalid = false;
+  IdentifierInfo *foundProtocolId = nullptr, *foundValidTypeId = nullptr;
+  SourceLocation foundProtocolSrcLoc, foundValidTypeSrcLoc;
+  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())
+      if (fullTypeArg.isUsable()) {
         typeArgs.push_back(fullTypeArg.get());
-      else
+        if (!foundValidTypeId) {
+          foundValidTypeId = identifiers[i];
+          foundValidTypeSrcLoc = identifierLocs[i];
+        }
+      } else {
         invalid = true;
+        unknownTypeArgs.push_back(identifiers[i]);
+        unknownTypeArgsLoc.push_back(identifierLocs[i]);
+      }
     } else {
       invalid = true;
+      if (!Actions.LookupProtocol(identifiers[i], identifierLocs[i])) {
+        unknownTypeArgs.push_back(identifiers[i]);
+        unknownTypeArgsLoc.push_back(identifierLocs[i]);
+      } else if (!foundProtocolId) {
+        foundProtocolId = identifiers[i];
+        foundProtocolSrcLoc = identifierLocs[i];
+      }
     }
   }
 
   // Continue parsing type-names.
   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'}}
+typedef PC4<NSString, NSCopying, NSString *> typeArgsProtocolQualsConflict5; // expected-error{{angle brackets contain both a type ('NSString') and a protocol ('NSCopying')}}
 
 // Handling the '>>' in type argument lists.
 typedef PC4<id<NSCopying>, NSObject *, id<NSObject>> typeArgs6;
 
 // --------------------------------------------------------------------------
 // Checking type arguments.
 // --------------------------------------------------------------------------