[clang-tidy] Fix example provided by add_new_check.py

[PiotrZSL]

Summary

Currently test for newly added check in clang-tidy fails,
this is because Fix is attached to note, but test is executed
without --fix-notes. This change adding --fix-notes to test.

Fixes: https://github.com/llvm/llvm-project/issues/50400

[PiotrZSL]

Added issue reference

[carlosgalvezp]

clang-tools-extra/clang-tidy/add_new_check.py

278

There is only 6 check tests in the clang-tidy folder that use --fix-notes, so I believe this is not quite standard. Thus I'm not convinced this should be part of the example test, which is supposed to be fairly basic. Would it be better to change the example check instead, so that the fix is not attached to the note? Example:

diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome, prepend it with 'awesome_'")
    << MatchedDecl
    << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");

[PiotrZSL]

I know that it would be better, best would be to remove that --fix-notes completely and enable those notes fixes with basic --fix, as it only create confusion.

Originally it were like this:
"Added an option to control whether to apply the fixes found in notes attached to clang tidy errors or not.
Diagnostics may contain multiple notes each offering different ways to fix the issue, for that reason the default behaviour should be to not look at fixes found in notes.
Instead offer up all the available fix-its in the output but don't try to apply the first one unless -fix-notes is supplied.
"
Commit: abbe9e227ed31e5dde9bb7567bb9f0dd047689c6

But that is stupid, because there could be conflicts between those fixes.
And this were added by D59932.

@njames93 What do you think, after all you broke this.
Option A) Change generator to attach fix to warning instead of note.
Option B) Revert your commit that introduced --fix-notes
Option C) Introduce option --dont-fix-notes, and enable notes fixes by default.

[carlosgalvezp]

Thanks for digging into this! It looks a bit strange to me:

Usually developers have to preview a code diff (before vs after apply the fix) to understand what the fix does before applying a fix.

This is not quite true - clang-tidy displays the fix it hint when running it, even if it's not a note. As a human you don't need to apply fixes and then run a diff to understand what the change is about.

source>:1:9: warning: C-style casts are discouraged; use static_cast [google-readability-casting]
int x = (int) 1.2F;
        ^~~~~~
        static_cast<int>( )

The patch also said:

will add implementations for existing checks in the future

Which didn't actually happen, the vast majority of checks do not attach fix it hints to notes.

Since that comment is 4 years old, perhaps is just obsolete?

I think easiest is to go for Option A) - this is how we normally write checks nowadays. Maybe the discussion about removing support for --fix-notes can be done in an RFC?

[PiotrZSL]

Ok, lets do that, I will change this.

[njames93]

In D146875#4229544, @carlosgalvezp wrote:

I think easiest is to go for Option A) - this is how we normally write checks nowadays. Maybe the discussion about removing support for --fix-notes can be done in an RFC?

Fix notes was added to address the situation that clang-tidy just blindly applies the first fix found with a warning, The behaviour contradicts with how clang handles fixes.
The basic idea is if a fix is attached to a warning, we have very high confidence that it is the correct fix and wont contradict the programmers initial intent.
Fixes attached to notes are for cases when there a multiple possible ways to silence a warning, or if the way to silence a warning could change the programmers initial intent.

Fix notes basically reverts clang tidy to use the old behaviour of just blindly applying the first fix it finds, which some users may wish to use, there fore shouldn't be removed.

The simplest thing here is to just update the example generator script to attach the fix directly to the warning.
In fact, I'd go as far as saying including adding --fix-notes to the test file can harm new checks in future as fixes may not be applied when running normally

[PiotrZSL]

Move fix from note to warning

[PiotrZSL]

Remove change added by mistake

[PiotrZSL]

Remove added --

clang-tools-extra/clang-tidy/add_new_check.py

 }
 
 void %(check_name)s::check(const MatchFinder::MatchResult &Result) {
   // FIXME: Add callback implementation.
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("x");
   if (!MatchedDecl->getIdentifier() || MatchedDecl->getName().startswith("awesome_"))
     return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-      << MatchedDecl;
-  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
+      << MatchedDecl
       << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
+  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note);
 }
 
 } // namespace clang::tidy::%(namespace)s
 """ % {'check_name': check_name_camel,
        'module': module,
        'namespace': namespace})
 
 

 # Adds a test for the check.
 def write_test(module_path, module, check_name, test_extension):
   check_name_dashes = module + '-' + check_name
   filename = os.path.normpath(os.path.join(
     module_path, '..', '..', 'test', 'clang-tidy', 'checkers',
     module, check_name + '.' + test_extension))
   print('Creating %s...' % filename)
   with io.open(filename, 'w', encoding='utf8', newline='\n') as f:
     f.write("""// RUN: %%check_clang_tidy %%s %(check_name_dashes)s %%t
 
 // FIXME: Add something that triggers the check here.
 void f();
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [%(check_name_dashes)s]
 
 // FIXME: Verify the applied fix.
 //   * Make the CHECK patterns specific enough and try to make verified lines
 //     unique to avoid incorrect matches.
 //   * Use {{}} for regular expressions.
 // CHECK-FIXES: {{^}}void awesome_f();{{$}}
 
 // FIXME: Add something that doesn't trigger the check here.
 void awesome_f2();
 """ % {'check_name_dashes': check_name_dashes})
 
 
 def get_actual_filename(dirname, filename):
-  if not os.path.isdir(dirname): 
+  if not os.path.isdir(dirname):
     return ''
   name = os.path.join(dirname, filename)
   if (os.path.isfile(name)):
     return name
   caselessname = filename.lower()
   for file in os.listdir(dirname):
     if (file.lower() == caselessname):
       return os.path.join(dirname, file)