Hi @NoQ . Would you have time to look if the changes I did to this PR solve your concerns?

@rnk , does this change answer your points?

Use getAs to see through ElaboratedType as recommended by https://reviews.llvm.org/D112374

Hi @NoQ ,
Do the changes correspond to what you had in mind?

Adding an additional false negative fixed by this patch.

Update diagnostic location to highlight the whole content of the initialization.

It might also be interesting to think about cases that went through transitive assignment stripping when talking about the diagnostic position:
Before my patch, a dead store on variable a was displayed as follows:

B b;
A a = static_cast<A>(b = createB());
                         ^~~~~~~~~

(I'm switching back to C++ because I'm not sure exactly how legal such a thing would be in objective-C).
With the current fix, that doesn't change.
It might be clearer for the user to either put the diagnostic on the whole assignment or at least the whole initialization.

The change in behavior is indeed what I was mentioning in the summary. After discussing it with a colleague, it seemed to us that pointing to a more precise range where the object is allocated was an improvement. I understand your point about pointing at something that is nearer to the assignment, but in that case, wouldn't it make sense to point at the whole assignment?

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thanks for looking at the PR!

Document tests better

Fix formatting

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

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?

After checking, I realize that the previous fix was really mimicking MSVC behavior up to its mishandling of standard compliant code.
https://godbolt.org/z/cEPGfMz3f
I'll revert it for now and try to see if there is a way to have it under another flag or something.

EDIT: patch cancelled, see next 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.

In D124613#3506740, @thakis wrote:

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

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.

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.

Fix formatting

As I don't have merge rights, would it be possible for you to merge it?

Applying recommendations

By the way, in the current state of the code, there is no risk of such conflict between typo correction and UnqualifiedBase, because when an unqualified base exists the lookup result will not be empty.
That's why the test was not failing even without moving the compatibility first.

Sorry about the confusion, it makes sense. Fixed.

Added two similar names in the tests, to try to trick the typo correction.

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

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