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

Validity check simple replacement fields #4078

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

ecatmur
Copy link
Contributor

@ecatmur ecatmur commented Oct 10, 2023

A custom formatter for a user-defined type might reject omitted (or empty) format-specs:

struct A {};
template<>
struct std::formatter<A> {
    constexpr auto parse(std::format_parse_context const& ctx) {
        std::string_view s("narf");
        auto [i, j] = std::mismatch(ctx.begin(), ctx.end(), s.begin(), s.end());
        if (j != s.end())
            throw std::runtime_error("you didn't say the magic word!");
        return i;
    }
    auto format(A, std::format_context& ctx) const {
        return ctx.out();
    }
};
int main() {
    std::ignore = std::format("{}", A());
}

Per [format.fmt.string]/3 this should be rejected, but MSVC instead accepts and throws at runtime.

This implementation seems most likely to not cause problems with existing code; it matches the runtime behavior of _Default_arg_formatter on custom formatters (basic_format_arg<_Context>::handle), so it should accept any format() call that would succeed at runtime.

A custom formatter for a user-defined type might reject omitted format-specs:

```c++
struct A {};
template<>
struct std::formatter<A> {
    constexpr auto parse(std::format_parse_context const& ctx) {
        std::string_view s("narf");
        auto [i, j] = std::mismatch(ctx.begin(), ctx.end(), s.begin(), s.end());
        if (j != s.end())
            throw std::runtime_error("you didn't say the magic word!");
        return i;
    }
    auto format(A, std::format_context& ctx) const {
        return ctx.out();
    }
};
int main() {
    std::ignore = std::format("{}", A());
}
```

Per [format.fmt.string]/3 this should be rejected, but MSVC instead accepts and throws at runtime.

This implementation seems most likely to not cause problems with existing code; it matches the runtime behavior of _Default_arg_formatter on custom formatters (basic_format_arg<_Context>::handle), so it should accept any format() call that would succeed at runtime.
@ecatmur ecatmur requested a review from a team as a code owner October 10, 2023 01:37
stl/inc/format Outdated Show resolved Hide resolved
ecatmur and others added 2 commits October 9, 2023 22:04
Co-authored-by: A. Jiang <de34@live.cn>
@frederick-vs-ja

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej added bug Something isn't working format C++20/23 format labels Oct 10, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 13, 2023
stl/inc/format Outdated Show resolved Hide resolved
@@ -3519,7 +3519,10 @@ struct _Format_checker {
consteval explicit _Format_checker(basic_string_view<_CharT> _Fmt) noexcept
: _Parse_context(_Fmt, _Num_args), _Parse_funcs{&_Compile_time_parse_format_specs<_Args, _ParseContext>...} {}
constexpr void _On_text(const _CharT*, const _CharT*) const noexcept {}
constexpr void _On_replacement_field(size_t, const _CharT*) const noexcept {}
constexpr void _On_replacement_field(const size_t _Id, const _CharT*) const {
_ParseContext _Parse_ctx({});
Copy link
Member

@CaseyCarter CaseyCarter Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this isn't conforming, since this parse context is very observably different from what the standard says we will pass to the user's parse function (i.e., to_address(_Parse_ctx.begin()) is not in the user-provided format string). I am not requesting a change, however, since this is consistent with how we handle simple replacement fields when actually formatting - we should maybe address this globally. @barcharcraz what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I think #4640 addresses this.

@StephanTLavavej StephanTLavavej removed their assignment Oct 30, 2023
@StephanTLavavej StephanTLavavej self-assigned this Nov 6, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 085cd7b into microsoft:main Nov 7, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for noticing and fixing this problem in <format>! 😻 🛠️ ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants