[ObjC] Insert method parameters in scope as they are parsed

[apple-fcloutier]

Before this change, ParseObjc would call the closing MaybeParseAttributes before it had created Objective-C ParmVarDecl objects (and associated name lookup entries), meaning that you could not reference Objective-C method parameters in __attribute__((diagnose_if)). This change moves the creation of the ParmVarDecl objects ahead of calling Sema::ActOnMethodDeclaration so that MaybeParseAttributes can find them. This is already how it works for C parameters hanging off of the selector.

This change alone is insufficient to enable diagnose_if for Objective-C methods and effectively is NFC. There will be a follow-up PR for diagnose_if. This change is still useful for any other work that may need attributes to reference Objective-C parameters.

rdar://138596211

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (apple-fcloutier)

Changes

Before this change, ParseObjc would call the closing MaybeParseAttributes before it had created Objective-C ParmVarDecl objects (and associated name lookup entries), meaning that you could not reference Objective-C method parameters in __attribute__((diagnose_if)). This change moves the creation of the ParmVarDecl objects ahead of calling Sema::ActOnMethodDeclaration so that MaybeParseAttributes can find them. This is already how it works for C parameters hanging off of the selector.

This change alone is insufficient to enable diagnose_if for Objective-C methods and effectively is NFC. There will be a follow-up PR for diagnose_if. This change is still useful for any other work that may need attributes to reference Objective-C parameters.

rdar://138596211

---

Full diff: https://github.com/llvm/llvm-project/pull/113745.diff

3 Files Affected:

• (modified) clang/include/clang/Sema/SemaObjC.h (+5-1)
• (modified) clang/lib/Parse/ParseObjc.cpp (+6-4)
• (modified) clang/lib/Sema/SemaDeclObjC.cpp (+58-55)

diff --git a/clang/include/clang/Sema/SemaObjC.h b/clang/include/clang/Sema/SemaObjC.h
index 1332eb4f4d4233..791a7f45b832f7 100644
--- a/clang/include/clang/Sema/SemaObjC.h
+++ b/clang/include/clang/Sema/SemaObjC.h
@@ -351,6 +351,10 @@ class SemaObjC : public SemaBase {
     ParsedAttributesView ArgAttrs;
   };
 
