[AST][RecoveryAST] Preserve the type by default for recovery expression.

[hokein]

Summary

Not intend to land it now, but ready for review.

[hokein]

rebase

[hokein]

clang/test/CXX/temp/temp.decls/temp.variadic/fixed-expansion.cpp

129 ↗

(On Diff #276733)

the secondary diagnostic is technically correct, but I don't quite like it, it is confusing, ok to leave it as-is? or fix that before landing this patch?

[sammccall]

As you said, we can't land this before the branch cut, and we shouldn't land this until we've run internal experiments to show it's not horribly crashy.

clang/test/CXX/temp/temp.constr/temp.constr.order/function-templates.cpp

71

this is really nice :-)

clang/test/CXX/temp/temp.decls/temp.variadic/fixed-expansion.cpp

129 ↗

(On Diff #276733)

I don't think it's technically correct (the int fallback is an implementation artifact, albeit one that leaks quite often).
This example looks pretty obscure though, it'd be nice to fix it at some point but I don't think it's severe enough to block on the fix.

Fix ideas:

• the instantiation of f should be invalid, right? Maybe we avoid considering return types for invalid decls when computing recovery type (or always give up in this case).
• we could eventually try to replace the use of int for these cases
• we could special-case int, and don't allow RecoveryExprs to have type int, reflecting the fact that it's used for these cases

[hokein]

rebase and adjust more existing diagnostic tests.

[hokein]

As you said, we can't land this before the branch cut, and we shouldn't land this until we've run internal experiments to show it's not horribly crashy.

The internal experiment result is good, I think we're close to land it. After a new rebase, we need to adjust more diagnostics, but all of them look like good secondary-diagnostic improvements.

clang/test/CXX/temp/temp.decls/temp.variadic/fixed-expansion.cpp

129 ↗

(On Diff #276733)

Fixed in D85714.

[sammccall]

New diagnostics are really good :-)

clang/test/SemaCXX/abstract.cpp

282

the new diagnostics are correct (nice!) but this is just a bug in the test:
see https://bugs.llvm.org/show_bug.cgi?id=12658 vs https://github.com/llvm/llvm-project/commit/ea03214a5e3413cf95e388f28e4d9b9eeb30210a

To keep the test simple, I'd consider changing C& to const C& here instead.

clang/test/SemaCXX/type-convert-construct.cpp

9

again, arr v3 = arr(); is almost certainly intended

[hokein]

update

[hokein]

update a comment.

clang/test/SemaCXX/abstract.cpp

282

I'd prefer to keep these as these are secondary-diagnostic improvements (so that we will not regress them in the future).

I fixed these tests, and created two in recovery-ast-type.cpp.

[vvereschaka]

Hi @hokein,

sorry, but looks like your changes break one of libc++ tests on ARM cross toolchain build on Windows: libc++::function_type_default_deleter.fail.cpp
Here is the first failed build: http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/945

I didn't get how these changes affects the failing test, but reverting of your commit fixes the problem.

Would you take a look and fix the problem? or revert the changes if it is going to take a lot of time to resolve?

Here is the command line to build the test:

C:/buildbot/temp/build/./bin/clang++.exe C:/buildbot/temp/llvm-project/libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp -v --sysroot=C:/buildbot/.arm-ubuntu --target=armv7-linux-gnueabihf -include C:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\__config_site -include C:/buildbot/temp/llvm-project/libcxx\test\support\nasty_macros.h -nostdinc++ -IC:/buildbot/temp/llvm-project/libcxx\include -IC:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\include\c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -IC:/buildbot/temp/llvm-project/libcxx\test/support -Werror -Wall -Wextra -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -std=c++2a -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0

Here is the result output:

clang -cc1 version 12.0.0 based upon LLVM 12.0.0git default target armv7-linux-gnueabihf
ignoring nonexistent directory "C:/buildbot/.arm-ubuntu/usr/local/include"
ignoring nonexistent directory "C:/buildbot/.arm-ubuntu/include"
#include "..." search starts here:
#include <...> search starts here:
 C:/buildbot/temp/llvm-project/libcxx\include
 C:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\include\c++build
 C:/buildbot/temp/llvm-project/libcxx\test/support
 C:\buildbot\temp\build\lib\clang\12.0.0\include
 C:/buildbot/.arm-ubuntu/usr/include/arm-linux-gnueabihf
 C:/buildbot/.arm-ubuntu/usr/include
End of search list.
error: 'error' diagnostics seen but not expected:
  File C:/buildbot/temp/llvm-project/libcxx\include\memory Line 2033: no member named 'value' in 'std::__1::is_empty<std::__1::__compressed_pair<void
(*)(Tag<5>), std::__1::default_delete<void (Tag<5>)>>>'
1 error generated.

[hokein]

In D82657#2240603, @vvereschaka wrote:

Hi @hokein,

sorry, but looks like your changes break one of libc++ tests on ARM cross toolchain build on Windows: libc++::function_type_default_deleter.fail.cpp
Here is the first failed build: http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/945

I didn't get how these changes affects the failing test, but reverting of your commit fixes the problem.

Would you take a look and fix the problem? or revert the changes if it is going to take a lot of time to resolve?

Here is the command line to build the test:

C:/buildbot/temp/build/./bin/clang++.exe C:/buildbot/temp/llvm-project/libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp -v --sysroot=C:/buildbot/.arm-ubuntu --target=armv7-linux-gnueabihf -include C:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\__config_site -include C:/buildbot/temp/llvm-project/libcxx\test\support\nasty_macros.h -nostdinc++ -IC:/buildbot/temp/llvm-project/libcxx\include -IC:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\include\c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -IC:/buildbot/temp/llvm-project/libcxx\test/support -Werror -Wall -Wextra -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -std=c++2a -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0

Here is the result output:

clang -cc1 version 12.0.0 based upon LLVM 12.0.0git default target armv7-linux-gnueabihf
ignoring nonexistent directory "C:/buildbot/.arm-ubuntu/usr/local/include"
ignoring nonexistent directory "C:/buildbot/.arm-ubuntu/include"
#include "..." search starts here:
#include <...> search starts here:
 C:/buildbot/temp/llvm-project/libcxx\include
 C:/buildbot/temp/build/runtimes/runtimes-bins/libcxx\include\c++build
 C:/buildbot/temp/llvm-project/libcxx\test/support
 C:\buildbot\temp\build\lib\clang\12.0.0\include
 C:/buildbot/.arm-ubuntu/usr/include/arm-linux-gnueabihf
 C:/buildbot/.arm-ubuntu/usr/include
End of search list.
error: 'error' diagnostics seen but not expected:
  File C:/buildbot/temp/llvm-project/libcxx\include\memory Line 2033: no member named 'value' in 'std::__1::is_empty<std::__1::__compressed_pair<void
(*)(Tag<5>), std::__1::default_delete<void (Tag<5>)>>>'
1 error generated.

thanks! I have a proposal fix in D86685.

[guopeilin]

Hi @hokein , I encounter a bug when clang parses enum and I have been recorded in https://bugs.llvm.org/show_bug.cgi?id=51554.
The source code like the following:

enum E { e = E() };
int main() {
  return 0;
}

Some error message are expected like the following:

test.cpp:1:14: error: invalid use of incomplete type 'E'
enum E { e = E() };
             ^~~
test.cpp:1:6: note: definition of 'E' is not complete until the closing '}'
enum E { e = E() };

Also, I have made some analyses like the following:

In ParseDecl.cpp:
1. llvm-10: 
   AssignedVal.get(): NULL
   
2. llvm-12(Context.getLangOpts().RecoveryASTType: default false)(early-version):
   AssignedVal.get(): RecoveryExpr 0xaaaaba19df20 '<dependent type>' contains-errors lvalue
   
3. llvm-12(Context.getLangOpts().RecoveryASTType: default true) (latest-version, Finally crashed):
   AssignedVal.get()->dump(): RecoveryExpr 0xaaaabb4d9500 'enum E' contains-errors
   
In CheckEnumConstant(), EltTy(type: QualType, value: EnumType 0xaaaabb517dd0 'enum E') cannot use getIntWidth(), so crashed. But the code doesn't seem to be wrong around here.

Could you please have a look at this issue? Thanks a lot.

[sammccall]

In D82657#2956751, @guopeilin wrote:

Hi @hokein , I encounter a bug when clang parses enum and I have been recorded in https://bugs.llvm.org/show_bug.cgi?id=51554.

I added a comment on the bug and sent https://reviews.llvm.org/D108451

This is a "rough edge" between erroneous and well-formed code and I'm not totally sure it's the perfect fix, but it does seem to be robust against crashes which is definitely he biggest thing.

[guopeilin]

In D82657#2956851, @sammccall wrote:

In D82657#2956751, @guopeilin wrote:

Hi @hokein , I encounter a bug when clang parses enum and I have been recorded in https://bugs.llvm.org/show_bug.cgi?id=51554.

I added a comment on the bug and sent https://reviews.llvm.org/D108451

This is a "rough edge" between erroneous and well-formed code and I'm not totally sure it's the perfect fix, but it does seem to be robust against crashes which is definitely he biggest thing.

Yes, that's great, thanks a lot.

clang/include/clang/Driver/Options.td

def fno_concept_satisfaction_caching : Flag<["-"],
                                             "fno-concept-satisfaction-caching">,
   HelpText<"Disable satisfaction caching for C++2a Concepts.">;
 
 def frecovery_ast : Flag<["-"], "frecovery-ast">,
   HelpText<"Preserve expressions in AST rather than dropping them when "
            "encountering semantic errors">;
 def fno_recovery_ast : Flag<["-"], "fno-recovery-ast">;
 def frecovery_ast_type : Flag<["-"], "frecovery-ast-type">,
-  HelpText<"Preserve the type for recovery expressions when possible "
-          "(experimental)">;
+  HelpText<"Preserve the type for recovery expressions when possible">;
 def fno_recovery_ast_type : Flag<["-"], "fno-recovery-ast-type">;
 
 let Group = Action_Group in {
 
 def Eonly : Flag<["-"], "Eonly">,
   HelpText<"Just run preprocessor, no output (for timings)">;
 def dump_raw_tokens : Flag<["-"], "dump-raw-tokens">,
   HelpText<"Lex file in raw mode and dump raw tokens">;

clang/lib/Frontend/CompilerInvocation.cpp

#include "clang/Basic/LangStandards.def"
   }
   Opts.ConceptSatisfactionCaching =
       !Args.hasArg(OPT_fno_concept_satisfaction_caching);
   if (Args.hasArg(OPT_fconcepts_ts))
     Diags.Report(diag::warn_fe_concepts_ts_flag);
   // Recovery AST still heavily relies on dependent-type machinery.
   Opts.RecoveryAST =
       Args.hasFlag(OPT_frecovery_ast, OPT_fno_recovery_ast, Opts.CPlusPlus);
-  Opts.RecoveryASTType =
-      Args.hasFlag(OPT_frecovery_ast_type, OPT_fno_recovery_ast_type, false);
+  Opts.RecoveryASTType = Args.hasFlag(
+      OPT_frecovery_ast_type, OPT_fno_recovery_ast_type, Opts.CPlusPlus);
   Opts.HeinousExtensions = Args.hasArg(OPT_fheinous_gnu_extensions);
   Opts.AccessControl = !Args.hasArg(OPT_fno_access_control);
   Opts.ElideConstructors = !Args.hasArg(OPT_fno_elide_constructors);
   Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);
   Opts.InstantiationDepth =
       getLastArgIntValue(Args, OPT_ftemplate_depth, 1024, Diags);
   Opts.ArrowDepth =
       getLastArgIntValue(Args, OPT_foperator_arrow_depth, 256, Diags);

