fix(firebase_auth): Fix std::variant compiler errors with VS 2022 17.12

[StephanTLavavej]

Fixes #16536. (I hope - see below.)

⚙️ The types involved

Flutter's EncodableValue derives from this std::variant:

https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L94-L95
https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L103-L116
https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L165

class EncodableValue : public internal::EncodableValueVariant {

using EncodableValueVariant = std::variant<std::monostate,
                                           bool,
                                           int32_t,
                                           int64_t,
                                           double,
                                           std::string,
                                           std::vector<uint8_t>,
                                           std::vector<int32_t>,
                                           std::vector<int64_t>,
                                           std::vector<double>,
                                           EncodableList,
                                           EncodableMap,
                                           CustomEncodableValue,
                                           std::vector<float>>;

using EncodableList = std::vector<EncodableValue>;
using EncodableMap = std::map<EncodableValue, EncodableValue>;

where CustomEncodableValue is constructible from std::any:

https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L60-L62

class CustomEncodableValue {
 public:
  explicit CustomEncodableValue(const std::any& value) : value_(value) {}

🛠️ How I fixed this

⚠️ I have NOT properly built this change and I definitely have NOT tested it. (I work on the C++ Standard Library all day, compiling on the command line, so I am utterly unfamiliar with your technology stack.)

I extracted Flutter SDKs and hacked up a compiler command line to consume them, adding guesses for macro definitions, until I was able to reproduce the compiler error. With the full template instantiation context, I guessed how to fix the error, successfully fixed it on the first try, and had to fix a very similar error on a following line.

Click to expand totally hacked-up compiler command line (with my fix):

C:\temp\flutterfire\packages\firebase_auth\firebase_auth\windows>cl /EHsc /nologo /W4 /std:c++17 /MDd /Od /I C:\Users\stl\Downloads\flutter_windows_3.24.5-stable\flutter\bin\cache\artifacts\engine\windows-x64\cpp_client_wrapper\include /I C:\Users\stl\Downloads\flutter_windows_3.24.5-stable\flutter\bin\cache\artifacts\engine\windows-x64 /I C:\temp\firebase-cpp-sdk\app\src\include /I C:\temp\firebase-cpp-sdk\auth\src\include /I C:\temp\flutterfire\packages\firebase_core\firebase_core\windows\include /DINTERNAL_EXPERIMENTAL /wd4100 /wd4996 /DFLUTTER_PLUGIN_IMPL /c *.cpp
firebase_auth_plugin.cpp
firebase_auth_plugin_c_api.cpp
messages.g.cpp
Generating Code...

C:\temp\flutterfire\packages\firebase_auth\firebase_auth\windows>

💡 Explaining the fix

The bottom of the template instantiation context for the compiler errors told me what line numbers and types were involved:

firebase_auth_plugin.cpp(124): note: see reference to function template instantiation 'flutter::EncodableValue::EncodableValue<const uint8_t*>(T &&) noexcept' being compiled
        with
        [
            T=const uint8_t *
        ]

firebase_auth_plugin.cpp(126): note: see reference to function template instantiation 'flutter::EncodableValue::EncodableValue<uint8_t*>(T &&) noexcept' being compiled
        with
        [
            T=uint8_t *
        ]

Observe that const uint8_t * and uint8_t * can't possibly be intended to construct any of the std::variant's Core Language types (bool, int32_t, int64_t, double). (See " 🐞 This is a runtime bugfix too!" below.) And std::monostate is clearly ruled out. An individual pointer cannot be used to construct a std::vector (there is no constructor, whether implicit or explicit, for vector<T> from T*, as there would be no length information), ruling out all of the std::vector types in the std::variant as well as the EncodableList typedef. The EncodableMap typedef is also super duper ruled out.

std::string is implicitly constructible from const char* and char* but not from const uint8_t * and uint8_t * - those are distinct types.

Therefore, this must have been intending to construct CustomEncodableValue within the std::variant, by process of elimination. Changes to MSVC's std::variant implementation (see my explanation #16536 (comment)) prevent this from compiling with VS 2022 17.12 in C++17 mode (or in earlier VS versions if you were to use C++20 mode).

EncodableValue actually has a dedicated implicit constructor from CustomEncodableValue, but it can't be selected here for two reasons - first, the arguments are const uint8_t * and uint8_t *, which will exactly match the templated EncodableValue(T&& t) constructor:

https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L182-L188

  // Allow implicit conversion from CustomEncodableValue; the only reason to
  // make a CustomEncodableValue (which can only be constructed explicitly) is
  // to use it with EncodableValue, so the risk of unintended conversions is
  // minimal, and it avoids the need for the verbose:
  //   EncodableValue(CustomEncodableValue(...)).
  // NOLINTNEXTLINE(google-explicit-constructor)
  EncodableValue(const CustomEncodableValue& v) : super(v) {}

https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L198-L199

  template <class T>
  constexpr explicit EncodableValue(T&& t) noexcept : super(t) {}

And even if EncodableValue(T&& t) wasn't a better match for overload resolution, the constructor explicit CustomEncodableValue(const std::any& value) is explicit, so it cannot be silently invoked. (Once you have a CustomEncodableValue, then you can implicitly construct an EncodableValue.)

To adapt to this C++20 source-breaking change that MSVC, GCC/libstdc++, and Clang/libc++ are now implementing retroactively in C++17 mode, you can explicitly construct CustomEncodableValue on these lines. This will work with both the old and new std::variant behaviors, as the given type exactly matches one of the std::variant alternatives.

(After constructing CustomEncodableValue, I have chosen to continue explicitly constructing EncodableValue, even though that could be done implicitly. This is a style question and the other choice of return flutter::CustomEncodableValue(MEOW); would be perfectly reasonable.)

🐞 This is a runtime bugfix too!

Was the original code actually constructing a CustomEncodableValue within the std::variant in C++17 mode in VS 2022 17.11 and earlier?

No! It was constructing a bool! 🙀 Observe:

https://godbolt.org/z/7n6WTq4dq

This is the EXACT problem that P0608R3 "Improving variant's Converting Constructor/Assignment" was designed to solve - observe that its introductory example is how C++17 variant<string, bool> x = "abc"; was constructing the bool alternative. So it's good that MSVC is now implementing this retroactively and unconditionally!

There is no way that the bool alternative was intended for the firebase::Variant::kTypeStaticBlob and firebase::Variant::kTypeMutableBlob cases, and presumably this was missed because (1) this code is Windows-only and (2) there was no runtime test coverage involving this variant's value. (I didn't notice this myself until I was writing up this PR description - it is very subtle and surprising behavior, which is how it got into the C++ Standard itself before being fixed.)

This should hopefully fix the Windows CI. Hope this helps!

[russellwheatley]

@StephanTLavavej - thank you for fixing this 🙏 . See successful windows CI run: https://github.com/firebase/flutterfire/actions/runs/12190941489/job/34026804174?pr=16840

I've slightly updated code for formatting, but will look to get this merged and out in the next release.

[PrimeTheFirst]

So the issue should be fixed with the latest release?