Fixit for 'typedef' instead of 'typename' typo

[vsapsai]

Parse/ParseTemplate.cpp

492 ↗

(On Diff #130152)

How does it work when you have typedef for the first template parameter?

Parser/typedef-instead-of-typename-typo.hpp

3 ↗

(On Diff #130152)

Maybe put this test in clang/test/FixIt ?

Also please check what file extensions are used for testing templates. .hpp reflects real-life usage but most tests are .cpp. Or maybe I wasn't paying attention.

5 ↗

(On Diff #130152)

It is a little bit confusing to what lines the messages would be attributed to. Need to check locally because not sure I interpret all those backslashes the same way lit does.

Also idea for the test. To check that the fix-it was applied properly you can add a member like B b; and it shouldn't trigger any errors.

clang/Basic/DiagnosticParseKinds.td

1167 ↗

(On Diff #130152)

Looks like other diagnostic messages "did you mean to use …" have lowercase "d" in "did". Though I haven't checked how it looks in various situations.

[jkorous-apple]

Parse/ParseTemplate.cpp

492 ↗

(On Diff #130152)

It works all right but I am puzzled by your question now :-)

/Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:3:11: error: expected template parameter
template <typedef A, typename B> struct Foo {
          ^
/Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:3:11: note: Did you mean to use 'typename'?
template <typedef A, typename B> struct Foo {
          ^~~~~~~
          typename
fix-it:"/Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp":{3:11-3:18}:"typename"
/Users/jankorous/src/oss/llvm/llvm/tools/clang/test/Parser/typedef-instead-of-typename-typo.hpp:4:3: error: unknown type name 'a'
  a
  ^

I am probably not following you here. Do you have any specific reason on your mind? Anything I should think about or check in the source code?

Parser/typedef-instead-of-typename-typo.hpp

3 ↗

(On Diff #130152)

Oh, I completely missed that directory! Thanks.

It seems like there are some header files with ".h" extension but these are usually somehow special (empty file or something) so I figure you are right and the extension should be ".cpp".

5 ↗

(On Diff #130152)

I am totally open to suggestions how to write better tests using FileCheck. As far as I understand it I have to keep the expected-* macro on the same line as the code. Or is there any better way?

Do I understand it right that you suggest to create another test in which to try applying fixit and check that compilation was successful? Do you mean to create compile_commands.json temporarily and use clang-check -fixit?

clang/Basic/DiagnosticParseKinds.td

1167 ↗

(On Diff #130152)

You are right. Thanks.

[jkorous-apple]

Changes based on Volodymyr's comments.

[vsapsai]

Parse/ParseTemplate.cpp

492 ↗

(On Diff #130152)

I was suspicious that

if (isStartOfTemplateTypeParameter())
  return ParseTypeParameter(Depth, Position);

earlier in the function won't let your code run for the first template parameter. But isStartOfTemplateTypeParameter has

if (Tok.isNot(tok::kw_typename))
  return false;

so everything works fine.

Parser/typedef-instead-of-typename-typo.hpp

5 ↗

(On Diff #130152)

To answer your question. You can keep the expected-* macro on the same line and attach subsequent expectations with backslash. Another option is doing something like expected-note@-1 {{...}}. That -1 is a relative line number.

Back to the test case. I believe you need to separate expected-* checks from fixit output check. The former uses -verify and the latter | FileCheck %s. I think test/FixIt/fixit-vexing-parse.cpp is a good example. And remember that you can split long lines, template <typename A, typedef B> struct Foo doesn't have to be on the same line.

For suggested test I didn't mean anything fancy. Just something like adding a line B b; Because currently for

template <typename A, typedef B> 
struct Foo {
    B b;
};

the output is

recovery.cc:1:31: error: unknown type name 'B'
template <typename A, typedef B>
                              ^
recovery.cc:3:5: error: unknown type name 'B'
    B b;
    ^
2 errors generated.

And with your change we won't have the second error.

[jkorous-apple]

I got rid of backslashes in test and added member of the type parameter with typo. Thanks for these suggestions.

[jkorous-apple]

I got rid of backslashes in test and added member of the type parameter with typo. Thanks for suggestions.

[vsapsai]

FixIt/fixit-typedef-instead-of-typename-typo.cpp

14 ↗

(On Diff #131693)

You don't need // CHECK: prefix for expected-error. And without FileCheck CHECK lines aren't verified. In fact, the test should fail as it expects the fix-it to be for the line 4, not line 3. I'm not sure you can test diagnostic and parseable fixits with a single RUN line, luckily we can have multiple RUN lines.

I don't think those relative error lines are good from readability standpoint. But please keep in mind that all my comments about readability are subjective and should be taken with the grain of salt. The problem is that one needs to count lines to understand which line diagnostic is referring to. From my experience it is easy to understand something like

blah  // expected-error
      // expected-note@-1

because you don't need to calculate lines, it is clear -1 refers to the previous line. But in your case I have to calculate 6 or 7 lines back.

Here is for comparison "expected-" annotations closer to corresponding lines

template <typename A, typedef B> // expected-error{{expected template parameter}}
                                 // expected-note@-1{{did you mean to use 'typename'?}}
// CHECK: fix-it:{{.*}}:{4:23-4:30}:"typename"
struct Foo {
  // Should not produce error about type since parsing speculatively with fixit applied.
  B member;
  // Let's just check that other errors get reported.
  a // expected-error{{unknown type name 'a'}}
}; // expected-error{{expected member name or ';' after declaration specifiers}}

I'm not entirely happy with the result as it looks too dense and noisy but it conveys my thought.

[jkorous-apple]

Tried to make test more readable.

[jkorous-apple]

Thanks for your patience with me still learning lit/filecheck. What do you think about the test now? I totally agree that this is subjective matter but I appreciate your feedback.

[vsapsai]

One small note about -fdiagnostics-parseable-fixits. The rest looks good to me. And the test looks readable.

FixIt/fixit-typedef-instead-of-typename-typo.cpp

1 ↗

(On Diff #132135)

I don't think you need -fdiagnostics-parseable-fixits here. Though it is required in the second RUN line.

include/clang/Basic/DiagnosticParseKinds.td

 def err_objc_expected_type_parameter : Error<
   "expected type parameter name">;
 
 def err_objc_parameterized_implementation : Error<
   "@implementation cannot have type parameters">;
 
 def err_objc_type_args_after_protocols : Error<
   "protocol qualifiers must precede type arguments">;
+
+def note_meant_to_use_typename : Note<
+  "did you mean to use 'typename'?">;
 }
 
 let CategoryName = "Coroutines Issue" in {
 def err_for_co_await_not_range_for : Error<
   "'co_await' modifier can only be applied to range-based for loop">;
 }
 
 } // end of Parser diagnostics

lib/Parse/ParseTemplate.cpp

 ///               = id-expression
 NamedDecl *Parser::ParseTemplateParameter(unsigned Depth, unsigned Position) {
   if (isStartOfTemplateTypeParameter())
     return ParseTypeParameter(Depth, Position);
 
   if (Tok.is(tok::kw_template))
     return ParseTemplateTemplateParameter(Depth, Position);
 
+  // Is there just a typo in the input code? ('typedef' instead of 'typename')
+  if (Tok.is(tok::kw_typedef)) {
+    Diag(Tok.getLocation(), diag::err_expected_template_parameter);
+
+    Diag(Tok.getLocation(), diag::note_meant_to_use_typename)
+        << FixItHint::CreateReplacement(CharSourceRange::getCharRange(
+                                            Tok.getLocation(), Tok.getEndLoc()),
+                                        "typename");
+
+    Tok.setKind(tok::kw_typename);
+
+    return ParseTypeParameter(Depth, Position);
+  }
+
   // If it's none of the above, then it must be a parameter declaration.
   // NOTE: This will pick up errors in the closure of the template parameter
   // list (e.g., template < ; Check here to implement >> style closures.
   return ParseNonTypeTemplateParameter(Depth, Position);
 }
 
 /// ParseTypeParameter - Parse a template type parameter (C++ [temp.param]).
 /// Other kinds of template parameters are parsed in

test/CXX/temp/temp.param/p2.cpp

 
 // typename followed by aqualified-id denotes the type in a non-type
 // parameter-declaration.
 template<typename T, typename T::type Value> struct Y0;
 template<typename T, typename X<T>::type Value> struct Y1;
 
 // A storage class shall not be specified in a template-parameter declaration.
 template<static int Value> struct Z; //expected-error{{invalid declaration specifier}}
-template<typedef int Value> struct Z0; //expected-error{{invalid declaration specifier}}
-template<extern inline int Value> struct Z0; //expected-error2{{invalid declaration specifier}}
-template<virtual int Value> struct Z0; //expected-error{{invalid declaration specifier}}
-template<explicit int Value> struct Z0; //expected-error{{invalid declaration specifier}}
-template<inline int Value> struct Z0; //expected-error{{invalid declaration specifier}}
-template<extern int> struct Z0; //expected-error{{invalid declaration specifier}}
-template<static int> struct Z0;  //expected-error{{invalid declaration specifier}}
-template<explicit int Value> struct Z0; //expected-error{{invalid declaration specifier}}
-template<mutable int> struct Z0; //expected-error{{invalid declaration specifier}}
+template<typedef int Value> struct Z0; //expected-error{{expected template parameter}} expected-error{{expected identifier}} expected-error{{extraneous 'template<>' in declaration of struct 'Z0'}} expected-note{{did you mean to use 'typename'?}}
+template<extern inline int Value> struct Z1; //expected-error2{{invalid declaration specifier}}
+template<virtual int Value> struct Z2; //expected-error{{invalid declaration specifier}}
+template<explicit int Value> struct Z3; //expected-error{{invalid declaration specifier}}
+template<inline int Value> struct Z4; //expected-error{{invalid declaration specifier}}
+template<extern int> struct Z5; //expected-error{{invalid declaration specifier}}
+template<static int> struct Z6;  //expected-error{{invalid declaration specifier}}
+template<explicit int Value> struct Z7; //expected-error{{invalid declaration specifier}}
+template<mutable int> struct Z8; //expected-error{{invalid declaration specifier}}
 
-template<const int> struct Z0; // OK 
-template<volatile int> struct Z0; // OK
+template<const int> struct Z9; // OK
+template<volatile int> struct Z10; // OK
 
 
 
 #ifdef CPP11
-template<thread_local int> struct Z0; //expected-error{{invalid declaration specifier}}
-template<constexpr int> struct Z0; //expected-error{{invalid declaration specifier}}
+template<thread_local int> struct Z11; //expected-error{{invalid declaration specifier}}
+template<constexpr int> struct Z12; //expected-error{{invalid declaration specifier}}
 
 #endif
 
 #ifdef CPP17
-template<auto> struct Z1; // OK
+template<auto> struct Z13; // OK
 #endif
 
 // Make sure that we properly disambiguate non-type template parameters that
 // start with 'class'.
 class X1 { };
 template<class X1 *xptr> struct Y2 { };
 
 // FIXME: add the example from p2

test/FixIt/fixit-typedef-instead-of-typename-typo.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+template <typename A, typedef B> struct Foo {
+  // expected-error@-1 {{expected template parameter}} expected-note@-1 {{did you mean to use 'typename'?}}
+
+  // Check that we are speculatively (with fixit applied) trying to parse the rest.
+
+  // Should not produce error about type since parsing speculatively with fixit applied.
+  B member;
+
+  a // expected-error {{unknown type name 'a'}} // expected-error@+1 {{expected member name or ';' after declaration specifiers}}
+};
+
+
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// CHECK: fix-it:{{.*}}:{3:23-3:30}:"typename"