+  ParmVarDecl *ActOnMethodParmDeclaration(Scope *S, ObjCArgInfo &ArgInfo,
+                                          int ParamIndex,
+                                          bool MethodDefinition);
+
   Decl *ActOnMethodDeclaration(
       Scope *S,
       SourceLocation BeginLoc, // location of the + or -.
@@ -359,7 +363,7 @@ class SemaObjC : public SemaBase {
       ArrayRef<SourceLocation> SelectorLocs, Selector Sel,
       // optional arguments. The number of types/arguments is obtained
       // from the Sel.getNumArgs().
-      ObjCArgInfo *ArgInfo, DeclaratorChunk::ParamInfo *CParamInfo,
+      ParmVarDecl **ArgInfo, DeclaratorChunk::ParamInfo *CParamInfo,
       unsigned CNumArgs, // c-style args
       const ParsedAttributesView &AttrList, tok::ObjCKeywordKind MethodImplKind,
       bool isVariadic, bool MethodDefinition);
diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
index 28ccd3061f8433..e69fa152481982 100644
--- a/clang/lib/Parse/ParseObjc.cpp
+++ b/clang/lib/Parse/ParseObjc.cpp
@@ -1454,7 +1454,7 @@ Decl *Parser::ParseObjCMethodDecl(SourceLocation mLoc,
 
   SmallVector<const IdentifierInfo *, 12> KeyIdents;
   SmallVector<SourceLocation, 12> KeyLocs;
-  SmallVector<SemaObjC::ObjCArgInfo, 12> ArgInfos;
+  SmallVector<ParmVarDecl *, 12> ObjCParamInfo;
   ParseScope PrototypeScope(this, Scope::FunctionPrototypeScope |
                             Scope::FunctionDeclarationScope | Scope::DeclScope);
 
@@ -1495,7 +1495,9 @@ Decl *Parser::ParseObjCMethodDecl(SourceLocation mLoc,
     ArgInfo.NameLoc = Tok.getLocation();
     ConsumeToken(); // Eat the identifier.
 
-    ArgInfos.push_back(ArgInfo);
+    ParmVarDecl *Param = Actions.ObjC().ActOnMethodParmDeclaration(
+        getCurScope(), ArgInfo, ObjCParamInfo.size(), MethodDefinition);
+    ObjCParamInfo.push_back(Param);
     KeyIdents.push_back(SelIdent);
     KeyLocs.push_back(selLoc);
 
@@ -1567,8 +1569,8 @@ Decl *Parser::ParseObjCMethodDecl(SourceLocation mLoc,
                                                    &KeyIdents[0]);
   Decl *Result = Actions.ObjC().ActOnMethodDeclaration(
       getCurScope(), mLoc, Tok.getLocation(), mType, DSRet, ReturnType, KeyLocs,
-      Sel, &ArgInfos[0], CParamInfo.data(), CParamInfo.size(), methodAttrs,
-      MethodImplKind, isVariadic, MethodDefinition);
+      Sel, ObjCParamInfo.data(), CParamInfo.data(), CParamInfo.size(),
+      methodAttrs, MethodImplKind, isVariadic, MethodDefinition);
 
   PD.complete(Result);
   return Result;
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 78acfeddb78639..9f72ec32e4fc37 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -4720,13 +4720,67 @@ static void checkObjCDirectMethodClashes(Sema &S, ObjCInterfaceDecl *IDecl,
           diagClash(IMD);
 }
 
+ParmVarDecl *SemaObjC::ActOnMethodParmDeclaration(Scope *S,
+                                                  ObjCArgInfo &ArgInfo,
+                                                  int ParamIndex,
+                                                  bool MethodDefinition) {
+  ASTContext &Context = getASTContext();
+  QualType ArgType;
+  TypeSourceInfo *DI;
+
+  if (!ArgInfo.Type) {
+    ArgType = Context.getObjCIdType();
+    DI = nullptr;
+  } else {
+    ArgType = SemaRef.GetTypeFromParser(ArgInfo.Type, &DI);
+  }
+  LookupResult R(SemaRef, ArgInfo.Name, ArgInfo.NameLoc,
+                 Sema::LookupOrdinaryName,
+                 SemaRef.forRedeclarationInCurContext());
+  SemaRef.LookupName(R, S);
+  if (R.isSingleResult()) {
+    NamedDecl *PrevDecl = R.getFoundDecl();
+    if (S->isDeclScope(PrevDecl)) {
+      Diag(ArgInfo.NameLoc,
+           (MethodDefinition ? diag::warn_method_param_redefinition
+                             : diag::warn_method_param_declaration))
+          << ArgInfo.Name;
+      Diag(PrevDecl->getLocation(), diag::note_previous_declaration);
+    }
+  }
+  SourceLocation StartLoc =
+      DI ? DI->getTypeLoc().getBeginLoc() : ArgInfo.NameLoc;
+
+  // Temporarily put parameter variables in the translation unit. This is what
+  // ActOnParamDeclarator does in the case of C arguments to the Objective-C
+  // method too.
+  ParmVarDecl *Param = SemaRef.CheckParameter(
+      Context.getTranslationUnitDecl(), StartLoc, ArgInfo.NameLoc, ArgInfo.Name,
+      ArgType, DI, SC_None);
+  Param->setObjCMethodScopeInfo(ParamIndex);
+  Param->setObjCDeclQualifier(
+      CvtQTToAstBitMask(ArgInfo.DeclSpec.getObjCDeclQualifier()));
+
+  // Apply the attributes to the parameter.
+  SemaRef.ProcessDeclAttributeList(SemaRef.TUScope, Param, ArgInfo.ArgAttrs);
+  SemaRef.AddPragmaAttributes(SemaRef.TUScope, Param);
+  if (Param->hasAttr<BlocksAttr>()) {
+    Diag(Param->getLocation(), diag::err_block_on_nonlocal);
+    Param->setInvalidDecl();
+  }
+
+  S->AddDecl(Param);
+  SemaRef.IdResolver.AddDecl(Param);
+  return Param;
+}
+
 Decl *SemaObjC::ActOnMethodDeclaration(
     Scope *S, SourceLocation MethodLoc, SourceLocation EndLoc,
     tok::TokenKind MethodType, ObjCDeclSpec &ReturnQT, ParsedType ReturnType,
     ArrayRef<SourceLocation> SelectorLocs, Selector Sel,
     // optional arguments. The number of types/arguments is obtained
     // from the Sel.getNumArgs().
-    ObjCArgInfo *ArgInfo, DeclaratorChunk::ParamInfo *CParamInfo,
+    ParmVarDecl **ArgInfo, DeclaratorChunk::ParamInfo *CParamInfo,
     unsigned CNumArgs, // c-style args
     const ParsedAttributesView &AttrList, tok::ObjCKeywordKind MethodDeclKind,
     bool isVariadic, bool MethodDefinition) {
@@ -4768,60 +4822,9 @@ Decl *SemaObjC::ActOnMethodDeclaration(
       HasRelatedResultType);
 
   SmallVector<ParmVarDecl*, 16> Params;
-
-  for (unsigned i = 0, e = Sel.getNumArgs(); i != e; ++i) {
-    QualType ArgType;
-    TypeSourceInfo *DI;
-
-    if (!ArgInfo[i].Type) {
-      ArgType = Context.getObjCIdType();
-      DI = nullptr;
-    } else {
-      ArgType = SemaRef.GetTypeFromParser(ArgInfo[i].Type, &DI);
-    }
-
-    LookupResult R(SemaRef, ArgInfo[i].Name, ArgInfo[i].NameLoc,
-                   Sema::LookupOrdinaryName,
-                   SemaRef.forRedeclarationInCurContext());
-    SemaRef.LookupName(R, S);
-    if (R.isSingleResult()) {
-      NamedDecl *PrevDecl = R.getFoundDecl();
-      if (S->isDeclScope(PrevDecl)) {
-        Diag(ArgInfo[i].NameLoc,
-             (MethodDefinition ? diag::warn_method_param_redefinition
-                               : diag::warn_method_param_declaration))
-          << ArgInfo[i].Name;
-        Diag(PrevDecl->getLocation(),
-             diag::note_previous_declaration);
-      }
-    }
-
-    SourceLocation StartLoc = DI
-      ? DI->getTypeLoc().getBeginLoc()
-      : ArgInfo[i].NameLoc;
-
-    ParmVarDecl *Param =
-        SemaRef.CheckParameter(ObjCMethod, StartLoc, ArgInfo[i].NameLoc,
-                               ArgInfo[i].Name, ArgType, DI, SC_None);
-
-    Param->setObjCMethodScopeInfo(i);
-
-    Param->setObjCDeclQualifier(
-      CvtQTToAstBitMask(ArgInfo[i].DeclSpec.getObjCDeclQualifier()));
-
-    // Apply the attributes to the parameter.
-    SemaRef.ProcessDeclAttributeList(SemaRef.TUScope, Param,
-                                     ArgInfo[i].ArgAttrs);
-    SemaRef.AddPragmaAttributes(SemaRef.TUScope, Param);
-    SemaRef.ProcessAPINotes(Param);
-
-    if (Param->hasAttr<BlocksAttr>()) {
-      Diag(Param->getLocation(), diag::err_block_on_nonlocal);
-      Param->setInvalidDecl();
-    }
-    S->AddDecl(Param);
-    SemaRef.IdResolver.AddDecl(Param);
-
+  for (unsigned I = 0; I < Sel.getNumArgs(); ++I) {
+    ParmVarDecl *Param = ArgInfo[I];
+    Param->setDeclContext(ObjCMethod);
     Params.push_back(Param);
   }
 

[apple-fcloutier]

Failing test seems to be failing on main.

[apple-fcloutier]

@ahatanak, will you be able to take a look?

[ahatanak]

It looks like the call to ProcessAPINotes is dropped. Was that intentional?

[apple-fcloutier]

No, this is an accident.

[apple-fcloutier]

I re-added it to ActOnMethodDeclaration because it looks up the DeclContext, which we change in the loop.