Instantiate static constexpr data members on MS ABI.

[zoecarver]

Summary

Because MSVC will not treat the definition of a static data member as part of the declaration, it will not get instantiated. This means Clang won't diagnose instantiation errors until the data member itself is used.

Note: this only happens for constexpr data members because they are the only ones that are marked as implicitly inline and therefore not part of the declaration.

This patch makes Clang instantiate those data members when the class is instantiated. This matches the behavior for other targets, the behavior of MSVC, and is more consistent with the standard.

Refs: https://llvm.org/PR49618
Refs: https://github.com/apple/swift/pull/35962

[zoecarver]

So, as I mentioned in the description of this patch, the reason that these members are treated as definitions is that Clang implicitly marks them as inline when targeting MS. This is sort of interesting a) because it only applies to constexpr vars, and b) because this also happens on all platforms for std=c++17. This means that the following program will compile successfully with std=c++17:

template <class T>
struct GetTypeValue {
  static constexpr const bool value = T::value;
};

using Invalid = GetTypeValue<int>;

However, if the constexpr is removed, it will fail to compile. Conversely, after this patch is applied, that will not happen on Windows (it will fail to compile regardless of the targeted standard or constexprness). I think it would make sense to "fix" this for std=c++17 as well, but I'm not sure if that's in line with the standard.

[rsmith]

In D98904#2636021, @zoecarver wrote:

So, as I mentioned in the description of this patch, the reason that these members are treated as definitions is that Clang implicitly marks them as inline when targeting MS. This is sort of interesting a) because it only applies to constexpr vars, and b) because this also happens on all platforms for std=c++17. This means that the following program will compile successfully with std=c++17:

template <class T>
struct GetTypeValue {
  static constexpr const bool value = T::value;
};

using Invalid = GetTypeValue<int>;

(I think you're missing something here to trigger the instantiation of the class, such as Invalid x;.)

However, if the constexpr is removed, it will fail to compile. Conversely, after this patch is applied, that will not happen on Windows (it will fail to compile regardless of the targeted standard or constexprness). I think it would make sense to "fix" this for std=c++17 as well, but I'm not sure if that's in line with the standard.

The above example is valid in C++17 onwards, because in C++17 onwards, static constexpr data members are implicitly inline, and the delayed instantiation behavior here is correct for inline static data members. We'll need to distinguish here between the case where the variable is implicitly inline solely because of the MS ABI rules and the case where the variable is inline because of the actual language rules (where it's declared either inline or constexpr).

[zoecarver]

(I think you're missing something here to trigger the instantiation of the class, such as Invalid x;.)

Yes, you're right.

The above example is valid in C++17 onwards, because in C++17 onwards, static constexpr data members are implicitly inline, and the delayed instantiation behavior here is correct for inline static data members. We'll need to distinguish here between the case where the variable is implicitly inline solely because of the MS ABI rules and the case where the variable is inline because of the actual language rules (where it's declared either inline or constexpr).

Hmm. This is interesting. Thanks for explaining. Because this patch isn't going to help solve the Swift interop issue anymore, I don't feel a need to continue with it. However, if you still think it would be a valuable change to make, I'm happy to continue working on it.

I think we'd basically need a condition that says: is-microsoft && less-than-cxx17 && is-constexpr && is-static-data-member.

[rsmith]

In D98904#2636109, @zoecarver wrote:

I think we'd basically need a condition that says: is-microsoft && less-than-cxx17 && is-constexpr && is-static-data-member.

Yes.

Looking back over the history a bit here, https://reviews.llvm.org/D47956 suggests that treating these static constexpr data members as implicitly inline matches the MS behavior. And I don't think I can disprove that: considering https://godbolt.org/z/7z6T3q, it looks like MSVC gets the "don't instantiate the initializer of an inline static data member with the class" rule wrong in general, which (perhaps by chance alone) means that MSVC doesn't have the same accepts-invalid bug that we have, but only because it's being hidden by a rejects-valid bug for the same case!

So. I think the status quo is OK but not great; we accept invalid code, in the name of MSVC compatibility, that MSVC doesn't accept. I don't think following MSVC would be a good thing, as that'd lead to our rejecting valid code (that MSVC also rejects but that we currently accept). If we can split the difference, and eagerly instantiate only in the case where the language rules say the variable is not inline but MSVC says it is inline, that would be an improvement, but it seems awkward as I think we've already lost the relevant information by the point we need to make the decision.

[zoecarver]

So. I think the status quo is OK but not great; we accept invalid code, in the name of MSVC compatibility, that MSVC doesn't accept. I don't think following MSVC would be a good thing, as that'd lead to our rejecting valid code (that MSVC also rejects but that we currently accept). If we can split the difference, and eagerly instantiate only in the case where the language rules say the variable is not inline but MSVC says it is inline, that would be an improvement, but it seems awkward as I think we've already lost the relevant information by the point we need to make the decision.

I agree. And, I think you're right about the fact that we've unfortunately lost the relevant info at the point. Even though it's a C++17 extension, someone could make value inline explicitly (as is done in your Godbolt) and we'd still error pre-c++17. Based on that, I'm going to close this for now. But if you (or someone else) thinks of another solution, or reason that it would be good to move forward with this, I'd be happy to re-open it and continue to work on it.

Thanks for all the help with this bug and patch.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::BuildVariableInstantiation(
   // Figure out whether to eagerly instantiate the initializer.
   if (InstantiatingVarTemplate || InstantiatingVarTemplatePartialSpec) {
     // We're producing a template. Don't instantiate the initializer yet.
   } else if (NewVar->getType()->isUndeducedType()) {
     // We need the type to complete the declaration of the variable.
     InstantiateVariableInitializer(NewVar, OldVar, TemplateArgs);
   } else if (InstantiatingSpecFromTemplate ||
              (OldVar->isInline() && OldVar->isThisDeclarationADefinition() &&
-              !NewVar->isThisDeclarationADefinition())) {
+              !NewVar->isThisDeclarationADefinition() &&
+              // On Microsoft's ABI, the new var's initializer will be part of
+              // the definition. But, we still want to instanciate it so that
+              // we can catch instantiation errors.
+              !Context.getTargetInfo().getCXXABI().isMicrosoft())) {
     // Delay instantiation of the initializer for variable template
     // specializations or inline static data members until a definition of the
     // variable is needed.
   } else {
     InstantiateVariableInitializer(NewVar, OldVar, TemplateArgs);
   }
 
   // Diagnose unused local variables with dependent types, where the diagnostic

clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++17 %s
+
+template <class T>
+struct GetTypeValue {
+  // expected-error@+1{{type 'int' cannot be used prior to '::' because it has no members}}
+  static constexpr const bool value = T::value;
+};
+
+using Invalid = GetTypeValue<int>;
+
+// expected-note@+1{{in instantiation of template class 'GetTypeValue<int>' requested here}}
+void test(Invalid) {}
+
+// expected-note@+2{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const S &' for 1st argument}}
+// expected-note@+1{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'S &&' for 1st argument}}
+struct S {};
+
+template <class T>
+struct MakeS {
+  // expected-error@+1{{no viable conversion from 'int' to 'const S'}}
+  static constexpr const S value = T();
+};
+
+using Invalid2 = MakeS<int>;
+
+// expected-note@+1{{in instantiation of template class 'MakeS<int>' requested here}}
+void test2(Invalid2) {}