Bug 79162 - [7/8 Regression] [C++17] ambiguity in string assignment due to string_view overload
Summary: [7/8 Regression] [C++17] ambiguity in string assignment due to string_view ov...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 7.0
: P1 normal
Target Milestone: 7.3
Assignee: Jonathan Wakely
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2017-01-20 12:06 UTC by Jonathan Wakely
Modified: 2017-09-20 22:26 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 6.3.0
Known to fail:
Last reconfirmed: 2017-03-09 00:00:00


Attachments
Minimized file from LLVM (760 bytes, text/plain)
2017-06-02 18:28 UTC, David Abdurachmanov
Details
This is just minimized *.ii file (332.85 KB, application/x-xz)
2017-06-02 18:52 UTC, David Abdurachmanov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2017-01-20 12:06:08 UTC
#include <string>

int main() {
 std::string s;
 s = {"abc", 1};
}

This compiled with GCC 6, but with 7 gives:

s.cc: In function ‘int main()’:
s.cc:5:15: error: ambiguous overload for ‘operator=’ (operand types are ‘std::__cxx11::string {aka std::__cxx11::basic_string<char>}’ and ‘<brace-enclosed initializer list>’)
  s = {"abc", 1};
               ^
In file included from /home/jwakely/gcc/7/include/c++/7.0.0/string:52:0,
                 from s.cc:1:
/home/jwakely/gcc/7/include/c++/7.0.0/bits/basic_string.h:627:7: note: candidate: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]
       operator=(const basic_string& __str)
       ^~~~~~~~
/home/jwakely/gcc/7/include/c++/7.0.0/bits/basic_string.h:680:7: note: candidate: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]
       operator=(basic_string&& __str)
       ^~~~~~~~
/home/jwakely/gcc/7/include/c++/7.0.0/bits/basic_string.h:747:7: note: candidate: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::__sv_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::__sv_type = std::basic_string_view<char>]
       operator=(__sv_type __sv)
       ^~~~~~~~
Comment 1 Xi Ruoyao 2017-03-09 09:02:03 UTC
I think it's a C++ Standard Library defect rather than a libstdc++ bug.

Seems similar to LWG 2758.
Comment 2 Xi Ruoyao 2017-03-09 09:07:39 UTC
> Seems similar to LWG 2758.