clang/test/CXX/temp/temp.constr/temp.constr.order/function-templates.cpp

 void f() { }
 // expected-note@-1 {{candidate function [with T = long long, U = int]}}
 
 template<class T, class U> requires BiggerThanInt<T>
 void f() { }
 // expected-note@-1 {{candidate function [with T = long long, U = int]}}
 
 static_assert(sizeof(f<long long, int>()));
-// expected-error@-1 {{call to 'f' is ambiguous}}
+// expected-error@-1 {{call to 'f' is ambiguous}} \
+   expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
 
 template<typename T>
 concept C3 = true;
 
 template<typename T>
 concept C4 = true && C3<T>;
 
 template<typename T> requires C3<void>

clang/test/SemaCXX/abstract.cpp

 
 namespace pr12658 {
   class C {
     public:
       C(int v){}
       virtual void f() = 0; // expected-note {{unimplemented pure virtual method 'f' in 'C'}}
   };
 
-  void foo( C& c ) {}
+  void foo(const C& c ) {}
 
   void bar( void ) {
     foo(C(99)); // expected-error {{allocating an object of abstract class type 'pr12658::C'}}
   }
 }
 
 namespace pr16659 {
   struct A {

clang/test/SemaCXX/decl-expr-ambiguity.cpp

 // RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors %s
 // RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors -std=gnu++98 %s
 // RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors -std=gnu++11 %s
 // RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors -x objective-c++ %s
 
 void f() {
   int a;
   struct S { int m; };
   typedef S *T;
 
   // Expressions.
   T(a)->m = 7;
   int(a)++; // expected-error {{assignment to cast is illegal}}
   __extension__ int(a)++; // expected-error {{assignment to cast is illegal}}
-  __typeof(int)(a,5)<<a; // expected-error {{excess elements in scalar initializer}}
+  __typeof(int)(a,5)<<a; // expected-error {{excess elements in scalar initializer}} expected-warning {{expression result unused}}
   void(a), ++a;
   if (int(a)+1) {}
   for (int(a)+1;;) {} // expected-warning {{expression result unused}}
   a = sizeof(int()+1);
   a = sizeof(int(1));
   typeof(int()+1) a2; // expected-error {{extension used}}
   (int(1)); // expected-warning {{expression result unused}}
 

clang/test/SemaCXX/recovery-expr-type.cpp

struct T {
   T() = delete; // expected-note {{has been explicitly marked deleted here}}
 };
 
 void func() {
   // verify that no -Wunused-value diagnostic.
   (T(T())); // expected-error {{call to deleted constructor}}
 }
 }
+
+// verify the secondary diagnostic "no matching function" is emitted.
+namespace test7 {
+struct C {
+  C() = delete; // expected-note {{has been explicitly marked deleted}}
+};
+void f(C &); // expected-note {{candidate function not viable: expects an l-value for 1st argument}}
+void test() {
+  f(C()); // expected-error {{call to deleted constructor}} \
+             expected-error {{no matching function for call}}
+}
+}
+
+// verify the secondary diagnostic "cannot initialize" is emitted.
+namespace test8 {
+typedef int arr[];
+int v = arr(); // expected-error {{array types cannot be value-initialized}} \
+                  expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'test8::arr'}}
+}

