[clang] Tweaked fixit for static assert with no message

[njames93]

Summary

If a static assert has a message as the right side of an and condition, suggest a fix it of replacing the '&&' to ','.

static_assert(cond && "Failed Cond") -> static_assert(cond, "Failed cond")

This use case comes up when lazily replacing asserts with static asserts.

[njames93]

I don't see any support for verifying fix-its in the test cases so unsure how i could go about testing this change.

clang/lib/Parse/ParseDeclCXX.cpp

19

I'll undo this, clangd include inserter and clang format making undesired changes

[njames93]

Remove unnecessary changes

[njames93]

Just noticed this doesn't quite work as expected, will update

[njames93]

FixIt now properly generated

[njames93]

Ping

[njames93]

Ping

[njames93]

Ping?

[aaron.ballman]

In D89065#2320031, @njames93 wrote:

I don't see any support for verifying fix-its in the test cases so unsure how i could go about testing this change.

We usually test fix-its with -fdiagnostics-parseable-fixits as in the tests here: https://github.com/llvm/llvm-project/tree/main/clang/test/FixIt (not saying the test has to go into the FixIt directory, it should probably live in test/Parser given the changes.)

Btw, this is going to cause merge conflicts with https://reviews.llvm.org/D95396 and I sort of wonder if we should combine the two patches into one given that they're fixing the same code in different ways.

[xbolva00]

clang/lib/Parse/ParseDeclCXX.cpp

872

Do we warn for:

static_assert(“my msg”)?

I remember some long time ago bug in llvm itself - assert(“my msg”) - condition expr was missing.

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

872

We don't, but that would be a reasonable warning to add (not necessarily as part of this patch).

[njames93]

clang/lib/Parse/ParseDeclCXX.cpp

872

There is a clang-tidy check, misc-static-assert, that will flag asserts like that, although it won't flag static asserts of that kind.

[njames93]

Add tests for fixits.

[njames93]

Add recompile test to ensure fix-its are correct

[aaron.ballman]

LGTM aside from some nits.

clang/lib/Parse/ParseDeclCXX.cpp

861

863

clang/test/FixIt/fixit-static-assert.cpp

3–4

Any reason not to pass -verify=cxx17 and // cxx17-no-diagnostics?

[njames93]

clang/test/FixIt/fixit-static-assert.cpp

3–4

I don't know my way around clang testing framework that well.
For the recompile test I think Werror has to be used as verify resulted in an assertion.

[njames93]

Address comments

[aaron.ballman]

clang/test/FixIt/fixit-static-assert.cpp

3–4

Ah, okay, thanks for the explanation.

clang/lib/Parse/ParseDeclCXX.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Parse/Parser.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/Attributes.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/TimeProfiler.h"
Decl *Parser::ParseAliasDeclarationAfterDeclarator(
   MultiTemplateParamsArg TemplateParamsArg(
     TemplateParams ? TemplateParams->data() : nullptr,
     TemplateParams ? TemplateParams->size() : 0);
   return Actions.ActOnAliasDeclaration(getCurScope(), AS, TemplateParamsArg,
                                        UsingLoc, D.Name, Attrs, TypeAlias,
                                        DeclFromDeclSpec);
 }
 
+static FixItHint getStaticAssertNoMessageFixIt(const Expr *AssertExpr,
+                                               SourceLocation EndExprLoc) {
+  if (const auto *BO = dyn_cast_or_null<BinaryOperator>(AssertExpr)) {
+    if (BO->getOpcode() == BO_LAnd &&
+        isa<StringLiteral>(BO->getRHS()->IgnoreImpCasts()))
+      return FixItHint::CreateReplacement(BO->getOperatorLoc(), ",");
+  }
+  return FixItHint::CreateInsertion(EndExprLoc, ", \"\"");
+}
+
 /// ParseStaticAssertDeclaration - Parse C++0x or C11 static_assert-declaration.
 ///
 /// [C++0x] static_assert-declaration:
 ///           static_assert ( constant-expression  ,  string-literal  ) ;
 ///
 /// [C11]   static_assert-declaration:
 ///           _Static_assert ( constant-expression  ,  string-literal  ) ;
 ///
 Decl *Parser::ParseStaticAssertDeclaration(SourceLocation &DeclEnd){
   assert(Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert) &&
          "Not a static_assert declaration");
 
Decl *Parser::ParseStaticAssertDeclaration(SourceLocation &DeclEnd){
   ExprResult AssertExpr(ParseConstantExpressionInExprEvalContext());
   if (AssertExpr.isInvalid()) {
     SkipMalformedDecl();
     return nullptr;
   }
 
   ExprResult AssertMessage;
   if (Tok.is(tok::r_paren)) {
-    Diag(Tok, getLangOpts().CPlusPlus17
-                  ? diag::warn_cxx14_compat_static_assert_no_message
-                  : diag::ext_static_assert_no_message)
-      << (getLangOpts().CPlusPlus17
-              ? FixItHint()
-              : FixItHint::CreateInsertion(Tok.getLocation(), ", \"\""));
+    if (getLangOpts().CPlusPlus17)
+      Diag(Tok, diag::warn_cxx14_compat_static_assert_no_message);
+    else
+      Diag(Tok, diag::ext_static_assert_no_message)
+          << getStaticAssertNoMessageFixIt(AssertExpr.get(), Tok.getLocation());
   } else {
     if (ExpectAndConsume(tok::comma)) {
       SkipUntil(tok::semi);
       return nullptr;
     }
 
     if (!isTokenStringLiteral()) {
       Diag(Tok, diag::err_expected_string_literal)

clang/test/FixIt/fixit-static-assert.cpp

+// RUN: %clang_cc1 -std=c++14 %s -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// Ensure no warnings are emitted in c++17.
+// RUN: %clang_cc1 -std=c++17 %s -verify=cxx17
+// RUN: %clang_cc1 -std=c++14 %s -fixit-recompile -fixit-to-temporary -Werror
+
+// cxx17-no-diagnostics
+
+static_assert(true && "String");
+// CHECK-DAG: {[[@LINE-1]]:20-[[@LINE-1]]:22}:","
+
+// String literal prefixes are good.
+static_assert(true && R"(RawString)");
+// CHECK-DAG: {[[@LINE-1]]:20-[[@LINE-1]]:22}:","
+static_assert(true && L"RawString");
+// CHECK-DAG: {[[@LINE-1]]:20-[[@LINE-1]]:22}:","
+
+static_assert(true);
+// CHECK-DAG: {[[@LINE-1]]:19-[[@LINE-1]]:19}:", \"\""
+
+// While its technically possible to transform this to
+// static_assert(true, "String") we don't attempt this fix.
+static_assert("String" && true);
+// CHECK-DAG: {[[@LINE-1]]:31-[[@LINE-1]]:31}:", \"\""
+
+// Don't be smart and look in parentheses.
+static_assert((true && "String"));
+// CHECK-DAG: {[[@LINE-1]]:33-[[@LINE-1]]:33}:", \"\""