diff --git a/bot/code_review_bot/tasks/lint.py b/bot/code_review_bot/tasks/lint.py index 220fc653a..884f28b74 100644 --- a/bot/code_review_bot/tasks/lint.py +++ b/bot/code_review_bot/tasks/lint.py @@ -34,8 +34,20 @@ def __init__( message, check, revision, - **kwargs + diff=None, ): + base_line = lineno and int(lineno) or 0 + nb_lines = 1 + + if diff: + try: + base_line, nb_lines = self.parse_diff(diff, base_line) + + # Add diff to message + message += f"\n```\n{diff}\n```" + except Exception as e: + logger.warning(f"Failed to parse diff: {e}") + # Use analyzer name when check is not provided # This happens for analyzers who only have one rule if check is None: @@ -44,10 +56,8 @@ def __init__( analyzer, revision, path, - line=( - lineno and int(lineno) or 0 - ), # mozlint sometimes produce strings here - nb_lines=1, + line=base_line, + nb_lines=nb_lines, check=check, column=column, level=Level(level), @@ -55,6 +65,29 @@ def __init__( ) self.linter = linter + def parse_diff(self, diff, base_line): + """ + Parse an optional Mozlint diff to get the lines from the original patch + """ + changes = list( + filter( + None, + [ + (i, c[0]) if c and c[0] in ("-", "+") else None + for i, c in enumerate(diff.splitlines(), start=base_line) + ], + ) + ) + assert changes, "No changes in diff" + + # First change has the position of the issue's top line + base_line, _ = changes[0] + + # We only consider the old lines as being in patch + nb_lines = len([c for c in changes if c[1] == "-"]) or 1 + + return base_line, nb_lines + def is_disabled_check(self): """ Some checks are disabled: @@ -126,6 +159,7 @@ def parse_issues(self, artifacts, revision): linter=issue["linter"], message=issue["message"], check=issue["rule"], + diff=issue.get("diff"), ) for artifact in artifacts.values() for _, path_issues in artifact.items() diff --git a/bot/tests/fixtures/mozlint_rustfmt_issues.json b/bot/tests/fixtures/mozlint_rustfmt_issues.json new file mode 100644 index 000000000..8a3f6759a --- /dev/null +++ b/bot/tests/fixtures/mozlint_rustfmt_issues.json @@ -0,0 +1,272 @@ +{ + "/build/checkout/": [ + { + "linter": "rust", + "path": "/build/checkout/", + "message": "Reformat rust", + "lineno": 0, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "Warning: can't set `binop_separator = Back`, unstable features are only available in nightly channel.\nWarning: can't set `match_block_trailing_comma = true`, unstable features are only available in nightly channel.", + "relpath": "." + } + ], + "/build/checkout/tools/fuzzing/rust/src/lib.rs": [ + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 16, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " use tempfile::Builder;\n\n fn eat_lmdb_err(value: Result) -> Result, rkv::StoreError> {\n- // Should trigger rustfmt in patch\n- match value {\n+ // Should trigger rustfmt in patch\n+ match value {\n Ok(value) => Ok(Some(value)),\n Err(rkv::StoreError::LmdbError(_)) => Ok(None),\n Err(err) => {", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 24, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " println!(\"Not a crash, but an error outside LMDB: {}\", err);\n println!(\"A refined fuzzing test, or changes to RKV, might be required.\");\n Err(err)\n- },\n+ }\n }\n }", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 44, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "\n let &mut builder = rkv::Rkv::environment_builder().set_max_dbs(2);\n let env = rkv::Rkv::from_env(Path::new(\".\"), builder).unwrap();\n- let store = env.open_single(\"test\", rkv::StoreOptions::create()).unwrap();\n+ let store = env\n+ .open_single(\"test\", rkv::StoreOptions::create())\n+ .unwrap();\n\n let reader = env.read().unwrap();\n eat_lmdb_err(store.get(&reader, &[0])).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 78, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " pub extern \"C\" fn fuzz_rkv_key_write(raw_data: *const u8, size: libc::size_t) -> libc::c_int {\n let data = unsafe { std::slice::from_raw_parts(raw_data as *const u8, size as usize) };\n\n- let root = Builder::new().prefix(\"fuzz_rkv_key_write\").tempdir().unwrap();\n+ let root = Builder::new()\n+ .prefix(\"fuzz_rkv_key_write\")\n+ .tempdir()\n+ .unwrap();\n fs::create_dir_all(root.path()).unwrap();\n\n let env = rkv::Rkv::new(root.path()).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 85, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- let store = env.open_single(\"test\", rkv::StoreOptions::create()).unwrap();\n+ let store = env\n+ .open_single(\"test\", rkv::StoreOptions::create())\n+ .unwrap();\n\n let mut writer = env.write().unwrap();\n // Some data are invalid values, and are handled as store errors.", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 96, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " pub extern \"C\" fn fuzz_rkv_val_write(raw_data: *const u8, size: libc::size_t) -> libc::c_int {\n let data = unsafe { std::slice::from_raw_parts(raw_data as *const u8, size as usize) };\n\n- let root = Builder::new().prefix(\"fuzz_rkv_val_write\").tempdir().unwrap();\n+ let root = Builder::new()\n+ .prefix(\"fuzz_rkv_val_write\")\n+ .tempdir()\n+ .unwrap();\n fs::create_dir_all(root.path()).unwrap();\n\n let env = rkv::Rkv::new(root.path()).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 103, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- let store = env.open_single(\"test\", rkv::StoreOptions::create()).unwrap();\n+ let store = env\n+ .open_single(\"test\", rkv::StoreOptions::create())\n+ .unwrap();\n\n let mut writer = env.write().unwrap();\n let string = String::from_utf8_lossy(data);", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 129, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "\n #[no_mangle]\n pub extern \"C\" fn fuzz_rkv_calls(raw_data: *const u8, size: libc::size_t) -> libc::c_int {\n- let mut fuzz = unsafe { std::slice::from_raw_parts(raw_data as *const u8, size as usize).to_vec() };\n+ let mut fuzz =\n+ unsafe { std::slice::from_raw_parts(raw_data as *const u8, size as usize).to_vec() };\n\n fn maybe_do(fuzz: &mut Vec, f: impl FnOnce(&mut Vec) -> ()) -> Option<()> {\n match fuzz.pop().map(|byte| byte % 2) {", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 136, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " Some(0) => Some(f(fuzz)),\n- _ => None\n+ _ => None,\n }\n }", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 141, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- fn maybe_abort(fuzz: &mut Vec, read: rkv::Reader) -> Result<(), rkv::StoreError> {\n+ fn maybe_abort(fuzz: &mut Vec, read: rkv::Reader) -> Result<(), rkv::StoreError> {\n match fuzz.pop().map(|byte| byte % 2) {\n Some(0) => Ok(read.abort()),\n- _ => Ok(())\n+ _ => Ok(()),\n }\n };", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 148, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- fn maybe_commit(fuzz: &mut Vec, write: rkv::Writer) -> Result<(), rkv::StoreError> {\n+ fn maybe_commit(fuzz: &mut Vec, write: rkv::Writer) -> Result<(), rkv::StoreError> {\n match fuzz.pop().map(|byte| byte % 3) {\n Some(0) => write.commit(),\n Some(1) => Ok(write.abort()),", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 152, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- _ => Ok(())\n+ _ => Ok(()),\n }\n };", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 187, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " });\n\n let env = rkv::Rkv::from_env(root.path(), builder).unwrap();\n- let store = env.open_single(\"test\", rkv::StoreOptions::create()).unwrap();\n+ let store = env\n+ .open_single(\"test\", rkv::StoreOptions::create())\n+ .unwrap();\n\n loop {\n match fuzz.pop().map(|byte| byte % 4) {", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 194, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " Some(0) => {\n let key = match get_key(&mut fuzz) {\n Some(value) => value,\n- None => break\n+ None => break,\n };\n let value = match get_value(&mut fuzz) {\n Some(value) => value,", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 201, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": "- None => break\n+ None => break,\n };\n let mut writer = env.write().unwrap();\n eat_lmdb_err(store.put(&mut writer, key, &rkv::Value::Str(value))).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 207, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " Some(1) => {\n let key = match get_key(&mut fuzz) {\n Some(value) => value,\n- None => break\n+ None => break,\n };\n let mut reader = env.read().unwrap();\n eat_lmdb_err(store.get(&mut reader, key)).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 216, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " Some(2) => {\n let key = match get_key(&mut fuzz) {\n Some(value) => value,\n- None => break\n+ None => break,\n };\n let mut writer = env.write().unwrap();\n eat_lmdb_err(store.delete(&mut writer, key)).unwrap();", + "relpath": "tools/fuzzing/rust/src/lib.rs" + }, + { + "linter": "rust", + "path": "/build/checkout/tools/fuzzing/rust/src/lib.rs", + "message": "Reformat rust", + "lineno": 229, + "column": null, + "hint": null, + "source": null, + "level": "warning", + "rule": null, + "lineoffset": null, + "diff": " let n = fuzz.pop().unwrap_or(1) as usize;\n builder.set_map_size(1_048_576 * (n % 100)); // 1,048,576 bytes, i.e. 1MiB.\n }\n- _ => {\n- break\n- }\n+ _ => break,\n }\n }\n\nWarning: can't set `binop_separator = Back`, unstable features are only available in nightly channel.\nWarning: can't set `match_block_trailing_comma = true`, unstable features are only available in nightly channel.\nWarning: can't set `binop_separator = Back`, unstable features are only available in nightly channel.\nWarning: can't set `match_block_trailing_comma = true`, unstable features are only available in nightly channel.\n", + "relpath": "tools/fuzzing/rust/src/lib.rs" + } + ] +} diff --git a/bot/tests/test_lint.py b/bot/tests/test_lint.py index d1f427f96..83df6c7dd 100644 --- a/bot/tests/test_lint.py +++ b/bot/tests/test_lint.py @@ -6,6 +6,7 @@ import json import os +from code_review_bot import Level from code_review_bot.tasks.lint import MozLintIssue from code_review_bot.tasks.lint import MozLintTask from conftest import FIXTURES_DIR @@ -139,3 +140,72 @@ def test_licence_payload(mock_revision, mock_hgmo): ) assert issue.check == issue.analyzer == "source-test-mozlint-license" assert issue.build_hash() == "0809d81e1e24ee94039c0e2733321a39" + + +def test_rustfmt_payload(mock_revision, mock_hgmo): + """ + Test mozlint rusfmt payload, with warnings + This payload should report only one manually created issue in patch + but with an improvement patch + """ + mock_revision.repository = "test-try" + mock_revision.mercurial_revision = "deadbeef1234" + task_status = { + "task": {"metadata": {"name": "source-test-mozlint-rustfmt"}}, + "status": {}, + } + task = MozLintTask("lintTaskId", task_status) + + # Setup patch to trigger publishable + mock_revision.lines = {"tools/fuzzing/rust/src/lib.rs": [19, 20]} + mock_revision.files = list(mock_revision.lines.keys()) + + # Load artifact related to that bug + path = os.path.join(FIXTURES_DIR, "mozlint_rustfmt_issues.json") + with open(path) as f: + issues = task.parse_issues( + {"public/code-review/mozlint": json.load(f)}, mock_revision + ) + + # Check the issues are all warnings and only one is publishable + assert len(issues) == 19 + assert {i.level for i in issues} == {Level.Warning} + publishables = [i for i in issues if i.is_publishable()] + for i in issues: + print(i) + assert len(publishables) == 1 + issue = publishables[0] + + assert ( + str(issue) + == "source-test-mozlint-rustfmt issue source-test-mozlint-rustfmt@warning tools/fuzzing/rust/src/lib.rs line 19" + ) + assert mock_revision.contains(issue) + assert issue.is_publishable() + + assert issue.as_dict() == { + "analyzer": "source-test-mozlint-rustfmt", + "check": "source-test-mozlint-rustfmt", + "column": None, + "hash": "4ea2998c1f2029f1b95c135b7a6b5100", + "in_patch": True, + "level": "warning", + "line": 19, + "message": """Reformat rust +``` + use tempfile::Builder; + + fn eat_lmdb_err(value: Result) -> Result, rkv::StoreError> { +- // Should trigger rustfmt in patch +- match value { ++ // Should trigger rustfmt in patch ++ match value { + Ok(value) => Ok(Some(value)), + Err(rkv::StoreError::LmdbError(_)) => Ok(None), + Err(err) => { +```""", + "nb_lines": 2, + "path": "tools/fuzzing/rust/src/lib.rs", + "publishable": True, + "validates": True, + }