In MSVC compatibility mode, friend function declarations behave as function declarations

[frederic-tingaud-sonarsource]

Summary

Before C++20, MSVC treated any friend function declaration as a function declaration, so the following code would compile despite funGlob being declared after its first call:

class Glob {
public:
  friend void funGlob();

  void test() {
    funGlob();
  }
};

void funGlob() {}

This proposed patch mimics the MSVC behavior when in MSVC compatibility mode

[rnk]

lgtm

I plugged this into godbolt to confirm the behavior of MSVC:
https://gcc.godbolt.org/z/v3P3WbxG3

This incompatibility has existed for a long time, right?

[frederic-tingaud-sonarsource]

I haven't searched far in the past, but I expect this behavior to go way back, yes.

[frederic-tingaud-sonarsource]

@rnk I don't have the rights to merge. Could you do it when you have the time?

[thakis]

Thanks for the patch!

Two comments, more specific, one general:

1. This broke compilation of (at least) one TU in chromium, see https://bugs.chromium.org/p/chromium/issues/detail?id=1323014#c5 . Can you look into if that's expected? (We've worked around this for now.)

2. We try to add workarounds like this only if it's necessary for system headers, and in cases when it is necessary we do want to emit some -Wmicrosoft warning so that user code has a chance to stay compliant. This old presentation talks about this some: https://docs.google.com/presentation/d/1oxNHaVjA9Gn_rTzX6HIpJHP7nXRua_0URXxxJ3oYRq0/edit#slide=id.g71ecd450e_2_812

What was the motivation for this change? Given we haven't needed this for such a long time, chances are it's not for system headers (?)

If we do need this, please add a warning that fires when this extension is required. Otherwise, it'd be good if we could undo this.

[frederic-tingaud-sonarsource]

That is definitely not expected. I'm looking into it, but as this is my first time building chromium, I'm taking a bit more time finding my way around.

[thakis]

I don't think you need to build chromium. https://bugs.chromium.org/p/chromium/issues/detail?id=1323014#c5 has a stand-alone reproducer attached that you can download.

[thakis]

Given that:

• This is not needed for system headers
• Before the change, users had to fix their standards-noncompliant code, but with it, users have to change their previously-compiling, standards-compliant code
• The change is a no-op for C++20 and up anyways
• There's no warning implemented, so the change is kind of against the spirit of clang-cl atm
• The change can't affect a lot of code since we didn't have if for a long time (and it only broke one single thing in all of chrome), so erring on less code in clang seems better

Should we revert this, at least for now, until the investigation is complete?

[frederic-tingaud-sonarsource]

Thanks a lot for pointing out the reproducer, it made the debug a lot easier.
I have pinpointed the problem to an unintended increase in ADL visibility for friend classes. I do have a test-case and fix for that. I'll make a last check tomorrow to make sure I'm not missing something important and push a pull-request.

[frederic-tingaud-sonarsource]

In D124613#3506740, @thakis wrote:

Should we revert this, at least for now, until the investigation is complete?

I started typing my message before you sent yours so it was not a direct answer. We are working on a fork of LLVM, so if you revert the fix for the moment, it will not put us in a bad position anyway. I'll let you judge on whether the current problem can remain until my fixing PR is reviewed or whether you prefer to revert immediately and for me to make a clean PR with the corrected version afterward.

[thakis]

It seems to not affect a ton of code, so I don't think there's a huge hurry.

I would like to hear an answer about the question "what is this for?" though, and your thoughts on how this relates to the goals in https://docs.google.com/presentation/d/1oxNHaVjA9Gn_rTzX6HIpJHP7nXRua_0URXxxJ3oYRq0/edit#slide=id.g71ecd450e_2_812 (see point 2 in https://reviews.llvm.org/D124613#3496824)

[frederic-tingaud-sonarsource]

Regarding the "why":
Our tools (SonarQube / SonarCloud / SonarLint) use the clang front-end to parse our customer's code and find bugs/bad patterns in it. Said customer code can target various compilers including MSVC and we need to handle it as gracefully as possible.
When we find gaps between MSVC and clang-cl, we try to fix them and if we think the fix will have negligible memory/performance/complexity impact on clang and be useful to the community, we try to upstream them. It's true that this fix purpose is not to fix handling of MSVC standard headers, but there are more and more tools that also target existing MSVC code (clang-tidy, Clang Power Tools, etc.) thanks to this compatibility feature and we felt that it could be valuable.
We also try to always have a warning attached to MSVC extensions, but this one has two particularities: it builds on top of an already existing MSVC compatibility feature (PerformFriendInjection in Decl::setObjectOfFriendDecl) and there is no way (without a big refactoring), that would allow to warn correctly when the extension is actually used.

