Include generated code in determining effective regions. #118

nimakarimipour · 2022-12-20T09:58:58Z

This PR is built on #116

This PR enhances logic for selecting the potentially impacted regions to include generated code by processors (e.g. Lombok).

Trackers are used to detect potentially impacted regions for a fix. These trackers only include the regions where the element was used directly. But since some processors, will copy the annotations on the element over some generated methods, these trackers will miss the real impacted regions because of the copied annotation. This PR tries to address this issue.

Changes in this PR:

Adds GeneratedCodeDetector interface in annotator-scanner module which is responsible for determining if a TreePath is pointing to a generated code.
Adds LombokGeneratedCodeDetector which implements GeneratedCodeDetector and can detect if a path is pointing to a Lombok generated code. For this feature to work properly (lombok.addLombokGeneratedAnnotation = true must be set)
Adds a fifth column in the element use serialization in regions which denotes if the serialized region exists in source code or is generated by a processor. If the region exists in the source code it will have the value "SOURCE", otherwise, it will have the name of the processor that created it.
Adds GeneratedRegionTracker in annotator-core module which is responsible for extending the list of potentially impacted regions to include users of generated code that uses the target element.
Adds LombokTracker which implements GeneratedRegionTracker and can extend regions with other regions that Lombok has created.

To enable handling regions created by Lombok, the flag below must be passed:

-ardl, --activate-region-detection-lombok

…-code

msridhar

I think I get the high-level idea. I have a few comments, mostly around use of enums instead of Strings

msridhar · 2022-12-20T18:51:04Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/Config.java

@@ -141,6 +141,8 @@ public class Config {
   */
  private NullAwayVersionAdapter adapter;

+  public final ImmutableSet<String> generatedCodeDetectors;


Rather than Strings it'd be better to create an enum of supported detectors. You could then create the set using Guava's Sets.immutableEnumSet https://guava.dev/releases/snapshot/api/docs/com/google/common/collect/Sets.html#immutableEnumSet(E,E...)

Thank you for the suggestion, I used enum 4bbd2e4 and f501021

msridhar · 2022-12-20T18:57:04Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayV0Adapter.java

            + values.length);
    String regionMember =
        !Region.getType(values[1]).equals(Region.Type.METHOD) ? "null" : values[1];
-    return new TrackerNode(new Region(values[0], regionMember), values[2], values[3]);
+    return new TrackerNode(new Region(values[0], regionMember, values[4]), values[2], values[3]);


this change relates to changed output from annotator-scanner, correct?

Yes exactly.

msridhar · 2022-12-20T18:58:14Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/Region.java

+   * Type of region. If region exists in source code, this value is {@code "SOURCE"}, otherwise it
+   * will be the name of the processor created this region. (e.g. {"LOMBOK"}).
+   */
+  public final String sourceType;


Again an enum would be better than a String here. Maybe SOURCE should be in the enum I suggested above?

msridhar · 2022-12-20T19:00:56Z

annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/Config.java

+   * @return Name of the generator that produced the code and {@code "SOURCE"} if element exists in
+   *     source code.
+   */
+  String getSourceForSymbolAtPath(TreePath path);


Again use enum for return

msridhar · 2022-12-20T19:02:38Z

...canner/src/main/java/edu/ucr/cs/riple/scanner/generatedcode/LombokGeneratedCodeDetector.java

