From defdb6d0beadc923f7cd6f45bd4674c676b92f05 Mon Sep 17 00:00:00 2001 From: Scott Ngan Date: Mon, 7 Dec 2020 03:10:46 -0800 Subject: [PATCH] Add Java linting using google-java-format (#30444) Summary: Adds `google-java-format` linting for all `.java` files in the `ReactAndroid/` folder - Linting requires java and is now performed on the android container - https://github.com/google/google-java-format ## Changelog [Internal] [Added] - Linting for *.java files (google-java-format) Pull Request resolved: https://github.com/facebook/react-native/pull/30444 Test Plan: See this example PR for lint comments: https://github.com/facebook/react-native/pull/30512 Reviewed By: hramos Differential Revision: D25253627 Pulled By: nganbread fbshipit-source-id: e39e4411bf09a96c054afaf6c12b3d05a80f40fa --- .circleci/config.yml | 44 ++++--- bots/README.md | 2 +- bots/code-analysis-bot.js | 148 ++++++++++++---------- package.json | 1 + scripts/circleci/analyze_code.sh | 2 +- scripts/lint-java.js | 205 +++++++++++++++++++++++++++++++ 6 files changed, 318 insertions(+), 84 deletions(-) create mode 100644 scripts/lint-java.js diff --git a/.circleci/config.yml b/.circleci/config.yml index 3e7f86577b3..9d1ada2052c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,10 +15,10 @@ defaults: &defaults environment: - GIT_COMMIT_DESC: git log --format=oneline -n 1 $CIRCLE_SHA1 # The public github tokens are publicly visible by design - - PUBLIC_PULLBOT_GITHUB_TOKEN_A: "a6edf8e8d40ce4e8b11a" - - PUBLIC_PULLBOT_GITHUB_TOKEN_B: "150e1341f4dd9c944d2a" - - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: &github_token_a "78a72af35445ca3f8180" - - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: &github_token_b "b1a98e0bbd56ff1ccba1" + - PUBLIC_PULLBOT_GITHUB_TOKEN_A: &github_pullbot_token_a "a6edf8e8d40ce4e8b11a" + - PUBLIC_PULLBOT_GITHUB_TOKEN_B: &github_pullbot_token_b "150e1341f4dd9c944d2a" + - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: &github_analysisbot_token_a "312d354b5c36f082cfe9" + - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: &github_analysisbot_token_b "07973d757026bdd9f196" # ------------------------- # EXECUTORS @@ -44,8 +44,10 @@ executors: - GRADLE_OPTS: '-Dorg.gradle.daemon=false -Dorg.gradle.jvmargs="-XX:+HeapDumpOnOutOfMemoryError"' - BUILD_THREADS: 2 # Repeated here, as the environment key in this executor will overwrite the one in defaults - - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: *github_token_a - - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: *github_token_b + - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: *github_analysisbot_token_a + - PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: *github_analysisbot_token_b + - PUBLIC_PULLBOT_GITHUB_TOKEN_A: *github_pullbot_token_a + - PUBLIC_PULLBOT_GITHUB_TOKEN_B: *github_pullbot_token_b reactnativeios: <<: *defaults macos: @@ -234,17 +236,17 @@ jobs: # Issues will be posted to the PR itself via GitHub bots. # This workflow should only fail if the bots fail to run. analyze_pr: - executor: nodelts + executor: reactnativeandroid steps: - restore_cache_checkout: - checkout_type: node + checkout_type: android - run_yarn - install_github_bot_deps - run: name: Install additional GitHub bot dependencies - command: sudo apt update && sudo apt install -y shellcheck jq + command: apt update && apt install -y shellcheck jq - run: name: Run linters against modified files (analysis-bot) @@ -262,10 +264,10 @@ jobs: # JOBS: Analyze Code # ------------------------- analyze_code: - executor: nodelts + executor: reactnativeandroid steps: - restore_cache_checkout: - checkout_type: node + checkout_type: android - setup_artifacts - run_yarn @@ -274,6 +276,11 @@ jobs: command: scripts/circleci/exec_swallow_error.sh yarn lint --format junit -o ./reports/junit/eslint/results.xml when: always + - run: + name: Lint Java + command: scripts/circleci/exec_swallow_error.sh yarn lint-java --check + when: always + - run: name: Check for errors in code using Flow (iOS) command: yarn flow-check-ios @@ -847,11 +854,18 @@ workflows: analysis: jobs: - - setup + - setup: + name: setup_js + + - setup: + name: setup_android + checkout_type: android + executor: reactnativeandroid + # Run lints on every commit other than those to the gh-pages branch - analyze_code: requires: - - setup + - setup_android filters: branches: ignore: gh-pages @@ -859,7 +873,7 @@ workflows: # Run code checks on PRs from forks - analyze_pr: requires: - - setup + - setup_android filters: branches: only: /^pull\/.*$/ @@ -867,7 +881,7 @@ workflows: # Gather coverage - js_coverage: requires: - - setup + - setup_js nightly: triggers: - schedule: diff --git a/bots/README.md b/bots/README.md index d108148033d..884a2ab55c6 100644 --- a/bots/README.md +++ b/bots/README.md @@ -15,7 +15,7 @@ DANGER_GITHUB_API_TOKEN=[ENV_ABOVE] yarn danger pr https://github.com/facebook/r The code analysis bot provides lint and other results as inline reviews on GitHub. It runs as part of the Circle CI analysis workflow. If you want to test changes to the Code Analysis Bot, I'd recommend checking out an existing PR and then running the `analyze pr` command. -You'll need a GitHub token. You can re-use this one: `78a72af35445ca3f8180` `b1a98e0bbd56ff1ccba1` (just remove the space). +You'll need a GitHub token. You can re-use this one: `312d354b5c36f082cfe9` `07973d757026bdd9f196` (just remove the space). So, for example: ``` diff --git a/bots/code-analysis-bot.js b/bots/code-analysis-bot.js index 55a80a9edec..5da10c52175 100644 --- a/bots/code-analysis-bot.js +++ b/bots/code-analysis-bot.js @@ -34,6 +34,8 @@ const converterSummary = { '`flow` found some issues. Run `yarn flow check` to analyze your code and address any errors.', shellcheck: '`shellcheck` found some issues. Run `yarn shellcheck` to analyze shell scripts.', + 'google-java-format': + '`google-java-format` found some issues. See https://github.com/google/google-java-format', }; /** @@ -57,6 +59,24 @@ const converters = { } }, + 'google-java-format': function(output, input) { + if (!input) { + return; + } + + input.forEach(function(change) { + push(output, change.file, { + message: `\`google-java-format\` suggested changes: +\`\`\`diff +${change.description} +\`\`\` +`, + line: change.line, + converter: 'google-java-format', + }); + }); + }, + flow: function(output, input) { if (!input || !input.errors) { return; @@ -113,30 +133,6 @@ const converters = { }, }; -function getShaFromPullRequest(octokit, owner, repo, number, callback) { - octokit.pullRequests.get({owner, repo, number}, (error, res) => { - if (error) { - console.error(error); - return; - } - - callback(res.data.head.sha); - }); -} - -function getFilesFromPullRequest(octokit, owner, repo, number, callback) { - octokit.pullRequests.listFiles( - {owner, repo, number, per_page: 100}, - (error, res) => { - if (error) { - console.error(error); - return; - } - callback(res.data); - }, - ); -} - /** * Sadly we can't just give the line number to github, we have to give the * line number relative to the patch file which is super annoying. This @@ -166,7 +162,15 @@ function getLineMapFromPatch(patchString) { return lineMap; } -function sendReview(octokit, owner, repo, number, commit_id, body, comments) { +async function sendReview( + octokit, + owner, + repo, + pull_number, + commit_id, + body, + comments, +) { if (process.env.GITHUB_TOKEN) { if (comments.length === 0) { // Do not leave an empty review. @@ -181,19 +185,14 @@ function sendReview(octokit, owner, repo, number, commit_id, body, comments) { const opts = { owner, repo, - number, + pull_number, commit_id, body, event, comments, }; - octokit.pullRequests.createReview(opts, function(error, res) { - if (error) { - console.error(error); - return; - } - }); + await octokit.pulls.createReview(opts); } else { if (comments.length === 0) { console.log('No issues found.'); @@ -216,7 +215,7 @@ function sendReview(octokit, owner, repo, number, commit_id, body, comments) { } } -function main(messages, owner, repo, number) { +async function main(messages, owner, repo, pull_number) { // No message, we don't need to do anything :) if (Object.keys(messages).length === 0) { return; @@ -234,40 +233,54 @@ function main(messages, owner, repo, number) { auth: process.env.GITHUB_TOKEN, }); - getShaFromPullRequest(octokit, owner, repo, number, sha => { - getFilesFromPullRequest(octokit, owner, repo, number, files => { - let comments = []; - let convertersUsed = []; - files - .filter(file => messages[file.filename]) - .forEach(file => { - // github api sometimes does not return a patch on large commits - if (!file.patch) { - return; - } - const lineMap = getLineMapFromPatch(file.patch); - messages[file.filename].forEach(message => { - if (lineMap[message.line]) { - const comment = { - path: file.filename, - position: lineMap[message.line], - body: message.message, - }; - convertersUsed.push(message.converter); - comments.push(comment); - } - }); // forEach - }); // filter + const opts = { + owner, + repo, + pull_number, + }; - let body = '**Code analysis results:**\n\n'; - const uniqueconvertersUsed = [...new Set(convertersUsed)]; - uniqueconvertersUsed.forEach(converter => { - body += '* ' + converterSummary[converter] + '\n'; - }); + const {data: pull} = await octokit.pulls.get(opts); + const {data: files} = await octokit.pulls.listFiles(opts); - sendReview(octokit, owner, repo, number, sha, body, comments); - }); // getFilesFromPullRequest - }); // getShaFromPullRequest + const comments = []; + const convertersUsed = []; + + files + .filter(file => messages[file.filename]) + .forEach(file => { + // github api sometimes does not return a patch on large commits + if (!file.patch) { + return; + } + const lineMap = getLineMapFromPatch(file.patch); + messages[file.filename].forEach(message => { + if (lineMap[message.line]) { + const comment = { + path: file.filename, + position: lineMap[message.line], + body: message.message, + }; + convertersUsed.push(message.converter); + comments.push(comment); + } + }); // forEach + }); // filter + + let body = '**Code analysis results:**\n\n'; + const uniqueconvertersUsed = [...new Set(convertersUsed)]; + uniqueconvertersUsed.forEach(converter => { + body += '* ' + converterSummary[converter] + '\n'; + }); + + await sendReview( + octokit, + owner, + repo, + pull_number, + pull.head.sha, + body, + comments, + ); } let content = ''; @@ -331,6 +344,7 @@ process.stdin.on('end', function() { const number = process.env.GITHUB_PR_NUMBER; - // intentional lint warning to make sure that the bot is working :) - main(messages, owner, repo, number); + (async () => { + await main(messages, owner, repo, number); + })(); }); diff --git a/package.json b/package.json index 42df7b41226..175d9fe880e 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "flow-check-android": "flow check --flowconfig-name .flowconfig.android", "lint": "eslint .", "lint-ci": "./scripts/circleci/analyze_code.sh && yarn shellcheck", + "lint-java": "node ./scripts/lint-java.js", "shellcheck": "./scripts/circleci/analyze_scripts.sh", "clang-format": "clang-format -i --glob=*/**/*.{h,cpp,m,mm}", "format": "npm run prettier && npm run clang-format", diff --git a/scripts/circleci/analyze_code.sh b/scripts/circleci/analyze_code.sh index fbf02e7668b..a2eda88385b 100755 --- a/scripts/circleci/analyze_code.sh +++ b/scripts/circleci/analyze_code.sh @@ -9,7 +9,7 @@ GITHUB_REPO=${CIRCLE_PROJECT_REPONAME:-react-native} export GITHUB_OWNER export GITHUB_REPO -cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run flow-check-ios --silent --json; echo flow; npm run flow-check-android --silent --json) | GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" node bots/code-analysis-bot.js +cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run flow-check-ios --silent --json; echo flow; npm run flow-check-android --silent --json; echo google-java-format; node scripts/lint-java.js --diff) | GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" node bots/code-analysis-bot.js STATUS=$? if [ $STATUS == 0 ]; then diff --git a/scripts/lint-java.js b/scripts/lint-java.js new file mode 100644 index 00000000000..82e07bb6eda --- /dev/null +++ b/scripts/lint-java.js @@ -0,0 +1,205 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +'use strict'; + +const {exec} = require('shelljs'); +const https = require('https'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const yargs = require('yargs'); + +const googleJavaFormatUrl = + 'https://github.com/google/google-java-format/releases/download/google-java-format-1.7/google-java-format-1.7-all-deps.jar'; +const googleJavaFormatPath = path.join( + os.tmpdir(), + 'google-java-format-all-deps.jar', +); +const javaFilesCommand = 'find ./ReactAndroid -name "*.java"'; + +function _download(url, downloadPath, resolve, reject, redirectCount) { + https.get(url, response => { + switch (response.statusCode) { + case 302: //Permanent Redirect + if (redirectCount === 0) { + throw new Error( + `Unhandled response code (HTTP${response.statusCode}) while retrieving google-java-format binary from ${url}`, + ); + } + + _download( + response.headers.location, + downloadPath, + resolve, + reject, + redirectCount - 1, + ); + break; + case 200: //OK + const file = fs.createWriteStream(downloadPath); + + response.pipe(file); + file.on('finish', () => file.close(() => resolve())); + break; + default: + reject( + `Unhandled response code (HTTP${response.statusCode}) while retrieving google-java-format binary from ${url}`, + ); + } + }); +} + +function download(url, downloadPath) { + return new Promise((resolve, reject) => { + _download(url, downloadPath, resolve, reject, 1); + }); +} + +function filesWithLintingIssues() { + const proc = exec( + `java -jar ${googleJavaFormatPath} --dry-run $(${javaFilesCommand})`, + {silent: true}, + ); + + if (proc.code !== 0) { + throw new Error(proc.stderr); + } + + return proc.stdout.split('\n').filter(x => x); +} + +function unifiedDiff(file) { + const lintedProc = exec( + `java -jar ${googleJavaFormatPath} --set-exit-if-changed ${file}`, + {silent: true}, + ); + + //Exit code 1 indicates lint violations, which is what we're expecting + if (lintedProc.code !== 1) { + throw new Error(lintedProc.stderr); + } + + const diffProc = lintedProc.exec(`diff -U 0 ${file} -`, {silent: true}); + + //Exit code 0 if inputs are the same, 1 if different, 2 if trouble. + if (diffProc.code !== 0 && diffProc.code !== 1) { + throw new Error(diffProc.stderr); + } + + return { + file, + diff: diffProc.stdout, + }; +} + +function extractRangeInformation(range) { + //eg; + // @@ -54 +54,2 @@ + // @@ -1,3 +1,9 @@ + + const regex = /^@@ [-+](\d+,?\d+) [-+](\d+,?\d+) @@$/; + const match = regex.exec(range); + + if (match) { + const original = match[1].split(','); + const updated = match[2].split(','); + + return { + original: { + line: parseInt(original[0], 10), + lineCount: parseInt(original[1], 10) || 1, + }, + updated: { + line: parseInt(updated[0], 10), + lineCount: parseInt(updated[1], 10) || 1, + }, + }; + } +} + +function parseChanges(file, diff) { + let group = null; + const groups = []; + + diff.split('\n').forEach(line => { + const range = extractRangeInformation(line); + + if (range) { + group = { + range, + description: [line], + }; + groups.push(group); + } else if (group) { + group.description.push(line); + } + }); + + return groups.map(x => ({ + file, + line: x.range.original.line, + lineCount: x.range.original.lineCount, + description: x.description.join('\n'), + })); +} + +async function main() { + const {argv} = yargs + .scriptName('lint-java') + .usage('Usage: $0 [options]') + .command( + '$0', + 'Downloads the google-java-format package and reformats Java source code to comply with Google Java Style.\n\nSee https://github.com/google/google-java-format', + ) + .option('check', { + type: 'boolean', + description: + 'Outputs a list of files with lint violations.\nExit code is set to 1 if there are violations, otherwise 0.\nDoes not reformat lint issues.', + }) + .option('diff', { + type: 'boolean', + description: + 'Outputs a diff of the lint fix changes in json format.\nDoes not reformat lint issues.', + }); + + await download(googleJavaFormatUrl, googleJavaFormatPath); + + if (argv.check) { + const files = filesWithLintingIssues(); + + files.forEach(x => console.log(x)); + + process.exit(files.length === 0 ? 0 : 1); + + return; + } + + if (argv.diff) { + const suggestions = filesWithLintingIssues() + .map(unifiedDiff) + .filter(x => x) + .map(x => parseChanges(x.file, x.diff)) + .reduce((accumulator, current) => accumulator.concat(current), []); + + console.log(JSON.stringify(suggestions)); + + return; + } + + const proc = exec( + `java -jar ${googleJavaFormatPath} --set-exit-if-changed --replace $(${javaFilesCommand})`, + ); + + process.exit(proc.code); +} + +(async () => { + await main(); +})();