Narrowing conversions in pair and tuple list-initialization

[David Krauss]

One might expect list-initialization of std::pair to catch narrowing conversions, but the perfect-forwarding constructor preempts the safety.

std::pair< bool, char > pbc = { 1000, 1000 }; // OK, narrowing/overflow.

std::map< char, int > mci = { { 0, 0 }, { 256, 1 } }; // Likewise.

In this simple case, the problem could be fixed by SFINAE-disabling the constructor template for numeric types. But when mixing numeric and nontrivial types, there’s a trade-off between catching the conversion and supporting in-place conversion.

std::pair< char, std::string > mcs = { 1000, "hi" };

The status quo makes this OK. Disabling the constructor template would catch the error but add a move-construction to well-formed cases. Disabling template parameters one-by-one would provide an optimal but complex solution. For std::tuple, everything is proportionately more complicated.

Does this problem merit a DR? It’s rather minor, and the solution space is hairy. The potential breakage looks substantial, but on the other hand, it’s all in the name of safety.

[Nicol Bolas]

I think the item is worthy of a DR, but I don't think your solution is appropriate.

The reason being that it would affect users who do `std::pair<bool, char> mcs(1000, 1000);` too. They didn't use braced-init-lists, so why are narrowing conversions forbidden for them?

I'd say that the ultimate problem is the desire for a converting constructor on `tuple` at all. So long as we want users to be able to pass values that do not correspond to the tuple elements' types, the braced-init-lists will be powerless to save you from narrowing conversions.

[Matthew Woehlke]

On 2015-11-20 02:12, David Krauss wrote:
> One might expect list-initialization of std::pair to catch narrowing conversions, but the perfect-forwarding constructor preempts the safety.
>
> std::pair< bool, char > pbc = { 1000, 1000 }; // OK, narrowing/overflow.
> std::map< char, int > mci = { { 0, 0 }, { 256, 1 } }; // Likewise.

AIEE! It doesn't trip -Wconversion or -Woverflow either. That's... horrible.

I'd file compiler bugs (in addition to a possible language DR). Whether
or not the standard technically permits that, I consider it a bug that
compilers aren't (at least) warning about it. Especially in the above
example which is *statically* broken (i.e. compiler can prove at compile
time that a bad conversion will happen).

Note: apparently the 1000 -> bool conversion is okay (at least, gcc
doesn't squawk about 'bool b = 1000;', though MSVC might squawk its
semi-infamous "performance warning").

--
Matthew

[dyp]

Possible workaround? Add more braces:

std::pair< bool, char > pbc = { {1000}, {1000} }; // error: narrowing
conversion
std::map< char, int > mci = { { {0}, {0} }, { {256}, {1} } }; // error:
narrowing conversion

I'm not convinced this is an appropriate solution, though. Maybe one
could help the compiler by using an implicit_cast function template:

template<class To>
To implicit_cast(To t)
{
return t;
}

template<class T, class U>
struct pair
{
T t;
U u;
template<class X, class Y>
pair(X x, Y y)
: t(implicit_cast<T>(x)), u(implicit_cast<U>(y))
{}
};

But this does not emit warnings as specific as the narrowing conversions
in braced-init-list: clang++ and g++ don't seem to care about the value,
even if it is known at compile-time.

[Ville Voutilainen]

On 20 November 2015 at 21:09, dyp <dyp...@gmx.net> wrote:
> But this does not emit warnings as specific as the narrowing conversions
> in braced-init-list: clang++ and g++ don't seem to care about the value,
> even if it is known at compile-time.

There's fairly little they can do, because the library implementation is valid
and doesn't call for a diagnostic.

Potential fixes are apparently more complex than they seem to the naked
eye; this code is valid:

int x = {1LL};

This means that pure type-based analysis will not help a library implementation
that would want to rely on a type trait. It looks like a proper fix
would require a
real compiler-magic function that can look at both types and values.

[dyp]

I'm not sure if we have a misunderstanding, so I'll try to clarify what
I wanted to say:

I was referring to my modified example, where the conversion is implicit
rather than explicit. That is, std::pair currently uses something like:

template<class X, class Y>
pair(X x, Y y)

: t(x), u(y)
{}

(with forwarding refs + std::forward or const lvalue refs)

whereas I've tried to make that conversion from X to T and Y to U implicit:

template<class X, class Y>
pair(X x, Y y)
: t(implicit_cast<T>(x)), u(implicit_cast<U>(y))
{}

This is related to Daniel Krügler et al.'s improvements on pair and
tuple: This implicit std::pair ctor performs explicit conversions.


An explicit conversion expresses intent, e.g. of a truncation:

static_cast<unsigned char>(1000)
or
unsigned char c(1000); // direct-init is related to explicit conversion

so a compiler should IMHO not emit a diagnostic for explicit conversion
(g++ and clang++ do emit a warning for the direct-init, but not for the
static_cast).

Implicit conversions OTOH should be considered safe conversions IMHO,
which is why I'd expect a diagnostic when potential narrowing occurs:

unsigned char c = 1000;
or
void foo(unsigned char);
foo(1000)


By adding the implicit_cast into the pair ctor, I make use of this
difference to tell the compiler that a truncation is not intended. g++
and clang++ understand this and emit a warning. However, this warning is
triggered solely by looking at the types involved; the compilers do not
perform (enough) static analysis to propagate the value across the
function border. So this technique does not get false negatives as in
David Krauss' example, but it gets false positives. A more
computationally expensive static analyzer could distinguish those cases
and perform a check similar to the narrowing conversion check we
currently have in compilers for braced-init-lists.

[Matthew Woehlke]

On 2015-11-20 14:14, Ville Voutilainen wrote:
> Potential fixes are apparently more complex than they seem to the naked
> eye; this code is valid:
>
> int x = {1LL};

I'd treat this as a red herring... *literals* appear to follow their own
rules. For instance, this:

char i = 0.0; // or {0.0}

...triggers neither a narrowing warning (error) not a conversion
warning. IOW, at least GCC will silently permit things that would
otherwise be "bad" if it statically knows that no problem will occur.
(Note: AFAICT, 'int x = {1LL}' is parsed as 'int x = int{1LL}' and not
as an initializer list.)

That doesn't excuse David's case of assigning 1000 to a char. That ought
to trip -Woverflow due to the compiler statically detecting that
overflow WILL occur. (It's also narrowing, of course, but that the
compiler flags neither the narrowing *nor the statically detectable
overflow* is just really, really bad.)

> This means that pure type-based analysis will not help a library implementation
> that would want to rely on a type trait.

Nope... red herring. This:

auto i = 1LL;
int x = i;

...triggers a -Wconversion warning as you'd expect. And even better,
this is a hard error:

auto i = {1LL};
int x = i; // error: can't convert initializer_list<long long> to int

Silent narrowing only occurs when trying to stuff a literal into a
smaller type where it is statically safe to do so. As soon as the
literal is assigned to a typed symbol, you start getting diagnostics as
expected. (Except in the case David noted.) So type-based analysis
should be fine; you won't be dealing with vaguely-typed literals at that
point.

Although, what we actually, *really* need is for the literal to not stop
being a literal (i.e. losing the combination of vague type and
statically-known value) quite so soon. This, unfortunately, sounds less
like a DR and more like a feature.

IOW, this:

auto x = {1LL}; // decltype(x) == std::integer_literal<1>

--
Matthew

[Ville Voutilainen]

On 20 November 2015 at 21:46, Matthew Woehlke <mwoehlk...@gmail.com> wrote:
>> This means that pure type-based analysis will not help a library implementation
>> that would want to rely on a type trait.
> Nope... red herring. This:
> auto i = 1LL;
> int x = i;
> ...triggers a -Wconversion warning as you'd expect. And even better,
> this is a hard error:

The example is meant to demonstrate a difference between
"raw code" and the possibility of a library wrapper doing the
same. "red herring"? Do tell me how you're going to specify
banning narrowing conversions in a library type? How are you
going to implement it? Without false positives?

[Matthew Woehlke]

On 2015-11-20 15:51, Ville Voutilainen wrote:
> On 20 November 2015 at 21:46, Matthew Woehlke <mwoehlk...@gmail.com> wrote:
>>> This means that pure type-based analysis will not help a library implementation
>>> that would want to rely on a type trait.
>> Nope... red herring. This:
>> auto i = 1LL;
>> int x = i;
>> ...triggers a -Wconversion warning as you'd expect. And even better,
>> this is a hard error:
>
> The example is meant to demonstrate a difference between
> "raw code" and the possibility of a library wrapper doing the
> same. "red herring"?

Maybe I misunderstood your point. My impression was that you are saying
that a library can't tell if a bad conversion *may* be occurring due to
compiler magic related to literals. That's not the case.

But it sounds like maybe what you meant was that a library doesn't have
access to said magic. That's true (currently), but a lesser problem; the
former would imply that a library has no hope of diagnosing real bugs.
The latter may be in the way of statically detecting definite bugs.

> Do tell me how you're going to specify banning narrowing conversions
> in a library type? How are you going to implement it? Without false
> positives?

"False" *positives* are indeed a problem, although I briefly mentioned
the solution; delay converting literals to regular types until needed.
False positives are more acceptable however than false negatives; a
false positive can be fixed with an explicit cast (especially if they
result in a warning rather than a hard error). A false negative is an
undetected bug and is not acceptable.

--
Matthew

[Richard Smith]

On Fri, Nov 20, 2015 at 1:41 PM, Matthew Woehlke <mwoehlk...@gmail.com> wrote:

On 2015-11-20 15:51, Ville Voutilainen wrote:
> On 20 November 2015 at 21:46, Matthew Woehlke <mwoehlk...@gmail.com> wrote:
>>> This means that pure type-based analysis will not help a library implementation
>>> that would want to rely on a type trait.
>> Nope... red herring. This:
>>   auto i = 1LL;
>>   int x = i;
>> ...triggers a -Wconversion warning as you'd expect. And even better,
>> this is a hard error:
>
> The example is meant to demonstrate a difference between
> "raw code" and the possibility of a library wrapper doing the
> same. "red herring"?

Maybe I misunderstood your point. My impression was that you are saying
that a library can't tell if a bad conversion *may* be occurring due to
compiler magic related to literals. That's not the case.

But it sounds like maybe what you meant was that a library doesn't have
access to said magic. That's true (currently), but a lesser problem; the
former would imply that a library has no hope of diagnosing real bugs.
The latter may be in the way of statically detecting definite bugs.

I think you're somewhat missing the point. The proposal was to disallow narrowing conversions when list-initializing a std::pair. However, converting a non-constant int to a char is narrowing and converting an in-bounds constant int to a char is *not* narrowing, so this seems very challenging for a library to do correctly. It's not about good conversions versus bad ones, it's about the core language definition of "narrowing conversion". If we want to provide a good experience to programmers, the narrowing conversion rules should be the same for struct types and for library types such as pair and tuple.

Possibly we should address this by also applying the narrowing-conversion rules to list initializations that call constructors. But even with such a rule, it's not clear how to form an overload set for tuple constructors that does the right thing. For pair, you could try this:

template<typename A, typename B> struct pair {

  template<typename T, typename U>

  pair(T&&, U&&)

  requires Convertible<T&&, A> && Convertible<U&&, B> && !Narrowing<T, A> && !Narrowing<U, B>;

  template<typename T>

  pair(A, T&&)

  requires Convertible<T&&, B> && !Narrowing<T, B>;

  template<typename T>

  pair(T&&, B)

  requires Convertible<T&&, A> && !Narrowing<T, A>;

  pair(A, B);

};

... but it's a mess, and for tuple you're going to need O(2^N) constructors.

[With a natural extension to P0091's deduction guides, you could imagine something like:

template<typename Param, typename Arg> struct select_ctor_param { /*...*/ };

template<typename A, typename B> struct pair {

private:

  enum selected_tag {};

  template<typename T, typename U, selected_tag> pair(T t, U u) : first(std::forward<T>(t)), second(std::forward<U>(u)) {}

public:

  template<typename T, typename U> pair(T&&, U&&) -> pair<select_ctor_param<A, T>, select_ctor_param<B, U>, selected_tag()>;

};

... and the same would work for std::tuple.]

> Do tell me how you're going to specify banning narrowing conversions
> in a library type? How are you going to implement it? Without false
> positives?

"False" *positives* are indeed a problem, although I briefly mentioned
the solution; delay converting literals to regular types until needed.
False positives are more acceptable however than false negatives; a
false positive can be fixed with an explicit cast (especially if they
result in a warning rather than a hard error). A false negative is an
undetected bug and is not acceptable.

--
Matthew

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussio...@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at http://groups.google.com/a/isocpp.org/group/std-discussion/.

[David Krauss]

On 2015–11–21, at 6:02 AM, Richard Smith <ric...@metafoo.co.uk> wrote:

Possibly we should address this by also applying the narrowing-conversion rules to list initializations that call constructors.

Narrowing already applies.

But even with such a rule, it's not clear how to form an overload set for tuple constructors that does the right thing. For pair, you could try this:

... but it's a mess, and for tuple you're going to need O(2^N) constructors.

Right… and no apparent way to even implement that explosion. I racked my brain and got an almost-solution: you can pass a parameter pack through an alias template to make deduction optional. This avoids the explosion:

template< typename mem, typename arg >

using force = mem;

template< typename mem, typename arg >

using deduce = arg;

template< bool, template< typename ... > class a, template< typename ... > class b >

struct conditional_temp {

    template< typename ... t >

    using temp = a< t ... >;

};

template< template< typename ... > class a, template< typename ... > class b >

struct conditional_temp< false, a, b > {

    template< typename ... t >

    using temp = b< t ... >;

};

template< typename ... mem >

struct tuple : std::tuple< mem ... > {

    template< typename ... arg >

    tuple( typename conditional_temp< std::is_numeric< mem >::value, force, deduce >

            ::template temp< mem, arg > ... ) {

    }

};

But I don’t think it’s possible, from that point, to deduce the arg elements that got passed to force. If only defaults were allowed on packs, it might have a shot:

    template< typename ... arg = mem ... > // or < typename ... arg = void >

    tuple( typename conditional_temp< std::is_numeric< mem >::value, force, deduce >

            ::template temp< mem, arg > ... ) {

Also, neither Clang and GCC can process this even when force is unused. Both seem to have issues with member alias templates.