+public class LombokGeneratedCodeDetector implements GeneratedCodeDetector {
+
+  /** Associated name for Lombok in region serializations in <i>SOURCE_TYPE</i> column. */
+  private static final String NAME = "LOMBOK";


msridhar · 2022-12-20T19:07:25Z

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

+  @Override
+  public Set<Region> extendWithGeneratedRegions(Set<Region> regions) {
+    return regions.stream()
+        // filter regions where are created by lombok


I think the idea here is that for any Lombok-generated method in regions, we also add all uses of that method to the impacted regions. Is this the right idea? If so, let's add some comments / Javadoc documenting the logic here. Also we should state briefly why we think this is sufficient to get all the relevant regions, given the documented assumptions on how Lombok propagates annotations (see previous comment)

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

lazaroclapp

Did a pass. Also, again, I think the description could use a bit of an edit/proofread. It's useful, but there are a few sentences that are harder to parse than they need to be.

lazaroclapp · 2022-12-20T21:32:46Z

annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/Config.java

+   * @return Associated Source type of the generator that produced the code. If element exists in
+   *     source code and not produced by any processor, it will return {@link SourceType#SOURCE}
+   */
+  SourceType getSourceForSymbolAtPath(TreePath path);


Wondering if this should be a method of Config or if it makes more sense to have Config merely return a separate object with this method (SymbolSourceResolver?), just to avoid config doing work not related to parsing configuration options.

...ator-scanner/src/main/java/edu/ucr/cs/riple/scanner/generatedcode/GeneratedCodeDetector.java

lazaroclapp · 2022-12-20T21:40:02Z

...canner/src/main/java/edu/ucr/cs/riple/scanner/generatedcode/LombokGeneratedCodeDetector.java

+        path.getLeaf() instanceof MethodTree
+            ? (MethodTree) path.getLeaf()
+            : ASTHelpers.findEnclosingNode(path, MethodTree.class);
+    if (enclosingMethod == null && path.getLeaf() instanceof MethodTree) {


How is this case not covered by the conditional expression above? Seems like an exact repetition of the first case of the conditional...

5a0eee1

Yes, that it true, thank you for noticing this.

lazaroclapp · 2022-12-20T21:40:31Z

...canner/src/main/java/edu/ucr/cs/riple/scanner/generatedcode/LombokGeneratedCodeDetector.java

+      enclosingMethod = (MethodTree) path.getLeaf();
+    }
+    if (enclosingMethod == null) {
+      return false;


So, this method is incomplete... no? What happens with e.g generated inner classes or generated fields?

I was only focusing on generated methods which can be called outside of the class and extend the potentially impacted regions. But, you are right, I don't see any harm in having a comprehensive check on the generated code of any type. Please see 3621feb. Used the same logic for detecting region member which already covers all cases.

lazaroclapp · 2022-12-20T21:45:05Z

annotator-core/src/test/resources/templates/lombok/build.gradle

+            check("NullAway", CheckSeverity.ERROR)
+            check("AnnotatorScanner", CheckSeverity.ERROR)
+            option("NullAway:AnnotatedPackages", "test")
+            option("NullAway:SerializeFixMetadata", "true")


Note that, internally we also run with the following related configuration flags, not sure if this makes lombok detection harder or easier for the auto-annotator, but worth keeping in mind:

-XepOpt:NullAway:ExcludedClassAnnotations=[...],lombok.Generated -XepOpt:NullAway:ExternalInitAnnotations=[...],lombok.Builder

Thank you for noting this. I will add these flags, in the followup PR for adding more unit tests.

lazaroclapp · 2022-12-20T21:50:08Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/Config.java

+        new Option(
+            "ardl",
+            "activate-region-detection-lombok",
+            false,


Any reason why we want this to be false by default? What's the cost on non-Lombok code? (Btw, I absolutely agree there should be a flag to control this, but maybe this should be on by default and possible to turn off via a flag?)

94f67a7 activated it by default.

lazaroclapp · 2022-12-20T21:52:51Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/CompoundTracker.java

+            new FieldRegionTracker(config, info),
+            methodRegionTracker,
+            new ParameterRegionTracker(tree, methodRegionTracker));
+    Set<GeneratedRegionTracker> generatedRegionTrackers = new HashSet<>();


Why not build with ImmutableSet.builder() rather than hashset and then copyOf?

lazaroclapp · 2022-12-20T21:56:37Z

annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/CompoundTracker.java

+    Set<Region> regions = new HashSet<>();
+    this.trackers.forEach(tracker -> tracker.getRegions(location).ifPresent(regions::addAll));
+    this.generatedRegionsTrackers.forEach(
+        tracker -> regions.addAll(tracker.extendWithGeneratedRegions(regions)));


Minor note here: This is safe as long as there aren't cases where generated code from one processor is derived from generated code from another, or if the GeneratedRegionTrackers are all listed in the same order as that in which processors run (assuming no cycles and no concurrency of annotation processors? Not sure how they actually work here, cc: @msridhar ). Otherwise this would need to be run to a fix-point. That said, I think it's very rare for generated code from one processor to be then fed to another, so I wouldn't worry about handle that case for now...

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

lazaroclapp · 2022-12-20T22:04:18Z

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

+              Optional<Set<Region>> ans = tracker.getRegions(onMethod);
+              return ans.isPresent() ? ans.get().stream() : Stream.of();
+            })
+        .collect(Collectors.toSet());


This works on methods only and one hop, right? I.e. if our normal exploration adds @Nullable to field Foo.bar and Foo is @Data, our initial affected regions (before this fix) will include getBar(). Then this chance makes sure we add all methods/fields/etc that would be affected by getBar() itself automatically becoming @Nullable due to Lombok. Correct?

This might not be enough to track all the changes to fields and methods of the inner class Foo.Builder or their impacted regions if the class is also annotated @Builder. Right?

Happy if the answer is no and we can do that other case as a follow up PR, btw 🙂

I looked at generated code by @Builder and I did not see any generated code getting reused again. But I think as discussed in the follow up PR for more tests, we can also resolve this issue if it comes up 🙂

…atedcode/GeneratedCodeDetector.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

…ackers/generatedcode/LombokTracker.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

nimakarimipour · 2022-12-21T18:53:59Z

@lazaroclapp Thank you for the review. I resolved all your comments and this is ready for another round. Thank you 🙂.

msridhar

Only minor comments this time. Approving assuming they will be addressed before landing

.../main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/GeneratedRegionTracker.java

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

msridhar · 2022-12-21T20:43:18Z

...-core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/LombokTracker.java

+        // add potentially impacted regions for the collected methods.
+        .flatMap(
+            onMethod -> {
+              Optional<Set<Region>> ans = tracker.getRegions(onMethod);


Not related to this PR, but it seems strange that the getRegions() method returns an Optional<Set<Region>> rather than just a Set<Region> that could be empty. Maybe fix in a follow up

@msridhar I wanted to distinguish between trackers "not having any knowledge about the passed element" and "having data but not having any actual usage". But this was required in the old designs and should be updated in follow up PRs.

msridhar · 2022-12-21T20:45:37Z

annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/SymbolUtil.java

+  @Nullable
+  public static Symbol locateRegionMemberForSymbolAtPath(
+      TreePath path, Symbol.ClassSymbol enclosingClass) {
+    // Class is not generated by lombok. Let's find the region member, can be a field, method or


Why is there a comment about lombok here? Feels somewhat unrelated to what this method is doing

Sorry thanks for noticing this. This was copied from existing method. 3738766

msridhar · 2022-12-21T20:47:07Z

...canner/src/main/java/edu/ucr/cs/riple/scanner/generatedcode/LombokGeneratedCodeDetector.java

+public class LombokGeneratedCodeDetector implements GeneratedCodeDetector {
+
+  /** Associated source type for Lombok in region serializations in <i>SOURCE_TYPE</i> column. */
+  private static final SourceType sourceType = SourceType.LOMBOK;


I don't think we need this constant, since it's only used once. Just return SourceType.LOMBOK from the method

…ackers/generatedcode/LombokTracker.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>

…cation

annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/ErrorProneCLIFlagsConfig.java

lazaroclapp · 2022-12-21T22:02:53Z

.../main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/GeneratedRegionTracker.java

+   * impacted as well.
+   *
+   * @param regions Set of potentially impacted regions.
+   * @return Set of potentially impacted regions including all knowing potentially impacted


Suggested change

* @return Set of potentially impacted regions including all knowing potentially impacted

* @return Set of potentially impacted regions including all known potentially impacted

But also this is both hard to parse and - I think - inaccurate.

You are not extending the list exclusively with generated regions, but, rather whenever a region in the initial set (a region affected by a fix) is a generated region, we are also adding every region that would be impacted by a fix within the generated region, since the annotation processor might propagate changes from the original fix to this generated region independent of the annotator's own logic. Is this correct? Is there a clearer way to explain this? (cc: @msridhar )

…ProneCLIFlagsConfig.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

…ackers/generatedcode/GeneratedRegionTracker.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

.../main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/GeneratedRegionTracker.java

…ackers/generatedcode/GeneratedRegionTracker.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>

nimakarimipour added 10 commits December 19, 2022 21:38

update tracker node construction

21ebc38

initial impl

7f8bc9f

update impl

9b60f7d

finish impl of generated code

2931ca2

extract config scanner serializer

63995e0

merge with nimak/extract-scanner-conf-writer

94a65eb

update on file creation

8cb2b55

Merge branch 'nimak/extract-scanner-conf-writer' into nimak/generated…

b10391a

…-code

update on configuration

25dd946

add flag to config

f73165b

nimakarimipour added the enhancement New feature or request label Dec 20, 2022

nimakarimipour requested review from msridhar and lazaroclapp December 20, 2022 09:58

nimakarimipour self-assigned this Dec 20, 2022

nimakarimipour added 3 commits December 20, 2022 10:35

merged master

ce1ff41

Merge branch 'master' into nimak/refactor-tracker-node

fedbecf

Merge branch 'nimak/refactor-tracker-node' into nimak/generated-code

7ab915a

msridhar reviewed Dec 20, 2022

View reviewed changes

nimakarimipour added 5 commits December 20, 2022 13:13

use enums

4bbd2e4

add missing javadoc

f501021

update javadoc

08ab792

add copyright

88a7fa8

add javadoc

f0880e2

lazaroclapp reviewed Dec 20, 2022

View reviewed changes

nimakarimipour and others added 6 commits December 20, 2022 16:54

extract SymbolSiurceResolver from config

10559d1

Update annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/gener…

bb60003

…atedcode/GeneratedCodeDetector.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

reformat

46547ab

simplification

5a0eee1

add coverage for generated classes

fea3fcf

updated region detection

3621feb

nimakarimipour and others added 4 commits December 21, 2022 10:07

activate lombok region detection by default

94f67a7

use builder

d0f30c6

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

3f27354

…ackers/generatedcode/LombokTracker.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

bug fix

2b34612

nimakarimipour requested review from lazaroclapp and msridhar December 21, 2022 18:56

Base automatically changed from nimak/refactor-tracker-node to master December 21, 2022 19:34

nimakarimipour added 2 commits December 21, 2022 11:36

merged master

c9406cf

updated header

235e0a6

msridhar approved these changes Dec 21, 2022

View reviewed changes

nimakarimipour and others added 6 commits December 21, 2022 12:54

update scanner config construction in core

1507ddb

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

303eed5

…ackers/generatedcode/LombokTracker.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>

reformat

0a0eddc

uodate javadoc

d97d3c4

updated locateRegionMemberForSymbolAtPath comments and minor simplifi…

3738766

…cation

remove constant

71cc443

lazaroclapp reviewed Dec 21, 2022

View reviewed changes

nimakarimipour and others added 3 commits December 21, 2022 14:55

Update annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/Error…

5c4ab08

…ProneCLIFlagsConfig.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

c18c576

…ackers/generatedcode/GeneratedRegionTracker.java Co-authored-by: Lázaro Clapp <lazaro@uber.com>

apply suggestion

7d3a20f

msridhar approved these changes Dec 21, 2022

View reviewed changes

.../main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/GeneratedRegionTracker.java Outdated Show resolved Hide resolved

.../main/java/edu/ucr/cs/riple/core/metadata/trackers/generatedcode/GeneratedRegionTracker.java Outdated Show resolved Hide resolved

nimakarimipour and others added 2 commits December 21, 2022 17:20

Update annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/tr…

59f9829

…ackers/generatedcode/GeneratedRegionTracker.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>

update regions

01a7bf5

nimakarimipour merged commit 3dfa1dc into master Dec 22, 2022

nimakarimipour deleted the nimak/generated-code branch December 22, 2022 01:48

nimakarimipour mentioned this pull request Dec 23, 2022

Remove redundant conflicts from Lombok generated code. #119

Open

	* @return Set of potentially impacted regions including all knowing potentially impacted
	* @return Set of potentially impacted regions including all known potentially impacted

Include generated code in determining effective regions. #118

Include generated code in determining effective regions. #118

Conversation

nimakarimipour commented Dec 20, 2022 • edited

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Dec 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Dec 21, 2022

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Dec 20, 2022 •

edited

nimakarimipour Dec 20, 2022 •

edited