[Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl

[arphaman]

Summary

This patch fixes an invalid Winitializer-overrides warning that's shown for a piece of code like this:

template <typename T> struct Foo {
  struct SubFoo {
    int bar1;
    int bar2;
  };

  static void Test() { 
   SubFoo sf = {.bar1 = 10, 
   // Incorrect "initializer overrides prior initialization" warning shown on the line below when instantiating Foo<bool>
                         .bar2 = 20}; 
  }
};

void foo() {
  Foo<int>::Test();
  Foo<bool>::Test();
}

The invalid warning is shown because in the second instantiation of Foo<T>::Test the DesignatedInitExpr that corresponds to the initializer for sf has a field designator with a pointer to the FieldDecl from the first instantiation of Foo<T>::SubFoo, which is incorrect in the context of the second instantiation. This means that InitListChecker::CheckDesignatedInitializer isn't able to find the correct FieldIndex for the correct field in the initialized record, leading to an incorrect warning.

This patch fixes this bug by ensuring that a DesignatedInitExpr is re-created by the tree-transformer when it has a field designator with a FieldDecl that has been already been set. This means that in the example above the DesignatedInitExpr won't be re-created by the tree-transformer in the first instantiation, but it will be re-created in the second instantiation, thus ensuring that the second instantiation doesn't have the incorrect FieldDecl pointers.

[manmanren]

It makes sense to rebuild the expression when a field designator stores a FieldDecl.

Thanks for working on this!
Manman

lib/Sema/TreeTransform.h

8926 ↗

(On Diff #75162)

Please add comment here on why we need to set ExprChanged to true.

[arphaman]

The updated patch adds a comment to the modified code as request by Manman.

[rjmccall]

The fact that this bug only arises when performing a *second* instantiation suggests that there's a deeper bug here, because template instantiation is not supposed to modify the pattern AST. In this case, the basic problem is that, when the parser processes a designator, it only has an identifier, not a FieldDecl*, because it doesn't know what type is being initialized yet. SemaInit eventually resolves that identifier to a FieldDecl and needs to record that in the AST; typically the AST is treated as immutable, but in this case, instead of cloning the expression, Sema just modifies the field designator in-place. That's not completely unreasonable, and it's definitely the most space-efficient solution for non-template code-building; but in template code, it does mean that we have to take care to not present the same unresolved field designator to Sema twice.

Fortunately, this is pretty easy: we just need to need to flag the expression as needing rebuilding when there isn't a resolved field in the field designator. When there *is* a resolved field, we just need to map it using TransformDecl; the expression then only needs to be rebuilt if that fails or returns a different declaration.

[arphaman]

In D25777#575564, @rjmccall wrote:

The fact that this bug only arises when performing a *second* instantiation suggests that there's a deeper bug here, because template instantiation is not supposed to modify the pattern AST. In this case, the basic problem is that, when the parser processes a designator, it only has an identifier, not a FieldDecl*, because it doesn't know what type is being initialized yet. SemaInit eventually resolves that identifier to a FieldDecl and needs to record that in the AST; typically the AST is treated as immutable, but in this case, instead of cloning the expression, Sema just modifies the field designator in-place. That's not completely unreasonable, and it's definitely the most space-efficient solution for non-template code-building; but in template code, it does mean that we have to take care to not present the same unresolved field designator to Sema twice.

Fortunately, this is pretty easy: we just need to need to flag the expression as needing rebuilding when there isn't a resolved field in the field designator. When there *is* a resolved field, we just need to map it using TransformDecl; the expression then only needs to be rebuilt if that fails or returns a different declaration.

You're right, the point that you made about modifying the pattern AST is correct. I will update the patch accordingly.

[arphaman]

The updated patch addresses John's comment by modifying the DesignatedInitExpr re-creation logic.

[rjmccall]

Thanks, LGTM with a small tweak.

lib/Sema/TreeTransform.h

8928 ↗

(On Diff #75414)

I'm pretty sure that TransformDecl promises to preserve the decl kind, i.e. this can be a cast_or_null.

cfe/trunk/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformDesignatedInitExpr(DesignatedInitExpr *E) {
   // transform the designators.
   SmallVector<Expr*, 4> ArrayExprs;
   bool ExprChanged = false;
   for (const DesignatedInitExpr::Designator &D : E->designators()) {
     if (D.isFieldDesignator()) {
       Desig.AddDesignator(Designator::getField(D.getFieldName(),
                                                D.getDotLoc(),
                                                D.getFieldLoc()));
+      if (D.getField()) {
+        FieldDecl *Field = cast_or_null<FieldDecl>(
+            getDerived().TransformDecl(D.getFieldLoc(), D.getField()));
+        if (Field != D.getField())
+          // Rebuild the expression when the transformed FieldDecl is
+          // different to the already assigned FieldDecl.
+          ExprChanged = true;
+      } else {
+        // Ensure that the designator expression is rebuilt when there isn't
+        // a resolved FieldDecl in the designator as we don't want to assign
+        // a FieldDecl to a pattern designator that will be instantiated again.
+        ExprChanged = true;
+      }
       continue;
     }
 
     if (D.isArrayDesignator()) {
       ExprResult Index = getDerived().TransformExpr(E->getArrayIndex(D));
       if (Index.isInvalid())
         return ExprError();
 

cfe/trunk/test/SemaCXX/designated-initializers.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template <typename T> struct Foo {
+  struct SubFoo {
+    int bar1;
+    int bar2;
+  };
+
+  static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no warning
+};
+
+void foo() {
+  Foo<int>::Test();
+  Foo<bool>::Test();
+  Foo<float>::Test();
+}
+
+template <typename T> struct Bar {
+  struct SubFoo {
+    int bar1;
+    int bar2;
+  };
+
+  static void Test() { SubFoo sf = {.bar1 = 10,    // expected-note 2 {{previous initialization is here}}
+                                    .bar1 = 20}; } // expected-warning 2 {{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+  Bar<int>::Test();  // expected-note {{in instantiation of member function 'Bar<int>::Test' requested here}}
+  Bar<bool>::Test(); // expected-note {{in instantiation of member function 'Bar<bool>::Test' requested here}}
+}