[clang] Rename Decl::isHidden() to isUnconditionallyVisible()

[mboehme]

Summary

Also invert the sense of the return value.

As pointed out by the FIXME that this change resolves, isHidden() wasn't a very accurate name for this function.

I haven't yet changed any of the strings that are output in ASTDumper.cpp / JSONNodeDumper.cpp / TextNodeDumper.cpp in response to whether isHidden() is set because

a) I'm not sure whether it's actually desired to change these strings (would appreciate feedback on this), and

b) In any case, I'd like to get this pure rename out of the way first, without any changes to tests. Changing the strings that are output in the various ...Dumper.cpp files will require changes to quite a few tests, and I'd like to make those in a separate change.

[rsmith]

I think renaming the flag in the AST dump output would be a good idea, though it'll be a lot of churn in the tests. I would prefer that we continue to dump a marker only if the declaration is not unconditionally visible rather than reversing the sense of the flag in the dump output. Maybe we should dump the ModuleOwnershipKind in general, not only an indicator of whether it's Visible or something else?

[mboehme]

In D81392#2088131, @rsmith wrote:

I think renaming the flag in the AST dump output would be a good idea, though it'll be a lot of churn in the tests. I would prefer that we continue to dump a marker only if the declaration is not unconditionally visible rather than reversing the sense of the flag in the dump output.

Agree -- I had tried that experimentally, but it does cause a lot of noise in the output.

Maybe we should dump the ModuleOwnershipKind in general, not only an indicator of whether it's Visible or something else?

I like this -- though would we then always dump the ModuleOwnershipKind (i.e. causing a lot of churn, as discussed above) or only if it's VisibleWhenImported or ModulePrivate (i.e. not unconditionally visible)?

[rsmith]

In D81392#2089251, @mboehme wrote:

In D81392#2088131, @rsmith wrote:

Maybe we should dump the ModuleOwnershipKind in general, not only an indicator of whether it's Visible or something else?

I like this -- though would we then always dump the ModuleOwnershipKind (i.e. causing a lot of churn, as discussed above) or only if it's VisibleWhenImported or ModulePrivate (i.e. not unconditionally visible)?

I think only producing output for the "non-default" state (not Visible) would be best, both too minimize the test churn and to keep the dump smaller and simpler in simple cases.

clang/include/clang/AST/DeclBase.h

public:
 
   /// Get the module that owns this declaration for linkage purposes.
   /// There only ever is such a module under the C++ Modules TS.
   ///
   /// \param IgnoreLinkage Ignore the linkage of the entity; assume that
   /// all declarations in a global module fragment are unowned.
   Module *getOwningModuleForLinkage(bool IgnoreLinkage = false) const;
 