[pavel.v.chupin]

We faced some similar problem in downstream project (https://github.com/intel/llvm/issues/6142) due to this change.
+1 for revert if fix takes longer.

[frederic-tingaud-sonarsource]

EDIT: patch cancelled, see next comment.

Sorry for the delay, the fix is here: https://reviews.llvm.org/D125526
If there is any risk or problem identified with this new fix that could delay it, I'll propose a revert of this one to avoid breaking more legitimate code.

[frederic-tingaud-sonarsource]

I realized that this current patch does in fact mimic MSVC behavior up to its mishandling of compliant code. I understand that it is not in the spirit of clang's MSVC compatibility to have that, but the fixed version I was about to propose would stray away from MSVC behavior, which doesn't really make sense either.
So instead I think the right approach should be to revert and see later whether we could put it back under the hood of another flag.
Is there currently a flag that could be used for "potentially C++ compliant breaking MSVC compatibility"? If there isn't, would it make sense to propose one?

[thakis]

Thanks, reverted in e0fcdf5496ca686c8cebb63b63af86e666b42ab3 for now.

In D124613#3511098, @frederic-tingaud-sonarsource wrote:

I realized that this current patch does in fact mimic MSVC behavior up to its mishandling of compliant code. I understand that it is not in the spirit of clang's MSVC compatibility to have that, but the fixed version I was about to propose would stray away from MSVC behavior, which doesn't really make sense either.
So instead I think the right approach should be to revert and see later whether we could put it back under the hood of another flag.
Is there currently a flag that could be used for "potentially C++ compliant breaking MSVC compatibility"? If there isn't, would it make sense to propose one?

I'm not sure we want to add this. The current setup seems to be working reasonably well, and adding a 3rd ms compat flag (in addition to -fms-extensions and -fms-compatibility) seems like a fairly expensive thing (from a testing pov) compared to the benefit: Your clients will have to update their code for this for msvc with c++20 anyways.

How difficult is it for your clients to make their code standards compliant for this particular thing? How many instances would they have to update?

[rnk]

I haven't given any input yet because I haven't yet seen a reduced example of the conforming code that is broken by this change. If someone can put together a small godbolt example that shows the issue affecting ADL, I'd appreciate it.

I don't fully understand the code patterns, but I think what I find most compelling so far is that we want clang users to be able to write conforming code, while accepting as much code that MSVC accepts with possible, ideally with warnings guiding folks to make code conform. Every goal can be compromised, there are no absolutes, so in the end, it's all tradeoffs.

Initially this patch appealed to me because it was a small change to accept what seemed like a small edge case of friend function definitions. It didn't seem worth coding up new codepaths for diagnostics. However, on reflection, the friend function definition is one of the gnarliest corners of the C++ language because of the mismatch between the lexical and semantic scopes. More care is probably required.

[frederic-tingaud-sonarsource]

@rnk Here is the test case that was broken: https://godbolt.org/z/Gef7PcqMf

One point where I was wrong in my initial analysis is that the behavior doesn't actually change based on /std:c++17 VS /std:c++latest, but based on /permissive VS /permissive- (which is related because /std:c++latest automatically defaults on /permissive-). Other MSVC extensions should also depend on /permissive, such as clang-cl handles friend classes visibility or ext_unqualified_base_class.

I saw that clang-cl already parses the /permissive flag and translates it to OPT__SLASH_Zc_twoPhase_ and OPT_fno_operator_names. Would it make sense to cover these extensions too? That would ensure conformant code is not broken by accident while giving people the possibility to parse their MSVC-specific code with more of its non-conformant specificities. But it does indeed increase the testing complexity.

clang/lib/Sema/SemaDecl.cpp

if (D.getDeclSpec().isModulePrivateSpecified()) {
       } else {
         NewFD->setModulePrivate();
         if (FunctionTemplate)
           FunctionTemplate->setModulePrivate();
       }
     }
 
     if (isFriend) {
+      // In MSVC mode for older versions of the standard, friend function
+      // declarations behave as declarations
+      bool PerformFriendInjection =
+          getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20;
       if (FunctionTemplate) {
-        FunctionTemplate->setObjectOfFriendDecl();
+        FunctionTemplate->setObjectOfFriendDecl(PerformFriendInjection);
         FunctionTemplate->setAccess(AS_public);
       }
-      NewFD->setObjectOfFriendDecl();
+      NewFD->setObjectOfFriendDecl(PerformFriendInjection);
       NewFD->setAccess(AS_public);
     }
 
     // If a function is defined as defaulted or deleted, mark it as such now.
     // We'll do the relevant checks on defaulted / deleted functions later.
     switch (D.getFunctionDefinitionKind()) {
     case FunctionDefinitionKind::Declaration:
     case FunctionDefinitionKind::Definition:

clang/test/SemaCXX/ms-friend-function-decl.cpp

+// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
+#if __cplusplus < 202002L
+// expected-no-diagnostics
+#endif
+
+namespace ns {
+
+class C {
+public:
+  template <typename T>
+  friend void funtemp();
+
+  friend void fun();
+
+  void test() {
+    ::ns::fun(); // modern-error {{no member named 'fun' in namespace 'ns'}}
+
+    // modern-error@+3 {{no member named 'funtemp' in namespace 'ns'}}
+    // modern-error@+2 {{expected '(' for function-style cast or type construction}}
+    // modern-error@+1 {{expected expression}}
+    ::ns::funtemp<int>();
+  }
+};
+
+void fun() {
+}
+
+template <typename T>
+void funtemp() {}
+
+} // namespace ns
+
+class Glob {
+public:
+  friend void funGlob();
+
+  void test() {
+    funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
+  }
+};
+
+void funGlob() {
+}

clang/unittests/AST/ASTImporterTest.cpp

 TEST_P(ImportFriendFunctions, Lookup) {
   auto FunctionPattern = functionDecl(hasName("f"));
   auto ClassPattern = cxxRecordDecl(hasName("X"));
 
   TranslationUnitDecl *FromTU =
       getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher<FunctionDecl>().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+            FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   {
     auto FromName = FromD->getDeclName();
     auto *Class = FirstDeclMatcher<CXXRecordDecl>().match(FromTU, ClassPattern);
     auto LookupRes = Class->noload_lookup(FromName);
     ASSERT_TRUE(LookupRes.empty());
     LookupRes = FromTU->noload_lookup(FromName);
     ASSERT_TRUE(LookupRes.isSingleResult());
   }
TranslationUnitDecl *FromTU =
                 // returns the found `NamedDecl` only if the set IDNS is matched
                 "void f();",
                 Lang_CXX03, "input0.cc");
   auto *FromFriend =
       FirstDeclMatcher<FunctionDecl>().match(FromTU, FunctionPattern);
   auto *FromNormal =
       LastDeclMatcher<FunctionDecl>().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+            FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
 
   auto FromName = FromFriend->getDeclName();
   auto *FromClass =
       FirstDeclMatcher<CXXRecordDecl>().match(FromTU, ClassPattern);
   auto LookupRes = FromClass->noload_lookup(FromName);
   ASSERT_TRUE(LookupRes.empty());
TranslationUnitDecl *FromFriendTU =
       getTuDecl("class X { friend void f(); };", Lang_CXX03, "input1.cc");
   auto *FromFriendF =
       FirstDeclMatcher<FunctionDecl>().match(FromFriendTU, Pattern);
   auto FromNormalName = FromNormalF->getDeclName();
   auto FromFriendName = FromFriendF->getDeclName();
 
   ASSERT_TRUE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromFriendTU->getLangOpts().MSVCCompat,
+            FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_TRUE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   auto LookupRes = FromNormalTU->noload_lookup(FromNormalName);
   ASSERT_TRUE(LookupRes.isSingleResult());
   LookupRes = FromFriendTU->noload_lookup(FromFriendName);
   ASSERT_TRUE(LookupRes.isSingleResult());
 
   auto *ToNormalF = cast<FunctionDecl>(Import(FromNormalF, Lang_CXX03));
   TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();