1483953 - Add a FixIt hint for the ExplicitImplicitChecker analysis

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[(no longer active)]

Now that we have our static analyses integrated with our review tools, it would be nice to generate FixIt Hints (https://clang.llvm.org/diagnostics.html) for our error messages, so that we generate error messages such as:

Bad implicit conversion constructor for 'Foo'...  Did you mean to add 'explicit' here?

Generating a FixIt Hint is easy, you can see an example here: https://github.com/AlexDenisov/ToyClangPlugin/blob/1365d16854988eea713bc025f14a4c02eaffbd0e/ToyClangPlugin.cpp#L41

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andi, could you please take care of that?

Comment 2

[Andi [:andi]]

I would do with a |FixItHint::CreateInsertion| something more or less like this:

>>  FixItHint FixItHint =
>>      FixItHint::CreateInsertion(Ctor->getLocation(), "explict ");

and the file would look like:

>>....
>>  FixItHint FixItHint =
>>      FixItHint::CreateInsertion(Ctor->getLocation(), "explicit ");
>>  diag(Ctor->getLocation(), "bad implicit conversion constructor for %0",
>>       DiagnosticIDs::Error)
>>      << Declaration->getDeclName();
>>  diag(Ctor->getLocation(),
>>       "consider adding the explicit keyword to the constructor",
>>       DiagnosticIDs::Note)
>>      << FixItHint;

Comment 3

[(no longer active)]

Yes, I think that's basically what's needed here.  Plus tests.  :-)

Please make sure the output looks nice on the command line...  I would appreciate if you can post some samples to the bug once the patch is ready.  Ideally once more developers start to use these checks locally, they will also find these warnings useful there too (if they don't build with -fix-it, that is!!)

Comment 4

[Andi [:andi]]

How does this output look like, does it seem reasonable:

>>/home/test.cpp:3:3: error: bad implicit conversion constructor for 'Foo' [mozilla-implicit-constructor]
>>  Foo(int);
>>  ^~~
>>  explicit 
>>/home/test.cpp:3:3: note: consider adding the explicit keyword to the constructor

This is generated exporting our checkers to clang-tidy.

Running through mach static-analysis:

>> 0:01.66 BUILDSTATUS TIER_FINISH export
>> 0:01.66 make: Leaving directory `/Users/abpostelnicu/Projects/mozilla/obj-ff-osx'
>> 0:01.86 /Users/abpostelnicu/Projects/mozilla/obj-ff-osx/_virtualenvs/init/bin/python /Users/abpostelnicu/.mozbuild/clang-tools/clang/share/clang/run-clang-tidy.py -j 0 -p /Users/abpostelnicu/Projects/mozilla/obj-ff-osx -clang-tidy-binary /Users/abpostelnicu/.mozbuild/clang-tools/clang/bin/clang-tidy -clang-apply-replacements-binary /Users/abpostelnicu/.mozbuild/clang-tools/clang/bin/clang-apply-replacements -checks=-*,mozilla-implicit-constructor -extra-arg=-DMOZ_CLANG_PLUGIN -header-filter=dom/cache/Action.cpp dom/cache/Action.cpp
>> 0:08.53 1 error generated.
>> 0:08.56 Error while processing /Users/abpostelnicu/Projects/mozilla/mozilla-unified/dom/cache/Action.cpp.
>> 0:08.57 /Users/abpostelnicu/Projects/mozilla/mozilla-unified/dom/cache/Action.cpp:15:3: error: bad implicit conversion constructor for 'S' [mozilla-implicit-constructor]
>> 0:08.57   S(int) {}
>> 0:08.57   ^
>> 0:08.57   explicit
>> 0:08.57 /Users/abpostelnicu/Projects/mozilla/mozilla-unified/dom/cache/Action.cpp:15:3: note: consider adding the explicit keyword to the constructor

The artifact that I've used can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f42c3cc514610c03985b41d8ad1add60fd49261c&selectedJob=195539447

Comment 5

[(no longer active)]

FWIW testing this requires the FileCheck utility which we don't have in the tree https://llvm.org/docs/CommandGuide/FileCheck.html

Comment 6

[Andi [:andi]]

Attachment: Bug 1483953 - Add a FixIt hint for the ExplicitImplicitChecker analysis. r=ehsan

Comment 7

[(no longer active)]

Comment on attachment 9003716 [details]
Bug 1483953 - Add a FixIt hint for the ExplicitImplicitChecker analysis. r=ehsan

:Ehsan Akhgari has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c51e568a0a51
Add a FixIt hint for the ExplicitImplicitChecker analysis. r=Ehsan

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c51e568a0a51