clang/test/SemaCXX/type-convert-construct.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s 
 // RUN: %clang_cc1 -fsyntax-only -verify -std=gnu++98 %s 
 // RUN: %clang_cc1 -fsyntax-only -verify -std=gnu++11 %s 
 
 void f() {
   float v1 = float(1);
   int v2 = typeof(int)(1,2); // expected-error {{excess elements in scalar initializer}}
   typedef int arr[];
-  int v3 = arr(); // expected-error {{array types cannot be value-initialized}}
+  arr(); // expected-error {{array types cannot be value-initialized}}
   typedef void fn_ty();
   fn_ty(); // expected-error {{cannot create object of function type 'fn_ty'}}
   fn_ty(0); // expected-error {{functional-style cast from 'int' to 'fn_ty'}}
   fn_ty(0, 0); // expected-error {{cannot create object of function type 'fn_ty'}}
 #if __cplusplus >= 201103L
   fn_ty{}; // expected-error {{cannot create object of function type 'fn_ty'}}
   fn_ty{0}; // expected-error {{cannot create object of function type 'fn_ty'}}
   fn_ty{0, 0}; // expected-error {{cannot create object of function type 'fn_ty'}}

clang/test/SemaTemplate/dependent-names.cpp

namespace PR10053 {
   }
 
   void f(char&);
 
   int k = N::M::g<char>(0);; // expected-note {{in instantiation of function}}
 
 
   namespace O {
-    void f(char&); // expected-note {{candidate function not viable}}
+    int f(char&); // expected-note {{candidate function not viable}}
 
     template<typename T> struct C {
       static const int n = f(T()); // expected-error {{no matching function}}
     };
   }
 
   int f(double); // no note, shadowed by O::f
   O::C<double> c; // expected-note {{requested here}}