blink_gc_plugin: Disallow only Optional GCed fields

GCed optionals on stack are fine.

Change-Id: If5a6fd603f06c376167a93b790df4970289a2663
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2566937Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Auto-Submit: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833302}

tools/clang/blink_gc_plugin/BadPatternFinder.cpp

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "BadPatternFinder.h"
+#include <clang/AST/Decl.h>
 #include "DiagnosticsReporter.h"
 
 #include <algorithm>
@@ -60,24 +61,30 @@ class OptionalGarbageCollectedMatcher : public MatchFinder::MatchCallback {
       : diagnostics_(diagnostics) {}
 
   void Register(MatchFinder& match_finder) {
-    // Matches any application of make_unique where the template argument is
-    // known to refer to a garbage-collected type.
-    auto optional_construction =
-        cxxConstructExpr(hasDeclaration(cxxConstructorDecl(ofClass(
-                             classTemplateSpecializationDecl(
-                                 hasName("::base::Optional"),
-                                 hasTemplateArgument(
-                                     0, refersToType(GarbageCollectedType())))
-                                 .bind("optional")))))
-            .bind("bad");
-    match_finder.addDynamicMatcher(optional_construction, this);
+    // Matches fields and new-expressions of type base::Optional where the
+    // template argument is known to refer to a garbage-collected type.
+    auto optional_type = hasType(
+        classTemplateSpecializationDecl(
+            hasName("::base::Optional"),
+            hasTemplateArgument(0, refersToType(GarbageCollectedType())))
+            .bind("optional"));
+    auto optional_field = fieldDecl(optional_type).bind("bad_field");
+    auto optional_new_expression =
+        cxxNewExpr(has(cxxConstructExpr(optional_type))).bind("bad_new");
+    match_finder.addDynamicMatcher(optional_field, this);
+    match_finder.addDynamicMatcher(optional_new_expression, this);
   }
 
   void run(const MatchFinder::MatchResult& result) override {
-    auto* bad_use = result.Nodes.getNodeAs<clang::Expr>("bad");
     auto* optional = result.Nodes.getNodeAs<clang::CXXRecordDecl>("optional");
     auto* gc_type = result.Nodes.getNodeAs<clang::CXXRecordDecl>("gctype");
-    diagnostics_.OptionalUsedWithGC(bad_use, optional, gc_type);
+    if (auto* bad_field =
+            result.Nodes.getNodeAs<clang::FieldDecl>("bad_field")) {
+      diagnostics_.OptionalFieldUsedWithGC(bad_field, optional, gc_type);
+    } else {
+      auto* bad_new = result.Nodes.getNodeAs<clang::Expr>("bad_new");
+      diagnostics_.OptionalNewExprUsedWithGC(bad_new, optional, gc_type);
+    }
   }
 
  private:

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -141,9 +141,15 @@ const char kUniquePtrUsedWithGC[] =
     "[blink-gc] Disallowed use of %0 found; %1 is a garbage-collected type. "
     "std::unique_ptr cannot hold garbage-collected objects.";
 
-const char kOptionalUsedWithGC[] =
-    "[blink-gc] Disallowed construction of %0 found; %1 is a garbage-collected "
-    "type. optional cannot hold garbage-collected objects.";
+const char kOptionalFieldUsedWithGC[] =
+    "[blink-gc] Disallowed optional field of %0 found; %1 is a "
+    "garbage-collected "
+    "type. Optional fields cannot hold garbage-collected objects.";
+
+const char kOptionalNewExprUsedWithGC[] =
+    "[blink-gc] Disallowed new-expression of %0 found; %1 is a "
+    "garbage-collected "
+    "type. GCed types cannot be created with new.";
 
 const char kVariantUsedWithGC[] =
     "[blink-gc] Disallowed construction of %0 found; %1 is a garbage-collected "
@@ -248,8 +254,10 @@ DiagnosticsReporter::DiagnosticsReporter(
 
   diag_unique_ptr_used_with_gc_ =
       diagnostic_.getCustomDiagID(getErrorLevel(), kUniquePtrUsedWithGC);
-  diag_optional_used_with_gc_ =
-      diagnostic_.getCustomDiagID(getErrorLevel(), kOptionalUsedWithGC);
+  diag_optional_field_used_with_gc_ =
+      diagnostic_.getCustomDiagID(getErrorLevel(), kOptionalFieldUsedWithGC);
+  diag_optional_new_expr_used_with_gc_ =
+      diagnostic_.getCustomDiagID(getErrorLevel(), kOptionalNewExprUsedWithGC);
   diag_variant_used_with_gc_ =
       diagnostic_.getCustomDiagID(getErrorLevel(), kVariantUsedWithGC);
 }
@@ -540,11 +548,19 @@ void DiagnosticsReporter::UniquePtrUsedWithGC(
       << bad_function << gc_type << expr->getSourceRange();
 }
 
-void DiagnosticsReporter::OptionalUsedWithGC(
+void DiagnosticsReporter::OptionalFieldUsedWithGC(
+    const clang::FieldDecl* field,
+    const clang::CXXRecordDecl* optional,
+    const clang::CXXRecordDecl* gc_type) {
+  ReportDiagnostic(field->getBeginLoc(), diag_optional_field_used_with_gc_)
+      << optional << gc_type << field->getSourceRange();
+}
+
+void DiagnosticsReporter::OptionalNewExprUsedWithGC(
     const clang::Expr* expr,
     const clang::CXXRecordDecl* optional,
     const clang::CXXRecordDecl* gc_type) {
-  ReportDiagnostic(expr->getBeginLoc(), diag_optional_used_with_gc_)
+  ReportDiagnostic(expr->getBeginLoc(), diag_optional_new_expr_used_with_gc_)
       << optional << gc_type << expr->getSourceRange();
 }
 

tools/clang/blink_gc_plugin/DiagnosticsReporter.h

@@ -79,9 +79,12 @@ class DiagnosticsReporter {
   void UniquePtrUsedWithGC(const clang::Expr* expr,
                            const clang::FunctionDecl* bad_function,
                            const clang::CXXRecordDecl* gc_type);
-  void OptionalUsedWithGC(const clang::Expr* expr,
-                          const clang::CXXRecordDecl* optional,
-                          const clang::CXXRecordDecl* gc_type);
+  void OptionalFieldUsedWithGC(const clang::FieldDecl* decl,
+                               const clang::CXXRecordDecl* optional,
+                               const clang::CXXRecordDecl* gc_type);
+  void OptionalNewExprUsedWithGC(const clang::Expr* expr,
+                                 const clang::CXXRecordDecl* optional,
+                                 const clang::CXXRecordDecl* gc_type);
   void VariantUsedWithGC(const clang::Expr* expr,
                          const clang::CXXRecordDecl* variant,
                          const clang::CXXRecordDecl* gc_type);
@@ -142,7 +145,8 @@ class DiagnosticsReporter {
   unsigned diag_member_in_stack_allocated_class_;
 
   unsigned diag_unique_ptr_used_with_gc_;
-  unsigned diag_optional_used_with_gc_;
+  unsigned diag_optional_field_used_with_gc_;
+  unsigned diag_optional_new_expr_used_with_gc_;
   unsigned diag_variant_used_with_gc_;
 };
 

tools/clang/blink_gc_plugin/tests/optional_gc_object.cpp

@@ -6,14 +6,23 @@
 
 namespace blink {
 
+class WithOpt : public GarbageCollected<WithOpt> {
+ public:
+  virtual void Trace(Visitor*) const {}
+
+ private:
+  base::Optional<Base> optional_field_;  // Optional fields are disallowed.
+};
+
 void DisallowedUseOfUniquePtr() {
-  base::Optional<Base> optional_base;
+  base::Optional<Base> optional_base;  // Must be okay.
   (void)optional_base;
 
-  base::Optional<Derived> optional_derived;
+  base::Optional<Derived> optional_derived;  // Must also be okay.
   (void)optional_derived;
 
-  new base::Optional<Base>;
+  new base::Optional<Base>;  // New expression with gced optionals are not
+                             // allowed.
 }
 
 }  // namespace blink

tools/clang/blink_gc_plugin/tests/optional_gc_object.txt

-optional_gc_object.cpp:10:24: warning: [blink-gc] Disallowed construction of 'Optional<blink::Base>' found; 'Base' is a garbage-collected type. optional cannot hold garbage-collected objects.
-  base::Optional<Base> optional_base;
-                       ^~~~~~~~~~~~~
-optional_gc_object.cpp:13:27: warning: [blink-gc] Disallowed construction of 'Optional<blink::Derived>' found; 'Derived' is a garbage-collected type. optional cannot hold garbage-collected objects.
-  base::Optional<Derived> optional_derived;
-                          ^~~~~~~~~~~~~~~~
-optional_gc_object.cpp:16:7: warning: [blink-gc] Disallowed construction of 'Optional<blink::Base>' found; 'Base' is a garbage-collected type. optional cannot hold garbage-collected objects.
-  new base::Optional<Base>;
-      ^~~~
-3 warnings generated.
+optional_gc_object.cpp:14:3: warning: [blink-gc] Disallowed optional field of 'Optional<blink::Base>' found; 'Base' is a garbage-collected type. Optional fields cannot hold garbage-collected objects.
+  base::Optional<Base> optional_field_;  // Optional fields are disallowed.
+  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+optional_gc_object.cpp:24:3: warning: [blink-gc] Disallowed new-expression of 'Optional<blink::Base>' found; 'Base' is a garbage-collected type. GCed types cannot be created with new.
+  new base::Optional<Base>;  // New expression with gced optionals are not
+  ^~~~~~~~~~~~~~~~~~~~~~~~
+2 warnings generated.