Closed Bug 1483953 Opened 2 years ago Closed 1 year ago

Add a FixIt hint for the ExplicitImplicitChecker analysis

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ehsan, Assigned: andi)

Details

Attachments

(1 file)

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
Andi, could you please take care of that?
Flags: needinfo?(bpostelnicu)
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;
Flags: needinfo?(bpostelnicu)
Assignee: nobody → bpostelnicu
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!!)
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
Flags: needinfo?(ehsan)
FWIW testing this requires the FileCheck utility which we don't have in the tree https://llvm.org/docs/CommandGuide/FileCheck.html
Flags: needinfo?(ehsan)
Comment on attachment 9003716 [details]
Bug 1483953 - Add a FixIt hint for the ExplicitImplicitChecker analysis. r=ehsan

:Ehsan Akhgari has approved the revision.
Attachment #9003716 - Flags: review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c51e568a0a51
Add a FixIt hint for the ExplicitImplicitChecker analysis. r=Ehsan
https://hg.mozilla.org/mozilla-central/rev/c51e568a0a51
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.