-
Notifications
You must be signed in to change notification settings - Fork 331
Expand file tree
/
Copy pathcode_review.py
More file actions
1669 lines (1345 loc) · 57.4 KB
/
code_review.py
File metadata and controls
1669 lines (1345 loc) · 57.4 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
861
862
863
864
865
866
867
868
869
870
871
872
873
874
875
876
877
878
879
880
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
913
914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
970
971
972
973
974
975
976
977
978
979
980
981
982
983
984
985
986
987
988
989
990
991
992
993
994
995
996
997
998
999
1000
# -*- coding: utf-8 -*-
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this file,
# You can obtain one at http://mozilla.org/MPL/2.0/.
import enum
import json
import os
import re
from abc import ABC, abstractmethod
from collections import defaultdict
from dataclasses import asdict, dataclass
from datetime import datetime
from functools import cached_property
from itertools import chain
from logging import INFO, basicConfig, getLogger
from typing import Iterable, Literal, Optional
import tenacity
from langchain.chains import ConversationChain, LLMChain
from langchain.memory import ConversationBufferMemory
from langchain.prompts import PromptTemplate
from langchain_openai import OpenAIEmbeddings
from libmozdata.phabricator import ConduitError
from tenacity import retry, retry_if_exception_type, stop_after_attempt
from tqdm import tqdm
from unidiff import Hunk, PatchedFile, PatchSet
from unidiff.errors import UnidiffParseError
from bugbug import db, phabricator, utils
from bugbug.code_search.function_search import FunctionSearch
from bugbug.generative_model_tool import GenerativeModelTool, get_tokenizer
from bugbug.ml_filter_tool import MLCommentFilter
from bugbug.utils import get_secret
from bugbug.vectordb import PayloadScore, QueryFilter, VectorDB, VectorPoint
basicConfig(level=INFO)
logger = getLogger(__name__)
@dataclass
class InlineComment:
filename: str
start_line: int
end_line: int
content: str
on_removed_code: bool | None
id: int | None = None
date_created: int | None = None
date_modified: int | None = None
is_done: bool | None = None
hunk_start_line: int | None = None
hunk_end_line: int | None = None
is_generated: bool | None = None
explanation: str | None = None
order: int | None = None
class ModelResultError(Exception):
"""Occurs when the model returns an unexpected result."""
class FileNotInPatchError(ModelResultError):
"""Occurs when the file in the model result is not part of the patch."""
class HunkNotInPatchError(ModelResultError):
"""Occurs when the hunk in the model result is not part of the patch."""
class LargeDiffError(Exception):
"""Occurs when the diff is too large to be processed."""
TARGET_SOFTWARE: str | None = None
PROMPT_TEMPLATE_SUMMARIZATION = """You are an expert reviewer for {experience_scope}, with experience on source code reviews.
Please, analyze the code provided and report a summarization about the new changes; for that, focus on the coded added represented by lines that start with "+".
{patch}"""
PROMPT_TEMPLATE_REVIEW = """**Task**:
Generate high-quality code review comments for the patch provided below.
**Instructions**:
1. **Analyze the Changes**:
* Understand the intent and structure of the changes in the patch.
* Use the provided summarization for context, but prioritize what's visible in the diff.
2. **Identify Issues**:
* Detect bugs, logical errors, performance concerns, security issues, or violations of the `{target_code_consistency}` coding standards.
* Focus only on **new or changed lines** (lines beginning with `+`).
3. **Assess Confidence and Order**:
* **Sort the comments by descending confidence and importance**:
* Start with issues you are **certain are valid**.
* Also, prioritize important issues that you are **confident about**.
* Follow with issues that are **plausible but uncertain** (possible false positives).
* Assign each comment a numeric `order`, starting at 1.
4. **Write Clear, Constructive Comments**:
* Use **direct, declarative language**.
* Keep comments **short and specific**.
* Focus strictly on code-related concerns.
* Avoid hedging language (e.g., don’t use “maybe”, “might want to”, or form questions).
* Avoid repeating what the code is doing unless it supports your critique.
**Avoid Comments That**:
* Refer to unmodified code (lines without a `+` prefix).
* Ask for verification or confirmation (e.g., “Check if…”).
* Provide praise or restate obvious facts.
* Focus on testing.
---
**Output Format**:
Respond only with a **JSON list**. Each object must contain the following fields:
* `"file"`: The relative path to the file the comment applies to.
* `"code_line"`: The number of the specific changed line of code that the comment refers to.
* `"comment"`: A concise review comment.
* `"explanation"`: A brief rationale for the comment, including how confident you are and why.
* `"order"`: An integer indicating the comment’s priority (1 = highest confidence/importance).
---
**Examples**:
{comment_examples}
{approved_examples}
---
**Patch to Review**:
{patch}
"""
TEMPLATE_COMMENT_EXAMPLE = """Patch example {example_number}:
{patch}
Review comments for example {example_number}:
{comments}"""
PROMPT_TEMPLATE_FILTERING_ANALYSIS = """Filter review comments to keep those that:
- are consistent with the {target_code_consistency} source code;
- focus on reporting possible bugs, functional regressions, issues, or similar concerns;
- report readability or design concerns.
Exclude comments that:
- only describe the change;
- restate obvious facts like renamed variables or replaced code;
- include praising;
- ask if changes are intentional or ask to ensure things exist.
Only return a valid JSON list. Do not drop any key from the JSON objects.
Comments:
{comments}
As examples of not expected comments, not related to the current patch, please, check some below:
- {rejected_examples}
"""
DEFAULT_REJECTED_EXAMPLES = """Please note that these are minor improvements and the overall quality of the patch is good. The documentation is being expanded in a clear and structured way, which will likely be beneficial for future development.
- Please note that these are just suggestions and the code might work perfectly fine as it is. It's always a good idea to test all changes thoroughly to ensure they work as expected.
- Overall, the patch seems to be well implemented with no major concerns. The developers have made a conscious decision to align with Chrome's behavior, and the reasoning is well documented.
- There are no complex code changes in this patch, so there's no potential for major readability regressions or bugs introduced by the changes.
- The `focus(...)` method is called without checking if the element and its associated parameters exist or not. It would be better to check if the element exists before calling the `focus()` method to avoid potential errors.
- It's not clear if the `SearchService.sys.mjs` file exists or not. If it doesn't exist, this could cause an error. Please ensure that the file path is correct.
- This is a good addition to the code."""
PROMPT_TEMPLATE_DEDUPLICATE = """Please, double check the code review comments below.
Just report the comments that are not redundant and not duplicating each other.
Do not change the contents of the comments and the report format.
Adopt the template below as the report format:
[
{{
"file": "com/br/main/Pressure.java",
"code_line": 458,
"comment" : "In the third code block, you are using `nsAutoStringN<256>` instead of `nsString`. This is a good change as `nsAutoStringN<256>` is more efficient for small strings. However, you should ensure that the size of `tempString` does not exceed 256 characters, as `nsAutoStringN<256>` has a fixed size."
"explanation": "THE JUSTIFICATION GOES HERE"
}}
]
Do not report any explanation about your choice. Only return a valid JSON list.
Review:
{review}"""
PROMPT_TEMPLATE_FURTHER_INFO = """Based on the patch provided below and its related summarization, identify the functions you don't know and need to look up for reviewing the patch.
List the names of these functions, providing only the function names, with each name on a separate line.
Avoid using list indicators such as hyphens or numbers.
If no function declaration is required, just return "".
{patch}
{summarization}"""
PROMPT_TEMPLATE_FURTHER_CONTEXT_LINES = """Based on the patch provided below and its related summarization, report the code lines more context is required.
For that, list the lines with the their associated line numbers, grouping each one on a separated line.
Avoid using list indicators such as hyphens or numbers. If no code line is required, just return "".
Examples of valid code lines:
- '152 const selector = notification.getDescription();'
- '56 file.getElement(this.targetElement());'
{patch}
{summarization}"""
STATIC_COMMENT_EXAMPLES = [
{
"comment": {
"filename": "netwerk/streamconv/converters/mozTXTToHTMLConv.cpp",
"start_line": 1211,
"content": "You are using `nsAutoStringN<256>` instead of `nsString`. This is a good change as `nsAutoStringN<256>` is more efficient for small strings. However, you should ensure that the size of `tempString` does not exceed 256 characters, as `nsAutoStringN<256>` has a fixed size.",
"explanation": "THE JUSTIFICATION GOES HERE",
},
"raw_hunk": """@@ -1206,11 +1206,11 @@
} else {
uint32_t start = uint32_t(i);
i = aInString.FindChar('<', i);
if (i == kNotFound) i = lengthOfInString;
- nsString tempString;
+ nsAutoStringN<256> tempString;
tempString.SetCapacity(uint32_t((uint32_t(i) - start) * growthRate));
UnescapeStr(uniBuffer, start, uint32_t(i) - start, tempString);
ScanTXT(tempString, whattodo, aOutString);
}
}""",
},
{
"comment": {
"filename": "toolkit/components/extensions/ExtensionDNR.sys.mjs",
"start_line": 1837,
"content": "The `filterAAR` function inside `#updateAllowAllRequestRules()` is created every time the method is called. Consider defining this function outside of the method to avoid unnecessary function creation.",
"explanation": "THE JUSTIFICATION GOES HERE",
},
"raw_hunk": """@@ -1812,18 +1821,27 @@
rulesets.push(
this.makeRuleset(id, idx + PRECEDENCE_STATIC_RULESETS_BASE, rules)
);
}
this.enabledStaticRules = rulesets;
+ this.#updateAllowAllRequestRules();
}
getSessionRules() {
return this.sessionRules.rules;
}
getDynamicRules() {
return this.dynamicRules.rules;
+ }
+
+ #updateAllowAllRequestRules() {
+ const filterAAR = rule => rule.action.type === "allowAllRequests";
+ this.hasRulesWithAllowAllRequests =
+ this.sessionRules.rules.some(filterAAR) ||
+ this.dynamicRules.rules.some(filterAAR) ||
+ this.enabledStaticRules.some(ruleset => ruleset.rules.some(filterAAR));
}
}
function getRuleManager(extension, createIfMissing = true) {
let ruleManager = gRuleManagers.find(rm => rm.extension === extension);""",
},
{
"comment": {
"filename": "devtools/shared/network-observer/NetworkUtils.sys.mjs",
"start_line": 496,
"content": "The condition in the `if` statement is a bit complex and could be simplified for better readability. Consider extracting `!Components.isSuccessCode(status) && blockList.includes(ChromeUtils.getXPCOMErrorName(status))` into a separate function with a descriptive name, such as `isBlockedError`.",
"explanation": "THE JUSTIFICATION GOES HERE",
},
"raw_hunk": """@@ -481,26 +481,21 @@
}
} catch (err) {
// "cancelledByExtension" doesn't have to be available.
}
- const ignoreList = [
- // This is emitted when the request is already in the cache.
- "NS_ERROR_PARSED_DATA_CACHED",
- // This is emitted when there is some issues around imgages e.g When the img.src
- // links to a non existent url. This is typically shown as a 404 request.
- "NS_IMAGELIB_ERROR_FAILURE",
- // This is emitted when there is a redirect. They are shown as 301 requests.
- "NS_BINDING_REDIRECTED",
+ const blockList = [
+ // When a host is not found (NS_ERROR_UNKNOWN_HOST)
+ "NS_ERROR_UNKNOWN_HOST",
];
// If the request has not failed or is not blocked by a web extension, check for
// any errors not on the ignore list. e.g When a host is not found (NS_ERROR_UNKNOWN_HOST).
if (
blockedReason == 0 &&
!Components.isSuccessCode(status) &&
- !ignoreList.includes(ChromeUtils.getXPCOMErrorName(status))
+ blockList.includes(ChromeUtils.getXPCOMErrorName(status))
) {
blockedReason = ChromeUtils.getXPCOMErrorName(status);
}
return { blockingExtension, blockedReason };""",
},
]
TEMPLATE_PATCH_FROM_HUNK = """diff --git a/{filename} b/{filename}
--- a/{filename}
+++ b/{filename}
{raw_hunk}
"""
class ReviewRequest:
patch_id: str
def __init__(self, patch_id) -> None:
super().__init__()
self.patch_id = patch_id
class Patch(ABC):
def __init__(self, patch_id: str) -> None:
self.patch_id = patch_id
@property
@abstractmethod
def base_commit_hash(self) -> str: ...
@property
@abstractmethod
def raw_diff(self) -> str: ...
@property
@abstractmethod
def date_created(self) -> datetime: ...
@cached_property
def patch_set(self) -> PatchSet:
return PatchSet.from_string(self.raw_diff)
class PhabricatorPatch(Patch):
def __init__(self, patch_id: str) -> None:
super().__init__(patch_id)
self.diff_id = int(patch_id)
@cached_property
def raw_diff(self) -> str:
assert phabricator.PHABRICATOR_API is not None
raw_diff = phabricator.PHABRICATOR_API.load_raw_diff(self.diff_id)
return raw_diff
@staticmethod
def _commit_available(commit_hash: str) -> bool:
r = utils.get_session("hgmo").get(
f"https://hg.mozilla.org/mozilla-unified/json-rev/{commit_hash}",
headers={
"User-Agent": utils.get_user_agent(),
},
)
return r.ok
@cached_property
def _diff_metadata(self) -> dict:
assert phabricator.PHABRICATOR_API is not None
diffs = phabricator.PHABRICATOR_API.search_diffs(diff_id=self.diff_id)
assert len(diffs) == 1
diff = diffs[0]
return diff
@cached_property
def base_commit_hash(self) -> str:
diff = self._diff_metadata
try:
base_commit_hash = diff["refs"]["base"]["identifier"]
if self._commit_available(base_commit_hash):
return base_commit_hash
except KeyError:
pass
end_date = datetime.fromtimestamp(diff["dateCreated"])
start_date = datetime.fromtimestamp(diff["dateCreated"] - 86400)
end_date_str = end_date.strftime("%Y-%m-%d %H:%M:%S")
start_date_str = start_date.strftime("%Y-%m-%d %H:%M:%S")
r = utils.get_session("hgmo").get(
f"https://hg.mozilla.org/mozilla-central/json-pushes?startdate={start_date_str}&enddate={end_date_str}&version=2&tipsonly=1",
headers={
"User-Agent": utils.get_user_agent(),
},
)
pushes = r.json()["pushes"]
closest_push = None
for push_id, push in pushes.items():
if diff["dateCreated"] - push["date"] < 0:
continue
if (
closest_push is None
or diff["dateCreated"] - push["date"]
< diff["dateCreated"] - closest_push["date"]
):
closest_push = push
assert closest_push is not None
return closest_push["changesets"][0]
@property
def date_created(self) -> datetime:
return datetime.fromtimestamp(self._diff_metadata["dateCreated"])
class ReviewData(ABC):
NIT_PATTERN = re.compile(r"[^a-zA-Z0-9]nit[\s:,]", re.IGNORECASE)
@abstractmethod
def get_review_request_by_id(self, review_id: int) -> ReviewRequest:
raise NotImplementedError
@abstractmethod
def get_patch_by_id(self, patch_id: str) -> Patch:
raise NotImplementedError
@abstractmethod
def get_all_inline_comments(
self, comment_filter
) -> Iterable[tuple[int, list[InlineComment]]]:
raise NotImplementedError
def load_raw_diff_by_id(self, diff_id) -> str:
"""Load a patch from local cache if it exists.
If the patch is not in the local cache it will be requested from the
provider and cache it locally.
Args:
diff_id: The ID of the patch.
Returns:
The patch.
"""
try:
with open(f"patches/{diff_id}.patch", "r") as f:
raw_diff = f.read()
except FileNotFoundError:
with open(f"patches/{diff_id}.patch", "w") as f:
patch = self.get_patch_by_id(diff_id)
raw_diff = patch.raw_diff
f.write(raw_diff)
return raw_diff
def get_matching_hunk(
self, patched_file: PatchedFile, comment: InlineComment
) -> Hunk:
def source_end(hunk: Hunk) -> int:
return hunk.source_start + hunk.source_length
def target_end(hunk: Hunk) -> int:
return hunk.target_start + hunk.target_length
if comment.on_removed_code is None:
matching_hunks = [
hunk
for hunk in patched_file
if hunk.target_start <= comment.start_line < target_end(hunk)
or hunk.source_start <= comment.start_line < source_end(hunk)
]
# If there is more than one matching hunk, choose the one where the
# line number of the comment corresponds to an added or deleted line. We
# prioritize added lines over deleted lines because comments are more
# likely to be on added lines than deleted lines.
if len(matching_hunks) > 1:
logger.warning(
"Multiple matching hunks found for comment %s in file %s",
comment.id,
comment.filename,
)
for hunk in matching_hunks:
for line in hunk:
if line.is_added and line.target_line_no == comment.start_line:
return hunk
for line in hunk:
if (
line.is_removed
and line.source_line_no == comment.start_line
):
return hunk
if len(matching_hunks) != 0:
return matching_hunks[0]
elif comment.on_removed_code:
for hunk in patched_file:
if hunk.source_start <= comment.start_line < source_end(hunk):
return hunk
else:
for hunk in patched_file:
if hunk.target_start <= comment.start_line < target_end(hunk):
return hunk
def retrieve_comments_with_hunks(self):
def comment_filter(comment: InlineComment):
# We want to keep all generated comments
if comment.is_generated:
return True
comment_content = comment.content
# Ignore very short and very long comments
if not 50 < len(comment_content) < 500:
return False
# Ignore comments with URLs
if "https://" in comment_content or "http://" in comment_content:
return False
# Ignore nit comments
if self.NIT_PATTERN.search(comment_content):
return False
# Ignore comments with code blocks
if "```" in comment_content:
return False
comment_lower = comment_content.lower()
if any(
phrase in comment_lower
for phrase in [
"wdyt?",
"what do you think?",
"you explain",
"understand",
]
):
return False
return True
for diff_id, comments in self.get_all_inline_comments(comment_filter):
try:
patch_set = PatchSet.from_string(self.load_raw_diff_by_id(diff_id))
except UnidiffParseError:
# TODO: use log instead of print
print(f"Failed to parse {diff_id}")
continue
except ConduitError:
logger.warning("Failed to load %d", diff_id)
continue
file_map = {
patched_file.path: patched_file
for patched_file in patch_set
if patched_file.is_modified_file
}
for comment in comments:
patched_file = file_map.get(comment.filename)
if not patched_file:
continue
hunk = self.get_matching_hunk(patched_file, comment)
if not hunk:
continue
yield hunk, comment
class PhabricatorReviewData(ReviewData):
def __init__(self):
super().__init__()
phabricator.set_api_key(
get_secret("PHABRICATOR_URL"), get_secret("PHABRICATOR_TOKEN")
)
def get_review_request_by_id(self, revision_id: int) -> ReviewRequest:
revisions = phabricator.get(rev_ids=[int(revision_id)])
assert len(revisions) == 1
return ReviewRequest(revisions[0]["fields"]["diffID"])
@tenacity.retry(
stop=tenacity.stop_after_attempt(7),
wait=tenacity.wait_exponential(multiplier=2, min=2),
reraise=True,
)
def get_patch_by_id(self, patch_id: str) -> Patch:
return PhabricatorPatch(patch_id)
def get_all_inline_comments(
self, comment_filter
) -> Iterable[tuple[int, list[InlineComment]]]:
db.download(phabricator.REVISIONS_DB)
revision_count = sum(1 for _ in phabricator.get_revisions())
for revision in tqdm(phabricator.get_revisions(), total=revision_count):
diff_comments: dict[int, list[InlineComment]] = defaultdict(list)
for transaction in revision["transactions"]:
if transaction["type"] != "inline":
continue
# Ignore replies
if transaction["fields"]["replyToCommentPHID"] is not None:
continue
if len(transaction["comments"]) != 1:
# Follow up: https://github.com/mozilla/bugbug/issues/4218
logger.warning(
"Unexpected number of comments in transaction %s",
transaction["id"],
)
continue
transaction_comment = transaction["comments"][0]
comment_content = transaction_comment["content"]["raw"]
is_generated = (
# This includes comments generated by Review Helper, but
# excludes any comments that have been edited by the user.
"> This comment was generated automatically and has been approved by"
in comment_content
)
# Ignore bot comments, except the ones by Review Helper
if (
transaction["authorPHID"] == "PHID-USER-cje4weq32o3xyuegalpj"
and not is_generated
):
continue
comment_id = transaction_comment["id"]
date_created = transaction_comment["dateCreated"]
diff_id = transaction["fields"]["diff"]["id"]
filename = transaction["fields"]["path"]
start_line = transaction["fields"]["line"]
end_line = (
transaction["fields"]["line"] + transaction["fields"]["length"] - 1
)
# Unfortunately, we do not have this information for a limitation
# in Phabricator's API.
on_removed_code = None
# store the last modified date and if the comments have been marked as done
date_modified = transaction_comment["dateModified"]
is_done = transaction["fields"]["isDone"]
# TODO: we could create an extended dataclass for this
# instead of adding optional fields.
comment = InlineComment(
filename=filename,
start_line=start_line,
end_line=end_line,
content=comment_content,
on_removed_code=on_removed_code,
id=comment_id,
date_created=date_created,
date_modified=date_modified,
is_done=is_done,
is_generated=is_generated,
)
if not comment_filter(comment):
continue
diff_comments[diff_id].append(comment)
for diff_id, comments in diff_comments.items():
yield diff_id, comments
class SwarmPatch(Patch):
def __init__(self, patch_id: str, auth: dict) -> None:
super().__init__(patch_id)
self.auth = auth
self.rev_id = int(patch_id)
@cached_property
def _revision_metadata(self) -> dict:
import swarm
revisions = swarm.get(self.auth, rev_ids=[self.rev_id])
assert len(revisions) == 1
return revisions[0]
@property
def raw_diff(self) -> str:
revision = self._revision_metadata
return revision["fields"]["diff"]
@cached_property
def base_commit_hash(self) -> str:
raise NotImplementedError
@property
def date_created(self) -> datetime:
revision = self._revision_metadata
return datetime.fromtimestamp(revision["fields"]["created"])
class SwarmReviewData(ReviewData):
def __init__(self):
self.auth = {
"user": get_secret("SWARM_USER"),
"password": get_secret("SWARM_PASS"),
"port": get_secret("SWARM_PORT"),
"instance": get_secret("SWARM_INSTANCE"),
}
def get_review_request_by_id(self, revision_id: int) -> ReviewRequest:
return ReviewRequest(revision_id)
def get_patch_by_id(self, patch_id: str) -> Patch:
return SwarmPatch(patch_id, self.auth)
def get_all_inline_comments(
self, comment_filter
) -> Iterable[tuple[int, list[InlineComment]]]:
# Todo
raise NotImplementedError
review_data_classes = {
"phabricator": PhabricatorReviewData,
"swarm": SwarmReviewData,
}
def find_comment_scope(file: PatchedFile, line_number: int):
hunks_based_on_added = (
hunk
for hunk in file
if hunk.target_start <= line_number <= hunk.target_start + hunk.target_length
)
hunks_based_on_deleted = (
hunk
for hunk in file
if hunk.source_start <= line_number <= hunk.source_start + hunk.source_length
)
try:
hunk = next(chain(hunks_based_on_added, hunks_based_on_deleted))
except StopIteration as e:
raise HunkNotInPatchError("Line number not found in the patch") from e
has_added_lines = any(line.is_added for line in hunk)
has_deleted_lines = any(line.is_removed for line in hunk)
if has_added_lines and has_deleted_lines:
first_line, last_line = find_mixed_lines_range(hunk)
elif has_added_lines:
first_line, last_line = find_added_lines_range(hunk)
else:
first_line, last_line = find_removed_lines_range(hunk)
return {
"line_start": first_line,
"line_end": last_line,
"has_added_lines": has_added_lines,
}
def find_added_lines_range(hunk: Hunk):
added_lines = [line.target_line_no for line in hunk if line.is_added]
return added_lines[0], added_lines[-1]
def find_removed_lines_range(hunk: Hunk):
removed_lines = [line.source_line_no for line in hunk if line.is_removed]
return removed_lines[0], removed_lines[-1]
def find_mixed_lines_range(hunk: Hunk):
def get_first_line(_hunk: Hunk, default: int | None = None):
for i, line in enumerate(_hunk):
if line.is_context:
continue
if line.target_line_no is None:
if i == 0:
# If this is the first line of the hunk, it
# means that we are adding lines is the first
# line in the file.
return default
return _hunk[i - 1].target_line_no
return line.target_line_no
# This should never happen
raise ValueError("Cannot find the line number")
first_line = get_first_line(hunk, 1)
last_line = get_first_line(list(reversed(hunk)))
if last_line is None:
_, last_line = find_added_lines_range(hunk)
return first_line, last_line
def get_hunk_with_associated_lines(hunk):
hunk_with_lines = ""
for line in hunk:
if line.is_added:
hunk_with_lines += f"{line.target_line_no} + {line.value}"
elif line.is_removed:
hunk_with_lines += f"{line.source_line_no} - {line.value}"
elif line.is_context:
hunk_with_lines += f"{line.target_line_no} {line.value}"
return hunk_with_lines
def format_patch_set(patch_set):
output = ""
for patch in patch_set:
for hunk in patch:
output += f"Filename: {patch.target_file}\n"
output += f"{get_hunk_with_associated_lines(hunk)}\n"
return output
def get_associated_file_to_function(function_name, patch):
for patch_by_file in patch:
for one_patch in patch_by_file:
if function_name in str(one_patch.source):
return patch_by_file.path
return None
def get_associated_file_to_line_context(context_line, patch):
for key, value in patch.items():
if context_line in str(value):
return key
return None
def parse_text_for_dict(text):
file_content = {}
current_filename = None
current_lines = []
lines = text.split("\n")
for line in lines:
if line.startswith("Filename:"):
filename = line.split(":", 1)[1].strip()
# Remove the first letter and the '/' character from the filename
filename = filename[2:]
current_filename = filename
current_lines = []
else:
current_lines.append(line)
# If we have content and filename, store it
if current_filename is not None and len(current_lines) > 0:
if file_content.get(current_filename) is not None:
file_content[current_filename] = (
file_content[current_filename] + "\n" + str(line)
)
else:
file_content[current_filename] = "\n".join(current_lines)
return file_content
def len_common_path(f1, f2):
"""Find length of the common path."""
f1_subsystems = f1.split("/")
if f1 == f2:
return len(f1_subsystems)
f2_subsystems = f2.split("/")
max_common_path_length = next(
idx
for idx, (sub1, sub2) in enumerate(zip(f1_subsystems, f2_subsystems))
if sub1 != sub2
)
return max_common_path_length
def solve_conflict_definitions(target_path, functions):
functions_common_path = [
(len_common_path(target_path, fun.file), fun) for fun in functions
]
max_common_path_length = max(
[common_path_length for (common_path_length, _) in functions_common_path]
)
functions = [
fun
for (common_path_length, fun) in functions_common_path
if common_path_length == max_common_path_length
]
if len(functions) == 1:
return functions
else:
return [] # could not solve conflict
def request_for_function_declarations(
function_search, commit_hash, functions_list, patch_set
):
functions_declarations = []
if functions_list is not None:
for function_name in functions_list:
if (
function_name != "Not found"
and function_name != "N/A"
and function_name != "None"
and function_name != ""
and len(function_name) < 50
):
target_path = get_associated_file_to_line_context(
function_name, parse_text_for_dict(format_patch_set(patch_set))
)
if target_path:
definitions = function_search.get_function_by_name(
commit_hash,
path=target_path,
function_name=function_name,
)
if len(definitions) > 1:
definitions = solve_conflict_definitions(
target_path, definitions
)
collect_function_definitions(
functions_declarations, function_name, definitions
)
return functions_declarations
def is_code_line_already_covered(code_line, target_file, function_declarations):
for function_declaration in function_declarations:
if (
function_declaration[1] == target_file
and code_line in function_declaration[2]
):
return True
return False
def collect_function_definitions(function_declarations, target_element, definitions):
for definition in definitions:
function_declarations.append(
[
target_element,
definition.file,
definition.source,
]
)
def gather_line_context(line_context):
r"""Reformat the line context list and remove duplicates.
Args:
line_context: List of lists, where each list is [line, file, function].
Returns:
List of tuples, where each tuple is (gathered_line, file, function). The
'gathered_line' is a str that concatenates the 'line' with a separator
(i.e., `\n`) that required the same function.
"""
file_dir = {}
for line, file, func in line_context:
if file not in file_dir:
file_dir[file] = {}
if func not in file_dir[file]:
file_dir[file][func] = []
file_dir[file][func].append(line)
gathered_context = []
for file, funcs in file_dir.items():