staticanalysis/bot: add a text field to enabled checkers to justify their use (e.g. in reviewbot comments)

[jankeromnes]

We could for example call this field reason (or rationale / justification / why / ...):

  - name: readability-braces-around-statements
    reason: "Mandated by Mozilla's style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures"

This field could then be printed in the output when a related defect is found by ./mach static-analysis check, and also in code review comments, like so:

dom/xslt/xslt/txPatternParser.cpp:240:

Warning: Statement should be inside braces [clang-tidy: readability-braces-around-statements]
  if (!XMLUtils::isValidQName(key, &colon))
                                          ^
                                           {
Reason: Mandated by Mozilla's style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

[rv404674]

hi @jankeromnes , can i take up this bug. I am a newcomer, can you guide me on how to proceed. I am following this guide https://docs.mozilla-releng.net/develop/contribute.html. I have successfully ran these commands.

rahul (master) release-services $ sudo ./please shell staticanalysis/bot.
rahul (master) release-services $ sudo ./please check staticanalysis/bot.

But on running this command i am getting an error.
rahul (master) release-services $ sudo ./please run staticanalysis/bot
Error: Application staticanalysis/bot is not configured to be runnable.

i have docker and git installed.

[jankeromnes]

Hi @rv404674, thanks a lot for your interest! We're so happy that you'd like to help. 😄

I'll do my best to guide you through the process, and if something doesn't make sense to you please never hesitate to ask.

The docs at https://docs.mozilla-releng.net/projects/shipit-static-analysis.html have a bit more detail on how to run staticanalysis/bot locally (Notes: It was recently renamed from shipit-static-analysis, and some parts of the docs might still use the old name. Also you can entirely skip 4. Setup a Mozreview test since MozReview no longer exists).

[rv404674]

Thanks a lot ,@jankeromnes appreciated. Can you tell me which irc do you use??. And how to connect to you using that.

[jankeromnes]

Sure! We usually hang out in #release-services in Mozilla's IRC network (not Freenode). You can join us there with riot.im (I find it a bit complicated) or KiwiIRC using this direct link.

[rv404674]

hey @jankeromnes , i have messaged you on irccloud. Can you take a look.

[jankeromnes]

Hello @rv404674! Regarding:

https://usercontent.irccloud-cdn.com/file/s9liug3P/Screenshot%20from%202018-09-26%2002-56-09.png

I see the following problems:

• The PHID should be a PHID-DIFF-*, not a PHID-DREV-* (e.g. PHID-DIFF-6xlnnhssoftbejhzleev)
• The first mkdir -p /tmp/app you ran worked, so no need to re-include it in all the following commands
• The actual command to run the bot in the Nix Shell is now:

static-analysis-bot --source=phabricator --id=$PHABRICATOR --taskcluster-secret=repo:github.com/mozilla-releng/services:branch:master --cache-root=/app/tmp

(See the docs that we updated yesterday)

[jankeromnes]

A few helpful pointers for this issue:

The YAML file in which we could add a reason field to each checker lives in the Firefox repository. It is called tools/clang-tidy/config.yaml, and can be found here:

https://hg.mozilla.org/mozilla-unified/raw-file/tip/tools/clang-tidy/config.yaml

It is downloaded at runtime by our bot here:

release-services/src/staticanalysis/bot/static_analysis_bot/config.py

Lines 81 to 95 in f124876

	def download(self, defaults={}):
	'''
	Configuration is stored on mozilla central
	It has to be downloaded on each run
	'''
	assert isinstance(defaults, dict)
	assert self.config is None, \
	'Config already set.'
	resp = requests.get(CONFIG_URL)
	assert resp.ok, \
	'Failed to retrieve configuration from mozilla-central #{}'.format(resp.status_code) # noqa
	self.config = defaults
	self.config.update(yaml.load(resp.content))
	logger.info('Loaded configuration from mozilla-central')

Maybe we could start by adding a reason field to one of the checkers in our mock config.yaml (used in tests):

https://github.com/mozilla/release-services/blob/f1248769455a673e477c2a4d990fcda33adcda24/src/staticanalysis/bot/tests/mocks/config.yaml

Then, to start implementing this field, I suggest that if there exists a reason field for a specified clang-tidy checker, you copy it into a self.reason attribute in any corresponding ClangTidyIssue:

release-services/src/staticanalysis/bot/static_analysis_bot/clang/tidy.py

Line 229 in 318207d

class ClangTidyIssue(Issue):

Notes:

• ClangTidyIssues are created during a code analysis, and self.check correspond's to config.yaml's - name field
• Also, there can be several ClangTidyIssues with the same self.check, or there can be none, e.g. if no such issue was detected during analysis.

Later, when this issue is exported, we'll be able to serialize this self.reason field into the output.

[abpostelnicu]

I would advise to use the official documentation of clang-tidy here in order to argument the checker. Of course we can modify a little bit, but if we are to add some sort of argumentation we should start with the official description of each checker, for example see the description of modernize-use-equals-default.

[rv404674]

heyy @jankeromnes , i created a PR for this. Sorry if the code looks shitty.

[sylvestre]

@rv404674 bravo for this work. I think we will start using that feature more and more with coverity and infer. cc @abpostelnicu

[rv404674]

Thanks @sylvestre

[jankeromnes]

@rv404674 I'll join @sylvestre in congratulating you on this! Thanks a lot for your time and hard work, you've created a really cool and useful feature. 🙂 👍 🎉

[rv404674]

Thanks @jankeromnes , i enjoyed a lot in developing this feature. I have stopped contributing for some time to rel eng, due to my placement activities. But once it gets over, i will again start contributing. One more thing, is there any swe internship opputunity available in your team. :)

[jankeromnes]

It's always ok to stop contributing to a project, especially when we need to spend time on other things (e.g. work, studies, family, health, etc). But we're happy to review more pull requests if you're motivated! 😄

Also, unfortunately I don't think our team is hosting interns this summer, but you can always see Mozilla's current internship offers over here:

https://careers.mozilla.org/listings/?position_type=Intern

Feel free to apply to any offer that looks fun!

[rv404674]

Hi @sylvestre. I am been an active contributor to releng from last couple of months. I want to work on the project "test automation for linting tools". So can you please tell me more about it . Thanks.:)

[sylvestre]

@rv404674 Hello
Sorry for the latency, I think there is enough information in this bug to start with, don't you think ? :)

[sylvestre]

I reported this in bugzilla too: https://bugzilla.mozilla.org/show_bug.cgi?id=1539072

[rv404674]

Hi @sylvestre . Yup there is, but i will not be applying for that project. To be frank i am not interested in testing at all and i found another project in mozilla for which i am greatly excited to to and it is being used by codecoverage - TUID. I hope you won't mind me applying for different project, btw i won't stop contributing to releng in my free time. ;).