[clang] template specialization for integer value matches wrong type

[vasama]

https://godbolt.org/z/MnqssGY1b

template<typename T, auto V>
struct S
{
    static constexpr bool value = true;
};

template<typename T>
struct S<T, 0>
{
    static constexpr bool value = false;
};

static_assert(S<void, 0L>::value);

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Lauri Vasama (vasama)

https://godbolt.org/z/MnqssGY1b

template<typename T, auto V>
struct S
{
    static constexpr bool value = true;
};

template<typename T>
struct S<T, 0>
{
    static constexpr bool value = false;
};

static_assert(S<void, 0L>::value);

[BaLiKfromUA]

Happy to look on this one if this is okay :)

[cor3ntin]

@mizvekov

[BaLiKfromUA]

Update: I found time to familiarize myself with the related code, and I will try to submit the patch on the weekend.

Also trying to keep an eye on the PRs of @mizvekov since it looks like their recent work was in similar parts of the code.

[BaLiKfromUA]

Hi folks!

I've made some progress and wanna share the update.

Short version:

I believe that Clang is doing the right thing according to C++ standard and no patch is needed.

Long version:

Investigation of a codebase

I started my investigation by checking how parsing of non-type template arguments works and didn't find anything suspicious.
I also checked the produced AST and found that 0L integral value is correctly stored in AST so assumptions that we changed type somewhere during parsing were dismissed:

| |-ClassTemplateSpecializationDecl 0x2de9c020 <line:7:1, line:11:1> line:8:8 struct S definition implicit_instantiation
| | |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
.........................
| | |-TemplateArgument type 'void'
| | | `-BuiltinType 0x2de258d0 'void'
| | |-TemplateArgument integral '0L'

Then I tried to find functionality that checks "matching" of template arguments.

I found isSameTemplateArg function and looks like it's the main cause of the described issue.
In the first part of this function we have next logic:

/// Determine whether two template arguments are the same.
static bool (ASTContext &Context,
                              TemplateArgument X,
                              const TemplateArgument &Y,
                              ...) {
   .... 
  if (X.getKind() != Y.getKind())
    return false;

  switch (X.getKind()) {
    ....
     case TemplateArgument::Integral:
      return hasSameExtendedValue(X.getAsIntegral(), Y.getAsIntegral());
    ....
  }
  ....
}

And hasSameExtendedValue returns true when we are trying to compare 0L to 0, which makes sense since the value is the same both in long and integer representations.

As an experiment, I tried to modify this branch and added additional checks like Context.hasSameType(X.getIntegralType(), Y.getIntegralType()).
And it helped, the checks below started passing.

static_assert(S<void, 0L>::value == true);
static_assert(S<void, 0>::value == false);

Obviously, this change fails some of the existing tests, e.g several test cases inside /test/SemaTemplate/temp_arg_nontype_cxx1z.cpp file.

These test cases rely on logic that we could successfully match one integer type with another different-width integer type as long as a value is the same in both representations (no narrowing conversions).

So I started checking if hasSameExtendedValue makes sense or not, from the perspective of C++ standard.

Explanation of expected behaviour

Disclaimer: Some of the wording might be misinterpreted by me so please let me know if I missed something.

I found the next statement about non-type template argument:

A template-argument for a non-type template-parameter shall be a converted constant expression (7.7) of the
type of the template-parameter.

Also, while looking into similar issues reports, I found this explanation which states that we could do the conversion of non-type template argument as long as the conversion is not narrowing.

One of the definitions of narrowing conversion is:

A narrowing conversion is an implicit conversion from an integer type or unscoped enumeration type to an integer type that cannot represent all the values of the original type, except the source is a constant expression whose value after integral promotions will fit into the target type.

If I am reading correctly, looks like conversion from 0L to 0 matches the exception, and clang is doing the right thing.

Request for input

I would appreciate if someone could double-check the correctness of my reasoning in the long version.

[mizvekov]

I think clang is wrong. For an NTTP of placeholder type, the type does matter. S<void, 0L> should deduce V to long, while the partial specialization is declared for a V of type int.

This issue arises out of a simplification we do in clang, which probably predates placeholder NTTP, where we leave the template argument conversions to a later step after deduction, instead of during it.

That's why isSameTemplateArgument, which is used only for deduction, does not look at the NTTP type, unlike our canonicalization for template arguments: for a template parameter of non-placeholder type, the template argument will be converted to the type of the template parameter, so not looking at the type is correct in this case, as if we did things by the books, the types would have been the same anyway at the point where we should be comparing them.

For an NTTP of placeholder type that is incorrect though, as for each template argument, the type of the NTTP will be deduced differently and conversions do not apply. So really if we would like to maintain the current strategy of deferring conversions, in this case we should also look if the template parameter would have been deduced to the same type.