-  /// Determine whether this declaration might be hidden from name
-  /// lookup. Note that the declaration might be visible even if this returns
-  /// \c false, if the owning module is visible within the query context.
-  // FIXME: Rename this to make it clearer what it does.
-  bool isHidden() const {
-    return (int)getModuleOwnershipKind() > (int)ModuleOwnershipKind::Visible;
+  /// Determine whether this declaration is definitely visible to name lookup,
+  /// independent of whether the owning module is visible.
+  /// Note: The declaration may be visible even if this returns \c false if the
+  /// owning module is visible within the query context. This is a low-level
+  /// helper function; most code should be calling Sema::isVisible() instead.
+  bool isUnconditionallyVisible() const {
+    return (int)getModuleOwnershipKind() <= (int)ModuleOwnershipKind::Visible;
   }
 
   /// Set that this declaration is globally visible, even if it came from a
   /// module that is not visible.
   void setVisibleDespiteOwningModule() {
-    if (isHidden())
+    if (!isUnconditionallyVisible())
       setModuleOwnershipKind(ModuleOwnershipKind::Visible);
   }
 
   /// Get the kind of module ownership for this declaration.
   ModuleOwnershipKind getModuleOwnershipKind() const {
     return NextInContextAndBits.getInt();
   }
 

clang/include/clang/AST/DeclObjC.h

 inline ObjCInterfaceDecl::filtered_category_iterator<Filter> &
 ObjCInterfaceDecl::filtered_category_iterator<Filter>::operator++() {
   Current = Current->getNextClassCategoryRaw();
   findAcceptableCategory();
   return *this;
 }
 
 inline bool ObjCInterfaceDecl::isVisibleCategory(ObjCCategoryDecl *Cat) {
-  return !Cat->isHidden();
+  return Cat->isUnconditionallyVisible();
 }
 
 inline bool ObjCInterfaceDecl::isVisibleExtension(ObjCCategoryDecl *Cat) {
-  return Cat->IsClassExtension() && !Cat->isHidden();
+  return Cat->IsClassExtension() && Cat->isUnconditionallyVisible();
 }
 
 inline bool ObjCInterfaceDecl::isKnownExtension(ObjCCategoryDecl *Cat) {
   return Cat->IsClassExtension();
 }
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_DECLOBJC_H

clang/include/clang/Sema/Lookup.h

public:
   CXXBasePaths *getBasePaths() const {
     return Paths;
   }
 
   /// Determine whether the given declaration is visible to the
   /// program.
   static bool isVisible(Sema &SemaRef, NamedDecl *D) {
     // If this declaration is not hidden, it's visible.
-    if (!D->isHidden())
+    if (D->isUnconditionallyVisible())
       return true;
 
     // During template instantiation, we can refer to hidden declarations, if
     // they were visible in any module along the path of instantiation.
     return isVisibleSlow(SemaRef, D);
   }
 
   /// Retrieve the accepted (re)declaration of the given declaration,

clang/include/clang/Sema/Sema.h

public:
   /// Make a merged definition of an existing hidden definition \p ND
   /// visible at the specified location.
   void makeMergedDefinitionVisible(NamedDecl *ND);
 
   bool isModuleVisible(const Module *M, bool ModulePrivate = false);
 
   /// Determine whether a declaration is visible to name lookup.
   bool isVisible(const NamedDecl *D) {
-    return !D->isHidden() || isVisibleSlow(D);
+    return D->isUnconditionallyVisible() || isVisibleSlow(D);
   }
 
   /// Determine whether any declaration of an entity is visible.
   bool
   hasVisibleDeclaration(const NamedDecl *D,
                         llvm::SmallVectorImpl<Module *> *Modules = nullptr) {
     return isVisible(D) || hasVisibleDeclarationSlow(D, Modules);
   }

clang/lib/AST/ASTDumper.cpp

for (auto I = Range.begin(), E = Range.end(); I != E; ++I) {
           OS << '\'' << Name << '\'';
         }
 
         for (DeclContextLookupResult::iterator RI = R.begin(), RE = R.end();
              RI != RE; ++RI) {
           NodeDumper.AddChild([=] {
             NodeDumper.dumpBareDeclRef(*RI);
 
-            if ((*RI)->isHidden())
+            if (!(*RI)->isUnconditionallyVisible())
               OS << " hidden";
 
             // If requested, dump the redecl chain for this lookup.
             if (DumpDecls) {
               // Dump earliest decl first.
               std::function<void(Decl *)> DumpWithPrev = [&](Decl *D) {
                 if (Decl *Prev = D->getPreviousDecl())
                   DumpWithPrev(Prev);

clang/lib/AST/DeclObjC.cpp

 // Get the local instance/class method declared in this interface.
 ObjCMethodDecl *
 ObjCContainerDecl::getMethod(Selector Sel, bool isInstance,
                              bool AllowHidden) const {
   // If this context is a hidden protocol definition, don't find any
   // methods there.
   if (const auto *Proto = dyn_cast<ObjCProtocolDecl>(this)) {
     if (const ObjCProtocolDecl *Def = Proto->getDefinition())
-      if (Def->isHidden() && !AllowHidden)
+      if (!Def->isUnconditionallyVisible() && !AllowHidden)
         return nullptr;
   }
 
   // Since instance & class methods can have the same name, the loop below
   // ensures we get the correct method.
   //
   // @interface Whatever
   // - (int) class_method;

 ObjCPropertyDecl *
 ObjCPropertyDecl::findPropertyDecl(const DeclContext *DC,
                                    const IdentifierInfo *propertyID,
                                    ObjCPropertyQueryKind queryKind) {
   // If this context is a hidden protocol definition, don't find any
   // property.
   if (const auto *Proto = dyn_cast<ObjCProtocolDecl>(DC)) {
     if (const ObjCProtocolDecl *Def = Proto->getDefinition())
-      if (Def->isHidden())
+      if (!Def->isUnconditionallyVisible())
         return nullptr;
   }
 
   // If context is class, then lookup property in its visible extensions.
   // This comes before property is looked up in primary class.
   if (auto *IDecl = dyn_cast<ObjCInterfaceDecl>(DC)) {
     for (const auto *Ext : IDecl->visible_extensions())
       if (ObjCPropertyDecl *PD = ObjCPropertyDecl::findPropertyDecl(Ext,

 /// FindPropertyDeclaration - Finds declaration of the property given its name
 /// in 'PropertyId' and returns it. It returns 0, if not found.
 ObjCPropertyDecl *ObjCContainerDecl::FindPropertyDeclaration(
     const IdentifierInfo *PropertyId,
     ObjCPropertyQueryKind QueryKind) const {
   // Don't find properties within hidden protocol definitions.
   if (const auto *Proto = dyn_cast<ObjCProtocolDecl>(this)) {
     if (const ObjCProtocolDecl *Def = Proto->getDefinition())
-      if (Def->isHidden())
+      if (!Def->isUnconditionallyVisible())
         return nullptr;
   }
 
   // Search the extensions of a class first; they override what's in
   // the class itself.
   if (const auto *ClassDecl = dyn_cast<ObjCInterfaceDecl>(this)) {
     for (const auto *Ext : ClassDecl->visible_extensions()) {
       if (auto *P = Ext->FindPropertyDeclaration(PropertyId, QueryKind))

 // it inherited.
 ObjCMethodDecl *ObjCProtocolDecl::lookupMethod(Selector Sel,
                                                bool isInstance) const {
   ObjCMethodDecl *MethodDecl = nullptr;
 
   // If there is no definition or the definition is hidden, we don't find
   // anything.
   const ObjCProtocolDecl *Def = getDefinition();
-  if (!Def || Def->isHidden())
+  if (!Def || !Def->isUnconditionallyVisible())
     return nullptr;
 
   if ((MethodDecl = getMethod(Sel, isInstance)))
     return MethodDecl;
 
   for (const auto *I : protocols())
     if ((MethodDecl = I->lookupMethod(Sel, isInstance)))
       return MethodDecl;

clang/lib/AST/JSONNodeDumper.cpp

void JSONNodeDumper::Visit(const Decl *D) {
   attributeOnlyIfTrue("isInvalid", D->isInvalidDecl());
 
   if (D->isUsed())
     JOS.attribute("isUsed", true);
   else if (D->isThisDeclarationReferenced())
     JOS.attribute("isReferenced", true);
 
   if (const auto *ND = dyn_cast<NamedDecl>(D))
-    attributeOnlyIfTrue("isHidden", ND->isHidden());
+    attributeOnlyIfTrue("isHidden", !ND->isUnconditionallyVisible());
 
   if (D->getLexicalDeclContext() != D->getDeclContext()) {
     // Because of multiple inheritance, a DeclContext pointer does not produce
     // the same pointer representation as a Decl pointer that references the
     // same AST Node.
     const auto *ParentDeclContextDecl = dyn_cast<Decl>(D->getDeclContext());
     JOS.attribute("parentDeclContextId",
                   createPointerRepresentation(ParentDeclContextDecl));

clang/lib/AST/TextNodeDumper.cpp

if (D->isFromASTFile())
     OS << " imported";
   if (Module *M = D->getOwningModule())
     OS << " in " << M->getFullModuleName();
   if (auto *ND = dyn_cast<NamedDecl>(D))
     for (Module *M : D->getASTContext().getModulesWithMergedDefinition(
              const_cast<NamedDecl *>(ND)))
       AddChild([=] { OS << "also in " << M->getFullModuleName(); });
   if (const NamedDecl *ND = dyn_cast<NamedDecl>(D))
-    if (ND->isHidden())
+    if (!ND->isUnconditionallyVisible())
       OS << " hidden";
   if (D->isImplicit())
     OS << " implicit";
 
   if (D->isUsed())
     OS << " used";
   else if (D->isThisDeclarationReferenced())
     OS << " referenced";

clang/lib/Sema/SemaDeclObjC.cpp

Decl *Sema::ActOnStartProtocolInterface(
   }
 
   CheckObjCDeclScope(PDecl);
   return ActOnObjCContainerStartDefinition(PDecl);
 }
 
 static bool NestedProtocolHasNoDefinition(ObjCProtocolDecl *PDecl,
                                           ObjCProtocolDecl *&UndefinedProtocol) {
-  if (!PDecl->hasDefinition() || PDecl->getDefinition()->isHidden()) {
+  if (!PDecl->hasDefinition() ||
+      !PDecl->getDefinition()->isUnconditionallyVisible()) {
     UndefinedProtocol = PDecl;
     return true;
   }
 
   for (auto *PI : PDecl->protocols())
     if (NestedProtocolHasNoDefinition(PI, UndefinedProtocol)) {
       UndefinedProtocol = PI;
       return true;

 bool Sema::MatchTwoMethodDeclarations(const ObjCMethodDecl *left,
                                       const ObjCMethodDecl *right,
                                       MethodMatchStrategy strategy) {
   if (!matchTypes(Context, strategy, left->getReturnType(),
                   right->getReturnType()))
     return false;
 
   // If either is hidden, it is not considered to match.
-  if (left->isHidden() || right->isHidden())
+  if (!left->isUnconditionallyVisible() || !right->isUnconditionallyVisible())
     return false;
 
   if (left->isDirectMethod() != right->isDirectMethod())
     return false;
 
   if (getLangOpts().ObjCAutoRefCount &&
       (left->hasAttr<NSReturnsRetainedAttr>()
          != right->hasAttr<NSReturnsRetainedAttr>() ||
bool Sema::CollectMultipleMethodsInGlobalPool(
   GlobalMethodPool::iterator Pos = MethodPool.find(Sel);
   if (Pos == MethodPool.end())
     return false;
 
   // Gather the non-hidden methods.
   ObjCMethodList &MethList = InstanceFirst ? Pos->second.first :
                              Pos->second.second;
   for (ObjCMethodList *M = &MethList; M; M = M->getNext())
-    if (M->getMethod() && !M->getMethod()->isHidden()) {
+    if (M->getMethod() && M->getMethod()->isUnconditionallyVisible()) {
       if (FilterMethodsByTypeBound(M->getMethod(), TypeBound))
         Methods.push_back(M->getMethod());
     }
 
   // Return if we find any method with the desired kind.
   if (!Methods.empty())
     return Methods.size() > 1;
 
   if (!CheckTheOther)
     return false;
 
   // Gather the other kind.
   ObjCMethodList &MethList2 = InstanceFirst ? Pos->second.second :
                               Pos->second.first;
   for (ObjCMethodList *M = &MethList2; M; M = M->getNext())
-    if (M->getMethod() && !M->getMethod()->isHidden()) {
+    if (M->getMethod() && M->getMethod()->isUnconditionallyVisible()) {
       if (FilterMethodsByTypeBound(M->getMethod(), TypeBound))
         Methods.push_back(M->getMethod());
     }
 
   return Methods.size() > 1;
 }
 
 bool Sema::AreMultipleMethodsInGlobalPool(
ObjCMethodDecl *Sema::LookupMethodInGlobalPool(Selector Sel, SourceRange R,
   GlobalMethodPool::iterator Pos = MethodPool.find(Sel);
   if (Pos == MethodPool.end())
     return nullptr;
 
   // Gather the non-hidden methods.
   ObjCMethodList &MethList = instance ? Pos->second.first : Pos->second.second;
   SmallVector<ObjCMethodDecl *, 4> Methods;
   for (ObjCMethodList *M = &MethList; M; M = M->getNext()) {
-    if (M->getMethod() && !M->getMethod()->isHidden())
+    if (M->getMethod() && M->getMethod()->isUnconditionallyVisible())
       return M->getMethod();
   }
   return nullptr;
 }
 
 void Sema::DiagnoseMultipleMethodInGlobalPool(SmallVectorImpl<ObjCMethodDecl*> &Methods,
                                               Selector Sel, SourceRange R,
                                               bool receiverIdOrClass) {

clang/lib/Sema/SemaLookup.cpp

 ///
 /// This routine determines whether the declaration D is visible in the current
 /// lookup context, taking into account the current template instantiation
 /// stack. During template instantiation, a declaration is visible if it is
 /// visible from a module containing any entity on the template instantiation
 /// path (by instantiating a template, you allow it to see the declarations that
 /// your module can see, including those later on in your module).
 bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) {
-  assert(D->isHidden() && "should not call this: not in slow case");
+  assert(!D->isUnconditionallyVisible() &&
+         "should not call this: not in slow case");
 
   Module *DeclModule = SemaRef.getOwningModule(D);
   assert(DeclModule && "hidden decl has no owning module");
 
   // If the owning module is visible, the decl is visible.
   if (SemaRef.isModuleVisible(DeclModule, D->isModulePrivate()))
     return true;
 

clang/lib/Serialization/ASTReader.cpp

for (ObjCMethodList *List = &Start; List; List = List->getNext()) {
     else
       List->setMethod(Method);
   }
 }
 
 void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner) {
   assert(Owner->NameVisibility != Module::Hidden && "nothing to make visible?");
   for (Decl *D : Names) {
-    bool wasHidden = D->isHidden();
+    bool wasHidden = !D->isUnconditionallyVisible();
     D->setVisibleDespiteOwningModule();
 
     if (wasHidden && SemaObj) {
       if (ObjCMethodDecl *Method = dyn_cast<ObjCMethodDecl>(D)) {
         moveMethodToBackOfGlobalList(*SemaObj, Method);
       }
     }
   }
void ASTReader::makeModuleVisible(Module *Mod,
   }
 }
 
 /// We've merged the definition \p MergedDef into the existing definition
 /// \p Def. Ensure that \p Def is made visible whenever \p MergedDef is made
 /// visible.
 void ASTReader::mergeDefinitionVisibility(NamedDecl *Def,
                                           NamedDecl *MergedDef) {
-  if (Def->isHidden()) {
+  if (!Def->isUnconditionallyVisible()) {
     // If MergedDef is visible or becomes visible, make the definition visible.
-    if (!MergedDef->isHidden())
+    if (MergedDef->isUnconditionallyVisible())
       Def->setVisibleDespiteOwningModule();
     else {
       getContext().mergeDefinitionIntoModule(
           Def, MergedDef->getImportedOwningModule(),
           /*NotifyListeners*/ false);
       PendingMergedDefinitionsToDeduplicate.insert(Def);
     }
   }

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::DeclarationMarkedOpenMPDeclareTarget(const Decl *D,
 
   DeclUpdates[D].push_back(
       DeclUpdate(UPD_DECL_MARKED_OPENMP_DECLARETARGET, Attr));
 }
 
 void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D, Module *M) {
   if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
-  assert(D->isHidden() && "expected a hidden declaration");
+  assert(!D->isUnconditionallyVisible() && "expected a hidden declaration");
   DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_EXPORTED, M));
 }
 
 void ASTWriter::AddedAttributeToRecord(const Attr *Attr,
                                        const RecordDecl *Record) {
   if (Chain && Chain->isProcessingUpdateRecords()) return;
   assert(!WritingAST && "Already writing the AST!");
   if (!Record->isFromASTFile())