[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

I realized that this current patch did in fact mimic MSVC behavior up to its 
mishandling on ADL. I understand that it is not in the spirit of clang's 
philosophy for 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Pavel Chupin via Phabricator via cfe-commits]

pavel.v.chupin added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad47114ad850: In MSVC compatibility mode, friend function 
declarations behave as function… (authored by frederic-tingaud-sonarsource, 
committed by mantognini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -2658,7 +2658,10 @@
   getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher().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().match(FromTU, ClassPattern);
@@ -2702,7 +2705,10 @@
   auto *FromNormal =
   LastDeclMatcher().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));
 
@@ -2793,7 +2799,10 @@
 
   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());
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -0,0 +1,45 @@
+// 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 
+  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();
+  }
+};
+
+void fun() {
+}
+
+template 
+void funtemp() {}
+
+} // namespace ns
+
+class Glob {
+public:
+  friend void funGlob();
+
+  void test() {
+funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
+  }
+};
+
+void funGlob() {
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9632,11 +9632,15 @@
 }
 
 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);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124613/new/

https://reviews.llvm.org/D124613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a reviewer: shafik.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124613

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -2658,7 +2658,10 @@
   getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher().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().match(FromTU, ClassPattern);
@@ -2702,7 +2705,10 @@
   auto *FromNormal =
   LastDeclMatcher().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));
 
@@ -2793,7 +2799,10 @@
 
   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());
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -0,0 +1,45 @@
+// 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 
+  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();
+  }
+};
+
+void fun() {
+}
+
+template 
+void funtemp() {}
+
+} // namespace ns
+
+class Glob {
+public:
+  friend void funGlob();
+
+  void test() {
+funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
+  }
+};
+
+void funGlob() {
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9631,11 +9631,15 @@
 }
 
 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);