[PATCH] D149516: [Sema] `setInvalidDecl` for error deduction declaration

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I wonder if there are any cases where the "deduction guide" is so broken that we shouldn't be creating a `CXXDeductionGuideDecl` at all. For example, if there's no trailing return type, then the declaration is syntactically invalid (and looks more like a constructor than a deduction guide), and it's not clear to me whether creating a `CXXDeductionGuideDecl` really makes sense.

In D149516#4308203 <https://reviews.llvm.org/D149516#4308203>, @shafik wrote:

> I have some questions, why don't we always call `CheckDeductionGuideDeclarator` when calling `CXXDeductionGuideDecl::Create`, I felt like perhaps `CheckDeductionGuideDeclarator` should be rolled into `CXXDeductionGuideDecl::Create` but then I noticed they are not paired up in other locations.

`CXXDeductionGuideDecl::Create` is the low-level AST primitive for creating a `CXXDeductionGuideDecl`. The responsibility for validating the declaration and issuing diagnostics doesn't belong at that level, it belongs in `Sema`. The other callers are:

- When `Sema` creates an implicit deduction guide from a constructor. In this case `Sema` knows it's valid by construction.
- When `Sema` instantiates a deduction guide. In this case `Sema` knows it's valid because the template was.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:9202
           << TrailingRequiresClause->getSourceRange();
-    SemaRef.CheckDeductionGuideDeclarator(D, R, SC);
+    bool IsValid = SemaRef.CheckDeductionGuideDeclarator(D, R, SC);
 
----------------
This is reversed from the normal Clang convention (where `true` indicates that an error was produced).


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9204
 
-    return CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(),
+    CXXDeductionGuideDecl* DGD = CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(),
                                          ExplicitSpecifier, NameInfo, R, TInfo,
----------------
Use `auto` here.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11141-11145
     if (!Chunk.Fun.hasTrailingReturnType()) {
+      IsValid = false;
       Diag(D.getName().getBeginLoc(),
            diag::err_deduction_guide_no_trailing_return_type);
       break;
----------------
Can we just return immediately here? I don't see a reason to keep going -- this error may well indicate that the developer didn't actually mean to declare a deduction guide in the first place, so the diagnostic on a function body below is likely to not be useful.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11196
   if (D.isFunctionDefinition())
     Diag(D.getIdentifierLoc(), diag::err_deduction_guide_defines_function);
+  return IsValid;
----------------
Should we return that we had an error after this one too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149516/new/

https://reviews.llvm.org/D149516