http://cplusplus.github.io/LWG/lwg-defects.html#2758
Comment 3 Jonathan Wakely 2017-03-09 13:40:15 UTC
(In reply to Xi Ruoyao from comment #1)
> I think it's a C++ Standard Library defect rather than a libstdc++ bug.

It can be both. We still need to fix it.

> Seems similar to LWG 2758.

But not the same.
Comment 4 Jonathan Wakely 2017-03-14 14:51:52 UTC
Author: redi
Date: Tue Mar 14 14:51:19 2017
New Revision: 246128

URL: https://gcc.gnu.org/viewcvs?rev=246128&root=gcc&view=rev
Log:
PR libstdc++/79162 disambiguate assignment from string_view

	PR libstdc++/79162
	* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
	(basic_string<C,T,A>::operator=(basic_string_view<C,T>)): Replace
	with a constrained template.
	[!_GLIBCXX_USE_CXX11_ABI]
	(basic_string<C,T,A>::operator=(basic_string_view<C,T>)): Likewise.
	* testsuite/21_strings/basic_string/cons/char/79162.cc: New test.
	* testsuite/21_strings/basic_string/cons/wchar_t/79162.cc: New test.

Added:
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/79162.cc
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/79162.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/basic_string.h
Comment 5 Jonathan Wakely 2017-03-14 14:53:00 UTC
Fixed.
Comment 6 lucdanton 2017-03-17 09:43:25 UTC
As a heads-up, deduction was possible when the constructor was a non-template:

    std::basic_string_view<CharT> sv = …;
    // produces an std::basic_string<CharT>
    auto into_string = std::basic_string { sv };

That made it possible to concisely convert a string view to its 'natural' string
type, even in a generic context. (Unless I'm mistaken there is nothing in the
current basic_string_view interface that would allow to do the same, e.g.
sv.to_string().)

Now that's it a constrained constructor template, it looks like deduction picks
an std::initializer_list<CharT> constructor. So into_string ends up being an
std::basic_string<std::basic_string_view>.

Of course the only thing that is lost is conciseness, the user can resort to any
of several other solutions.
Comment 7 David Abdurachmanov 2017-05-29 13:52:46 UTC
This potentially caused errors while compiling Clang 3.9 in C++1z mode.

See Richard Smith comments on http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170522/193826.html

Quote:
"""
This is wrong. We have a "public:" on the previous line! But I think this
is just GCC's diagnostic being screwed up and what it's trying to say is
that it selected a deleted function.

But this function is the wrong overload resolution result; the operator
std::string from the base class should be selected.
"""
Comment 8 Jonathan Wakely 2017-05-30 15:38:29 UTC
Can you reduce the failing code down to something smaller than the entirety of LLVM?

Richard also says the overload shouldn't exist and is a bug, but the overload has to exist, because the C++17 draft is defective.
Comment 9 Jonathan Wakely 2017-05-30 16:07:14 UTC
I'm unable to reproduce this with the following, based on the llvm code. GCC does the right thing here, so without a testcase there's nothing we can do.

#include <string>

template<typename T>
class storage {
public:
  operator T() const { return {}; }
};

template<class T>
class opt : public storage<T> {
public:
  explicit opt() { }
  opt(const opt&) = delete;
};

opt<std::string> pf{};

class Pass {
public:
  std::string filename;

  Pass() {
    filename = pf;
  }
};

Pass p;
Comment 10 Jonathan Wakely 2017-06-02 17:42:26 UTC
(In reply to Jonathan Wakely from comment #8)
> Richard also says the overload shouldn't exist and is a bug, but the
> overload has to exist, because the C++17 draft is defective.

That's https://wg21.link/lwg2946
Comment 11 David Abdurachmanov 2017-06-02 18:28:25 UTC
Created attachment 41461 [details]
Minimized file from LLVM
Comment 12 David Abdurachmanov 2017-06-02 18:29:46 UTC
I have attached minimized file (PGOInstrumentation.cpp) from LLVM.

Compile line: g++ -c PGOInstrumentation.cpp

Result:
PGOInstrumentation.cpp: In constructor '{anonymous}::PGOInstrumentationUseLegacyPass::PGOInstrumentationUseLegacyPass(std::__cxx11::string)':
PGOInstrumentation.cpp:105:25: error: 'llvm::cl::opt<DataType, ExternalStorage, ParserClass>::opt(const llvm::cl::opt<DataType, ExternalStorage, ParserClass>&) [with DataType = std::__cxx11::basic_string<char>; bool ExternalStorage = false; ParserClass = llvm::cl::parser<std::__cxx11::basic_string<char> >]' is private within this context
       ProfileFileName = PGOTestProfileFile;
                         ^~~~~~~~~~~~~~~~~~
PGOInstrumentation.cpp:92:3: note: declared private here
   opt(const opt &) = delete;
   ^~~
PGOInstrumentation.cpp:105:25: error: use of deleted function 'llvm::cl::opt<DataType, ExternalStorage, ParserClass>::opt(const llvm::cl::opt<DataType, ExternalStorage, ParserClass>&) [with DataType = std::__cxx11::basic_string<char>; bool ExternalStorage = false; ParserClass = llvm::cl::parser<std::__cxx11::basic_string<char> >]'
       ProfileFileName = PGOTestProfileFile;
                         ^~~~~~~~~~~~~~~~~~
PGOInstrumentation.cpp:92:3: note: declared here
   opt(const opt &) = delete;
   ^~~
PGOInstrumentation.cpp:72:2: note:   initializing argument 1 of 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_If_sv<_Tp, std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&> std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(_Tp) [with _Tp = llvm::cl::opt<std::__cxx11::basic_string<char> >; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_If_sv<_Tp, std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&> = std::__cxx11::basic_string<char>&]'
  operator=(_Tp __sv)
  ^~~~~~~~
Comment 13 David Abdurachmanov 2017-06-02 18:52:50 UTC
Created attachment 41463 [details]
This is just minimized *.ii file

I am also adding PGOInstrumentation2.cpp.xz, which is just slightly minimized original code. The previous one was minimized with C-Reduce.
Comment 14 Daniel Krügler 2017-07-15 20:51:17 UTC
(In reply to Jonathan Wakely from comment #10)
> (In reply to Jonathan Wakely from comment #8)
> > Richard also says the overload shouldn't exist and is a bug, but the
> > overload has to exist, because the C++17 draft is defective.
> 
> That's https://wg21.link/lwg2946

The problem here is that the current gcc implementation differs from the LWG 2949 wording. The wording suggests to use

template<class T>
basic_string& operator=(const T& t);

but the existing gcc approach uses effectively a by-value signature:

template<class T>
basic_string& operator=(T t);

In the example code, the model type llvm::cl::opt is not copy-constructible, but the constraints accept it, because it is (a) convertible to std::string_view (Because it inherits from std::string) and (b) it is not convertible to char*. Now after acceptance of the constraints, the template signature is instantiated, because it is a (slightly) better match than the copy_assignment operator of std::string. When this has happened, the compile error arises, because it is attempted to call the deleted (and private) copy constructor of llvm::cl::opt.

It seems to me that it would suffice to change the signature of the constrained assignment operator to use const T& instead of T, as suggested by the issue resolution proposal.

Here is a reproducer for the situation:

#include <string>

template <class DataType>
class opt : public DataType
{
  opt(const opt &) = delete;
  opt &operator=(const opt &) = delete;
public:
  opt() {}
};
    
int main() 
{
  opt<std::string> PGOTestProfileFile;
  std::string ProfileFileName;
  ProfileFileName = PGOTestProfileFile;
}

Here is a minimized emulation of basic_string that shows that the suggested fix should work (Please uncomment '#define USE_FIX' below):

#include <type_traits>
#include <string>
#include <memory>
#include <cstddef>

namespace xstd {
    
  template<class CharT, class Traits = std::char_traits<CharT>>
  struct basic_string_view
  {
     using traits_type = Traits;
     using size_type = std::size_t;
      
     constexpr basic_string_view() noexcept
      : len{0}, str{nullptr}
      {}
      
     constexpr basic_string_view(const CharT* str) 
       : len{str == nullptr ? 0 : traits_type::length(str)}, 	
         str{str}
      {}
      
      constexpr size_type
      size() const noexcept
      { return this->len; }
      
      constexpr const CharT*
      data() const noexcept
      { return this->str; }
      
  private:
     size_type len;
     const CharT* str;
  };
    
  template<class CharT, class Traits = std::char_traits<CharT>>
  struct basic_string
  {
  private:
      using sv_type = basic_string_view<CharT, Traits>;

      template<typename T, typename Res>
	  using If_sv = std::enable_if_t<
	     std::__and_<
               std::is_convertible<const T&, sv_type>,
	       std::__not_<std::is_convertible<const T&, const CharT*>>
         >::value,
	     Res>;

  public:
      
      using size_type = std::size_t;
      
      static const size_type npos = static_cast<size_type>(-1);
      
      basic_string&
      operator=(const basic_string& str);
      
      basic_string&
      operator=(const CharT* s);
      
      basic_string&
      operator=(CharT c);
      
      basic_string&
      operator=(std::initializer_list<CharT>);
      
      basic_string&
      operator=(basic_string&& str);
      
//#define USE_FIX      
      
#ifndef USE_FIX
      template<typename T>
	  If_sv<T, basic_string&>
	  operator=(T sv);
#else
      template<typename T>
	  If_sv<T, basic_string&>
	  operator=(const T& sv);
#endif

     operator sv_type() const noexcept;
           
};
    
using string = basic_string<char>;

}

namespace llvm {
namespace cl {
    
template <class DataType>
class opt_storage : public DataType
{
};
    
template <class DataType>
class opt : public opt_storage<DataType> 
{
  opt(const opt &) = delete;
  opt &operator=(const opt &) = delete;
public:
  opt();
};
    
}
}

using namespace llvm;
cl::opt<xstd::string>
    PGOTestProfileFile;

namespace {
class PGOInstrumentationUseLegacyPass {
  PGOInstrumentationUseLegacyPass(xstd::string)
  {
      ProfileFileName = PGOTestProfileFile;
  }
  xstd::string ProfileFileName;
};
}

int main() 
{
}
Comment 15 Jonathan Wakely 2017-07-17 12:55:45 UTC
Thanks, Daniel. Let's reopen this to make the T -> const T& changes.
Comment 16 Daniel Krügler 2017-07-27 18:51:15 UTC
(In reply to Jonathan Wakely from comment #15)
> Thanks, Daniel. Let's reopen this to make the T -> const T& changes.

I'm now working at that problem, thereby also attempting to implement the full P/R of LWG 2946.
Comment 17 Jonathan Wakely 2017-09-04 15:49:19 UTC
Author: redi
Date: Mon Sep  4 15:48:47 2017
New Revision: 251664

URL: https://gcc.gnu.org/viewcvs?rev=251664&root=gcc&view=rev
Log:
PR libstdc++/79162 implement LWG 2946 and LWG 2758

2017-09-04  Daniel Kruegler  <daniel.kruegler@gmail.com>

	PR libstdc++/79162
	Implement LWG 2946, LWG 2758's resolution missed further corrections
	* include/bits/basic_string.h (basic_string::compare): Add missing
	required noexcept specifications.
	(basic_string): Introduce internal _S_to_string_view and __sv_wrapper
	for implicit string_view conversion.
	(basic_string::basic_string): Fix explicit string_view conversion by
	implicit conversion using _S_to_string_view and __sv_wrapper.
	(basic_string): Introduce internal basic_string(__sv_wrapper, Alloc)
	constructor.
	(basic_string): Fix operator=(T) template by operator=(const T&)
	template for uncopyable types (PR 79162).
	(basic_string::operator+=, basic_string::append, basic_string::assign)
	(basic_string::insert, basic_string::replace, basic_string::find)
	(basic_string::rfind, basic_string::find_first_of)
	(basic_string::find_last_of, basic_string::find_first_not_of)
	(basic_string::find_last_not_of, basic_string::compare): Replace
	__sv_type argument by template const T& (LWG 2946) and correct
	documentation describing __sv_type argument.
	(basic_string::find, basic_string::rfind, basic_string::find_first_of)
	(basic_string::find_last_of, basic_string::find_first_not_of)
	(basic_string::find_last_not_of, basic_string::compare): Replace
	unconditional noexcept specification by conditional noexcept
	specification to partially balance the removal of noexcept by LWG 2946.
	* testsuite/21_strings/basic_string/79162.cc: New.
	* testsuite/21_strings/basic_string/lwg2946.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/79162.cc
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/lwg2946.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/basic_string.h
Comment 18 Jonathan Wakely 2017-09-04 17:12:52 UTC
Fixed on trunk so far.
Comment 19 Jonathan Wakely 2017-09-13 16:39:19 UTC
Author: redi
Date: Wed Sep 13 16:38:47 2017
New Revision: 252337

URL: https://gcc.gnu.org/viewcvs?rev=252337&root=gcc&view=rev
Log:
PR libstdc++/79162 implement LWG 2946 and LWG 2758

Backport from mainline
2017-09-04  Daniel Kruegler  <daniel.kruegler@gmail.com>

	PR libstdc++/79162
	Implement LWG 2946, LWG 2758's resolution missed further corrections
	* include/bits/basic_string.h (basic_string::compare): Add missing
	required noexcept specifications.
	(basic_string): Introduce internal _S_to_string_view and __sv_wrapper
	for implicit string_view conversion.
	(basic_string::basic_string): Fix explicit string_view conversion by
	implicit conversion using _S_to_string_view and __sv_wrapper.
	(basic_string): Introduce internal basic_string(__sv_wrapper, Alloc)
	constructor.
	(basic_string): Fix operator=(T) template by operator=(const T&)
	template for uncopyable types (PR 79162).
	(basic_string::operator+=, basic_string::append, basic_string::assign)
	(basic_string::insert, basic_string::replace, basic_string::find)
	(basic_string::rfind, basic_string::find_first_of)
	(basic_string::find_last_of, basic_string::find_first_not_of)
	(basic_string::find_last_not_of, basic_string::compare): Replace
	__sv_type argument by template const T& (LWG 2946) and correct
	documentation describing __sv_type argument.
	(basic_string::find, basic_string::rfind, basic_string::find_first_of)
	(basic_string::find_last_of, basic_string::find_first_not_of)
	(basic_string::find_last_not_of, basic_string::compare): Replace
	unconditional noexcept specification by conditional noexcept
	specification to partially balance the removal of noexcept by LWG 2946.
	* testsuite/21_strings/basic_string/79162.cc: New.
	* testsuite/21_strings/basic_string/lwg2946.cc: New.

Added:
    branches/gcc-7-branch/libstdc++-v3/testsuite/21_strings/basic_string/79162.cc
    branches/gcc-7-branch/libstdc++-v3/testsuite/21_strings/basic_string/lwg2946.cc
Modified:
    branches/gcc-7-branch/libstdc++-v3/ChangeLog
    branches/gcc-7-branch/libstdc++-v3/include/bits/basic_string.h
Comment 20 Jonathan Wakely 2017-09-13 16:40:05 UTC
Fixed for 7.3 - thanks to Daniel for the patch.
Comment 21 Jonathan Wakely 2017-09-20 18:00:21 UTC
Author: redi
Date: Wed Sep 20 17:59:50 2017
New Revision: 253024

URL: https://gcc.gnu.org/viewcvs?rev=253024&root=gcc&view=rev
Log:
PR libstdc++/79162 Fix std::string regression due to LWG 2946

	PR libstdc++/79162
	* include/bits/basic_string.h (basic_string::_If_sv): Remove from the
	overload set when the argument is derived from basic_string.
	* testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc: New
	test.
	* testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc:
	New test.

Added:
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc
    trunk/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/basic_string.h
Comment 22 Jonathan Wakely 2017-09-20 18:53:28 UTC
Author: redi
Date: Wed Sep 20 18:52:56 2017
New Revision: 253025

URL: https://gcc.gnu.org/viewcvs?rev=253025&root=gcc&view=rev
Log:
PR libstdc++/79162 Fix std::string regression due to LWG 2946

	PR libstdc++/79162
	* include/bits/basic_string.h (basic_string::_If_sv): Remove from the
	overload set when the argument is derived from basic_string.
	* testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc: New
	test.
	* testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc:
	New test.

Added:
    branches/gcc-7-branch/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc
    branches/gcc-7-branch/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc
Modified:
    branches/gcc-7-branch/libstdc++-v3/ChangeLog
    branches/gcc-7-branch/libstdc++-v3/include/bits/basic_string.h
Comment 23 Jonathan Wakely 2017-09-20 22:04:09 UTC
Author: redi
Date: Wed Sep 20 22:03:37 2017
New Revision: 253035

URL: https://gcc.gnu.org/viewcvs?rev=253035&root=gcc&view=rev
Log:
PR libstdc++/79162 Fix std::string regression due to LWG 2946 (old ABI)

	PR libstdc++/79162
	* include/bits/basic_string.h [!_GLIBCXX_USE_CXX11_ABI]
	(basic_string::_If_sv): Remove from the overload set when the
	argument is derived from basic_string.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/basic_string.h
Comment 24 Jonathan Wakely 2017-09-20 22:26:50 UTC
Author: redi
Date: Wed Sep 20 22:26:19 2017
New Revision: 253038

URL: https://gcc.gnu.org/viewcvs?rev=253038&root=gcc&view=rev
Log:
PR libstdc++/79162 Fix std::string regression due to LWG 2946 (old ABI)

	PR libstdc++/79162
	* include/bits/basic_string.h [!_GLIBCXX_USE_CXX11_ABI]
	(basic_string::_If_sv): Remove from the overload set when the
	argument is derived from basic_string.

Modified:
    branches/gcc-7-branch/libstdc++-v3/ChangeLog
    branches/gcc-7-branch/libstdc++-v3/include/bits/basic_string.h