Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fmtlib trunk] Including both "fmt/std.h" and "fmt.ranges.h" causes Visual Studio compiler error #2954

Closed
Dani-Hub opened this issue Jun 30, 2022 · 5 comments

Comments

@Dani-Hub
Copy link
Contributor

Dani-Hub commented Jun 30, 2022

Tested on fmtlib trunk downloaded at 2022-06-29. The following complete code leads to a compiler error for Visual Studio compilers (Tested for VS 2019 and VS 2022 targeting C++17):

#include "fmt/ranges.h"
#include "fmt/std.h"

int main()
{
  std::filesystem::path p("data/somewhat");
  fmt::format("Path={}", p);
}

The produced error is:

1>XXX\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\type_traits(593,1): error C2752: 'fmt::v8::formatter<std::filesystem::path,char,void>': more than one partial specialization matches the template argument list
1>XXX\fmt-master\include\fmt\ranges.h(396,1): message : could be 'fmt::v8::formatter<R,Char,std::enable_if<fmt::v8::is_range<T,Char>::value,void>::type>'
1>XXX\fmt-master\include\fmt\std.h(54,1): message : or       'fmt::v8::formatter<std::filesystem::path,Char,void>'
1>XXX\fmt-master\include\fmt\core.h(1485): message : see reference to class template instantiation 'std::is_constructible<fmt::v8::formatter<std::filesystem::path,char,void>>' being compiled
1>XXX\fmt-master\include\fmt\core.h(1679): message : see reference to alias template instantiation 'fmt::v8::detail::mapped_type_constant<std::filesystem::path,fmt::v8::format_context>' being compiled
1>XXX\fmt-master\include\fmt\core.h(1840): message : see reference to function template instantiation 'unsigned __int64 fmt::v8::detail::encode_types<Context,std::filesystem::path,>(void)' being compiled
1>        with
1>        [
1>            Context=fmt::v8::format_context
1>        ]
1>XXX\fmt-master\include\fmt\core.h(3224): message : see reference to class template instantiation 'fmt::v8::format_arg_store<fmt::v8::format_context,std::filesystem::path>' being compiled
1>XXX\Cpp\vs2019\fmtlib-composition\fmt-issue.cpp(7): message : see reference to function template instantiation 'void fmt::v8::print<std::filesystem::path&>(fmt::v8::basic_format_string<char,std::filesystem::path &>,std::filesystem::path &)' being compiled
1>XXX\fmt-master\include\fmt\core.h(1678): error C2062: type 'unknown-type' unexpected
1>XXX\fmt-master\include\fmt\core.h(1678): error C2144: syntax error: 'unknown-type' should be preceded by '('
1>XXX\fmt-master\include\fmt\core.h(1679,68): error C2039: 'value': is not a member of '`global namespace''
1>XXX\fmt-master\include\fmt\core.h(1837,56): error C3615: constexpr function 'fmt::v8::detail::encode_types' cannot result in a constant expression
1>XXX\fmt-master\include\fmt\core.h(1840): message : failure was caused by control reaching the end of a constexpr function

I'm aware that the problem does not occur for gcc or clang, the most likely reason is the special exclusion code involving

#if !FMT_MSC_VERSION

in ranges.h#L401, but we really need to support VS 2019 for quite a while. The reason for that workaround code is not quite clear to me, but it is possible as a workaround even in the presence of this workaround to replace the partial specialization in std.h#L54 by two specializations

template <>
struct formatter<std::filesystem::path, char>;
template <>
struct formatter<std::filesystem::path, wchar_t>;

to solve the issue.

The issue is rather severe, because it happens quite often that you need both ranges.h and std.h in the same translation unit.

I recommend to add at least the here described code as test case regardless of the actual solution.

I'd like to point out that we stumbled across this problem when we borrowed the technique described in issue 2778 but using the same partial template specialization technique that is used in std.h, so this also likely happens if user-code attempts to adopt the technique described in that issue.

I'm open to make a corresponding pull request, but would like to get feedback on the preferred resolution.

@vitaut
Copy link
Contributor

vitaut commented Jul 1, 2022

Thanks for reporting. Could you check if this workaround

fmt/include/fmt/ranges.h

Lines 400 to 401 in e29c2bc

// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VERSION
is still needed on your system? The condition has changed since it was first introduced so it might no longer be required.

@Dani-Hub
Copy link
Contributor Author

Dani-Hub commented Jul 1, 2022

Could you check if this workaround

fmt/include/fmt/ranges.h

Lines 400 to 401 in e29c2bc

// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VERSION

is still needed on your system?

Sure. I assume you mean I should try to build the project via cmake without this conditional or do you have a special test case in mind that was the reason to introduce it?

@Dani-Hub
Copy link
Contributor Author

Dani-Hub commented Jul 1, 2022

Sure. I assume you mean I should try to build the project via cmake without this conditional or do you have a special test case in mind that was the reason to introduce it?

Short feedback: Just eliminating the conditional makes the Visual Studio builds fail for 2019 and 2022, so the existing comment should be extended to mention 2022 as well. I have not attempted to fully understand the source of the problem right now, this would need more time.

@Dani-Hub
Copy link
Contributor Author

Dani-Hub commented Jul 2, 2022

I made progress on this issue. I found the following to be an effective approach:
For Visual Studio > 2017 the here reported partial specialization problem as well as the extra preprocessor conditional in the two formatter specializations for maps and ranges in ranges.h can be got rid of by replacing && and || within the conditional code by lazy my_and and my_or class templates (rewriting std::conjunction and std::disjunction so that the code works even without depending on C++17 and for compilers that don't support these type traits). For Visual Studio 2017 (I have not tested older ones but I suspect the same problem exists for them, too) we still need both the preprocessor exclusion within the two formatter specializations for maps and ranges in ranges.h and we need the splitting of single partial specialization of the formatter for std::filesystem::path in std.h into two explicit specializations.

I'm planning to make a pull-request following this suggestion, OK?

@vitaut
Copy link
Contributor

vitaut commented Jul 2, 2022

I'm planning to make a pull-request following this suggestion, OK?

Sounds good. Thanks for investigating the issue!

Dani-Hub added a commit to Dani-Hub/fmt that referenced this issue Jul 2, 2022
…tter specializations for ranges and maps more robust (especially for Visual Studio compiler family)
Dani-Hub added a commit to Dani-Hub/fmt that referenced this issue Jul 2, 2022
…td::filesystem::path> partial template specialization into two explicit specialization.
Dani-Hub added a commit to Dani-Hub/fmt that referenced this issue Jul 2, 2022
Dani-Hub added a commit to Dani-Hub/fmt that referenced this issue Jul 2, 2022
…tter specializations for ranges and maps more robust (especially for Visual Studio compiler family)
Dani-Hub added a commit to Dani-Hub/fmt that referenced this issue Jul 2, 2022
…td::filesystem::path> partial template specialization into two explicit specialization.
vitaut pushed a commit that referenced this issue Jul 3, 2022
…2957)

* #2954: Provide std::conjunction and std::disjunction substitutes

* #2954: Use conjunction and disjunction substitute to make formatter specializations for ranges and maps more robust (especially for Visual Studio compiler family)

* #2954: As workaround for older MSVC compilers split formatter<std::filesystem::path> partial template specialization into two explicit specialization.

* 2954: Add test case

* Provide simplified implementations of conjunction and disjunction

* Remove workaround explicit specializations if the partial specialization would cause an ambiguity error

* Eliminate extra-test and merge it into existing std-test instead. Add conditionals for filesystem::path testing that does not run into the ambiguity problem.
@vitaut vitaut closed this as completed Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants