fix(firebase_auth): Fix std::variant compiler errors with VS 2022 17.12
#16840
+3
−2
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16536. (I hope - see below.)
⚙️ The types involved
Flutter's
EncodableValuederives from thisstd::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
where
CustomEncodableValueis constructible fromstd::any:https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L60-L62
🛠️ How I fixed this
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):
💡 Explaining the fix
The bottom of the template instantiation context for the compiler errors told me what line numbers and types were involved:
Observe that
const uint8_t *anduint8_t *can't possibly be intended to construct any of thestd::variant's Core Language types (bool,int32_t,int64_t,double). (See " 🐞 This is a runtime bugfix too!" below.) Andstd::monostateis clearly ruled out. An individual pointer cannot be used to construct astd::vector(there is no constructor, whether implicit or explicit, forvector<T>fromT*, as there would be no length information), ruling out all of thestd::vectortypes in thestd::variantas well as theEncodableListtypedef. TheEncodableMaptypedef is also super duper ruled out.std::stringis implicitly constructible fromconst char*andchar*but not fromconst uint8_t *anduint8_t *- those are distinct types.Therefore, this must have been intending to construct
CustomEncodableValuewithin thestd::variant, by process of elimination. Changes to MSVC'sstd::variantimplementation (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).EncodableValueactually has a dedicated implicit constructor fromCustomEncodableValue, but it can't be selected here for two reasons - first, the arguments areconst uint8_t *anduint8_t *, which will exactly match the templatedEncodableValue(T&& t)constructor:https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L182-L188
https://github.com/flutter/engine/blob/cea11063ae318691f0ede4ef12f2978ad01aff70/shell/platform/common/client_wrapper/include/flutter/encodable_value.h#L198-L199
And even if
EncodableValue(T&& t)wasn't a better match for overload resolution, the constructorexplicit CustomEncodableValue(const std::any& value)isexplicit, so it cannot be silently invoked. (Once you have aCustomEncodableValue, then you can implicitly construct anEncodableValue.)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
CustomEncodableValueon these lines. This will work with both the old and newstd::variantbehaviors, as the given type exactly matches one of thestd::variantalternatives.(After constructing
CustomEncodableValue, I have chosen to continue explicitly constructingEncodableValue, even though that could be done implicitly. This is a style question and the other choice ofreturn flutter::CustomEncodableValue(MEOW);would be perfectly reasonable.)🐞 This is a runtime bugfix too!
Was the original code actually constructing a
CustomEncodableValuewithin thestd::variantin 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++17variant<string, bool> x = "abc";was constructing theboolalternative. So it's good that MSVC is now implementing this retroactively and unconditionally!There is no way that the
boolalternative was intended for thefirebase::Variant::kTypeStaticBlobandfirebase::Variant::kTypeMutableBlobcases, and presumably this was missed because (1) this code is Windows-only and (2) there was no runtime test coverage involving thisvariant'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!