microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.23k stars 1.51k forks source link

<vector>: VS 2019 ignores explicit keyword #1041

Open AlexGuteniev opened 4 years ago

AlexGuteniev commented 4 years ago

Describe the bug STL allows range-based insertion to vector (and other containers) thru implicit conversion operator

Command-line test case


d:\Temp2>type repro.cpp
#include <algorithm>
#include <iostream>
#include <vector>

class test
{

};

class no_implicit_convertion
{
public:

        //Here, there is no implicit convertion from one type to another,

        //however, VS2017 ignores explicit keyword.
        explicit no_implicit_convertion(test)
        {
                std::cout << "Inserted test with implicit conversion!" << std::endl;
        }
};

int main()
{
        std::vector<no_implicit_convertion> target_vec;
        std::vector<test> test_vec{ {},{},{} };

        target_vec.insert(target_vec.end(), test_vec.begin(), test_vec.end());
}

d:\Temp2>cl /EHsc /W4 repro.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.27.29009.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp
Microsoft (R) Incremental Linker Version 14.27.29009.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:repro.exe
repro.obj

d:\Temp2>.\repro.exe
Inserted test with implicit conversion!
Inserted test with implicit conversion!
Inserted test with implicit conversion!

Expected behavior Should not compile

STL version

Microsoft Visual Studio Professional 2019 Preview
Version 16.7.0 Preview 3.1

Additional context Also tracked by DevCom-275079 and VSO-636498 / AB#636498

StephanTLavavej commented 4 years ago

Apparently I believed this was by design when I saw it initially reported, but I was busy with <charconv> and didn't write up a full analysis. Need to check.

CaseyCarter commented 4 years ago

Per [sequence.reqmts]/3 the arguments to the various constructor/insert/assign overloads that accept ranges must be "iterators that meet the Cpp17InputIterator requirements and refer to elements implicitly convertible to value_type". By violating this requirement, the repro has undefined behavior.

(Note that LWG-3297 wants to strike this requirement, which would render the program well-defined with the behavior we implement.)

AlexGuteniev commented 4 years ago

Note that LWG-3297 wants to strike this requirement, which would render the program well-defined with the behavior we implement.

I agree that having this case undefined is an issue, classes with explicit constructor are common, so something well defined should be done here.

However, doing explicit conversion inside insert seems to violate least surprise principle, and defeat the purpose of explicit constructor. To me, only emplace_* methods should be allowed to do this, if any at all.

Predelnik commented 4 years ago

Also got tripped by that issue due to standard libraries behaving differently. Intuitively I think it should not be allowed without explicit cast e.g. through ranges::views::transform, even if due to iterator interface this form of casting is not the most convenient still.

frederick-vs-ja commented 1 year ago

I agree that having this case undefined is an issue, classes with explicit constructor are common, so something well defined should be done here.

What should happen if is_convertible_v<iter_reference_t<It>, value_type> is true but the construction (possibly via Alloc::construct) is not equivalent to the implicit conversion?

Note that there's uses-allocator construction, and some weird types can make difference between explicit cast and implicit conversion while both are well-formed.