Fix use of namespace qualifiers that have not been explicitly introduced

[morten-ofstad]

These changed are required to compile the azure-core library with VS2017 compilers. I am a bit surprised that it works with C++17 compilers, since a minimal example I made of introducing a name with a 'using X::Y::Z' declaration and then trying to refer to the name as Y::Z doesn't seem to work with any compilers (Using Compiler Explorer to test).

[github-actions]

Thank you for your contribution @morten-ofstad! We will review the pull request and get back to you soon.

[morten-ofstad]

@microsoft-github-policy-service agree company="Bluware"

[antkmsft]

Thank you, @morten-ofstad!
The situation is similar to #4352:
"Note that we do not have CI for VS 2017, and at this point we support VS 2019 and 2022. I.e. we can't guarantee you that we won't break VS 2017 build in future commits in some other place, as we don't have tools to verify that and we don't have plans to add them. But we will likely accept fixes like this in the future as well (I hope that it won't be needed often).".

As of the PR, the CI is currently failing on clang-format in the sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp file.
One way to fix it, is to run "find ./sdk \( -iname '*.hpp' -o -iname '*.cpp' \) ! -iname 'json.hpp' -exec clang-format -i {} \;" but if your dev environment is not really set up for that, let me know, we can figure something out - for example I could run it on my machine, and propose a suggestion to this PR.

[morten-ofstad]

Thank you, @morten-ofstad! The situation is similar to #4352: "Note that we do not have CI for VS 2017, and at this point we support VS 2019 and 2022. I.e. we can't guarantee you that we won't break VS 2017 build in future commits in some other place, as we don't have tools to verify that and we don't have plans to add them. But we will likely accept fixes like this in the future as well (I hope that it won't be needed often).".

As of the PR, the CI is currently failing on clang-format in the sdk/core/azure-core/src/http/bearer_token_authentication_policy.cpp file. One way to fix it, is to run "find ./sdk \( -iname '*.hpp' -o -iname '*.cpp' \) ! -iname 'json.hpp' -exec clang-format -i {} \;" but if your dev environment is not really set up for that, let me know, we can figure something out - for example I could run it on my machine, and propose a suggestion to this PR.

I did not make any changes to the formatting, and it's not that easy for me to run clang-format, so I would appreciate your help with this.

[morten-ofstad]

These changed are required to compile the azure-core library with VS2017 compilers. I am a bit surprised that it works with C++17 compilers, since a minimal example I made of introducing a name with a 'using X::Y::Z' declaration and then trying to refer to the name as Y::Z doesn't seem to work with any compilers (Using Compiler Explorer to test).

... I looked a bit more into this, and it is because the rules for unqualified name lookup was changed in C++17 so it also searches the nested namespaces where the function/method was first declared when resolving names (of types) in the function definition (including the parameter list). So your code is perfectly valid C++17 but not valid C++14.

I worked with Jørgen who contributed the #4352 changes, this time I was doing the upgrade to a more recent version of azure-sdk-for-cpp and since you wrote that you "will likely accept fixes like this in the future as well" I decided to make a PR with the patch I had to apply in our build system...

[LarryOsterman]

These changed are required to compile the azure-core library with VS2017 compilers. I am a bit surprised that it works with C++17 compilers, since a minimal example I made of introducing a name with a 'using X::Y::Z' declaration and then trying to refer to the name as Y::Z doesn't seem to work with any compilers (Using Compiler Explorer to test).

... I looked a bit more into this, and it is because the rules for unqualified name lookup was changed in C++17 so it also searches the nested namespaces where the function/method was first declared when resolving names (of types) in the function definition (including the parameter list). So your code is perfectly valid C++17 but not valid C++14.

I worked with Jørgen who contributed the #4352 changes, this time I was doing the upgrade to a more recent version of azure-sdk-for-cpp and since you wrote that you "will likely accept fixes like this in the future as well" I decided to make a PR with the patch I had to apply in our build system...

That's fascinating, because it implies that there's a bug in MSVC, clang and GCC - when they added C++17 name resolution support, they didn't include the change in an "if C++17" conditional, and thus they accidentally changed the rules for C++ namespace resolution even when /std::c++14 is specified. That's the kind of mistake I could easily imagine being made.

And of course it would only be discovered when using an older compiler that hadn't been updated to the new rules.

I'll start a discussion on our internal C++ alias to see if this break can be fixed in MSVC.

[LarryOsterman]

These changed are required to compile the azure-core library with VS2017 compilers. I am a bit surprised that it works with C++17 compilers, since a minimal example I made of introducing a name with a 'using X::Y::Z' declaration and then trying to refer to the name as Y::Z doesn't seem to work with any compilers (Using Compiler Explorer to test).

... I looked a bit more into this, and it is because the rules for unqualified name lookup was changed in C++17 so it also searches the nested namespaces where the function/method was first declared when resolving names (of types) in the function definition (including the parameter list). So your code is perfectly valid C++17 but not valid C++14.
I worked with Jørgen who contributed the #4352 changes, this time I was doing the upgrade to a more recent version of azure-sdk-for-cpp and since you wrote that you "will likely accept fixes like this in the future as well" I decided to make a PR with the patch I had to apply in our build system...

That's fascinating, because it implies that there's a bug in MSVC, clang and GCC - when they added C++17 name resolution support, they didn't include the change in an "if C++17" conditional, and thus they accidentally changed the rules for C++ namespace resolution even when /std::c++14 is specified. That's the kind of mistake I could easily imagine being made.

And of course it would only be discovered when using an older compiler that hadn't been updated to the new rules.

I'll start a discussion on our internal C++ alias to see if this break can be fixed in MSVC.

@morten-ofstad I asked our C++ standards committee members and they're not aware of any changes in type resolution introduced in C++17. Do you happen to have the DR number which changed the behavior, or a reference page describing the type resolution change?

[morten-ofstad]

@morten-ofstad I asked our C++ standards committee members and they're not aware of any changes in type resolution introduced in C++17. Do you happen to have the DR number which changed the behavior, or a reference page describing the type resolution change?

Sorry, I think I was confused by the nested-namespace support that was added in C++17. In fact the unqualified name lookup rules that require searching enclosing namespaces of the class definition were already part of C++11 ( 3.4.1 Unqualified name lookup, section 6), but it appears it was not implemented correctly in Visual Studio prior to Visual Studio 2019 (where nested-namespace support was added).

[LarryOsterman]

@morten-ofstad I asked our C++ standards committee members and they're not aware of any changes in type resolution introduced in C++17. Do you happen to have the DR number which changed the behavior, or a reference page describing the type resolution change?

Sorry, I think I was confused by the nested-namespace support that was added in C++17. In fact the unqualified name lookup rules that require searching enclosing namespaces of the class definition were already part of C++11 ( 3.4.1 Unqualified name lookup, section 6), but it appears it was not implemented correctly in Visual Studio prior to Visual Studio 2019 (where nested-namespace support was added).

Ah, that makes much more sense. So this PR is to work around a bug in VS 2017, which isn't at all surprising.