Variable definition distorted

[khaes-kth]

When 2142 fixes are applied on this (commit: 9eab3f69a4a9125b9bf9c9fd77949903f94a67f9), the following change is made:

-  private final SourcePartitionValidator.MatchingStrategy validationStrategy;
+  private finalcom.salesforce.mirus.SourcePartitionValidator.MatchingStrategy validationStrategy;

[khaes-kth]

A similar issue when 2272 fixes applied on this (commit: 24d4f4b83a7d2e14f16419fbf7ce05cb019c8066)

@@ -106,7 +112,7 @@ public class RoadSegment extends DefaultWeightedEdge implements Iterable<Vehicle
 
     private final int laneCount;
 
-    private final LaneSegment laneSegments[];
+    private final LaneSegment laneSegments;
 
     // TODO extend Node idea to keep information of connecting roadSegments
     private int sizeSourceRoadSegments = -1;

Related to INRIA/spoon#4315.

[algomaster99]

@khaes-kth This may be fixed by INRIA/spoon#4279.

[khaes-kth]

Great! Is it possible to add a test in Sorald that checks if it is fixed or not?

[algomaster99]

Yes, that is possible. Let the linked PR get merged and then I will submit a PR with the test for it. Meanwhile, I can test with my local build of spoon.

[khaes-kth]

Awesome. Looking forward to it.

[algomaster99]

Apparently, this is not so straightforward 😞. This won't be fixed by INRIA/spoon#4279.

The fragment SourcePartitionValidator.MatchingStrategy|2817, 2858|SourcePartitionValidator.MatchingStrategy| is an instance of TokenSourceFragment instead of ElementSourceFragment. Another anomaly is that its role is IDENTIFIER instead of TYPE which is wrong. com.salesforce.mirus.SourcePartitionValidator.MatchingStrategy is clearly the `type of the field and not an identifier.

Another thing I noted is that when you pass /src/main/java/com/salesforce/mirus/KafkaMonitor.java as the source to sorald, there is no distortion produced in the field.

[khaes-kth]

Hmm.
Can you send a link to this fragment in the repository? SourcePartitionValidator.MatchingStrategy|2817, 2858|SourcePartitionValidator.MatchingStrategy|

[algomaster99]

@khaes-kth Here it is - https://github.com/salesforce/mirus/blob/9eab3f69a4a9125b9bf9c9fd77949903f94a67f9/src/main/java/com/salesforce/mirus/KafkaMonitor.java#L62.

[algomaster99]

I found the problem. The visitor defined here is not creating the tree correctly. It somehow skips setting the sibling of final to com.salesforce.mirus.SourcePartitionValidator.MatchingStrategy. I am not sure why this is happening. I am still looking into it.

[algomaster99]

@khaes-kth I found the root cause of the bug. Somehow, the spoon visitor, or something else, is converting SourcePartitionValidator.MatchingStrategy to its fully-qualified name com.salesforce.mirus.SourcePartitionValidator.MatchingStrategy. And when it tries to append this to the tree, it fails because source position of the latter is unknown.

Somehow, everything works in order when only KafkaMonitor.java is repaired.

[algomaster99]

@slarse I needed some help here because I think the auto-importer preprocessor is causing this bug. I am drawing upon this conclusion because you said something along those lines here and I feel you would have an idea.

Anyway, the type of the field, which is SourcePartitionValidator.MatchingStrategy, is not being appended to the source fragment tree. I suspect that the auto-importer is converting this type to its fully-qualified name. Further, when addChild tries to find its source position, it returns null. Thus, the type is skipped and we don't have this element in the source fragment tree.

[slarse]

@algomaster99 The auto-importer used in diffmin is not exacty the same as is used here (diffmin uses stock Spoon, sorald has the SelectiveForceIMport thingy).

Anyway, it's pretty easy to validate your theory, just disable the auto-importer and see if it solves the problem :)

To do that, remove the preprocessors that get added when creating the sniper printer here in Sorald. I forget exactly where that is but I'm sure you can find it.

[algomaster99]

The problem is with the source position only. Consider the following two fields in KafkaMonitor.java:62-63.

10 = {CtFieldImpl@5731} "private final com.salesforce.mirus.SourcePartitionValidator.MatchingStrategy validationStrategy;"
 type = {CtTypeReferenceImpl@5752} "com.salesforce.mirus.SourcePartitionValidator.MatchingStrategy"
  position = {NoSourcePosition@5621} "(unknown file)"

11 = {CtFieldImpl@5732} "private final com.salesforce.mirus.metrics.MissingPartitionsJmxReporter missingPartsJmxReporter = new com.salesforce.mirus.metrics.MissingPartitionsJmxReporter();"
 type = {CtTypeReferenceImpl@5760} "com.salesforce.mirus.metrics.MissingPartitionsJmxReporter"
  position = {SourcePositionImpl@5772} "(/home/assert/Desktop/testrepos/mirus/src/main/java/com/salesforce/mirus/KafkaMonitor.java:63)"

We need to fix spoon's code to set the source position of this field's type. Once we're through that, I am quite confident about fixing this bug.

[algomaster99]

Okay, so I fixed this, but I am unsure if I have done it correctly. This is the patch (in the spoon repository):

diff --git a/src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java b/src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java
index e86605ae..51c4e824 100644
--- a/src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java
+++ b/src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java
@@ -154,6 +154,7 @@ public class ReferenceBuilder {
                                }
                        });
                        if (ref != null) {
+                               ref.setPosition(accessedType.getPosition());
                                accessedType = ref;
                        }
                }

Link to file in GitHub repository.

The premise of this patch is the above comment - #603 (comment) Once I identified the element which doesn't have a type, I just set its position. But I don't understand some things about ReferenceBuilder.java here. I will open a draft PR in spoon and articulate my questions there because that discussion won't be directly related to sorald.

[khaes-kth]

Great. Do you have also opened a PR in spoon with this patch?

BTW, I think the best way to show this is actually working is to add a test in Sorald that checks it.

[algomaster99]

Do you have also opened a PR in spoon with this patch?

I will open it by today for sure. Not sure about what exact questions I have to ask so it will take time.

BTW, I think the best way to show this is actually working is to add a test in Sorald that checks it.

I can only do that once we my patch gets merged and we start using that SNAPSHOT version of spoon. For now, I can show you the demo. I will come to your office.

[monperrus]

BTW, I think the best way to show this is actually working is to add a test in Sorald that checks it.

yes, agree

we start using that SNAPSHOT version of spoon.

looking forward to it!

[algomaster99]

BTW, I think the best way to show this is actually working is to add a test in Sorald that checks it.

I think we can skip writing the tests here in Sorald because this issue was the fault of the sniper printer and not Sorald. Therefore, the correct place of the testing this bug is spoon and it's there.

We can close this issue once we upgrade Sorald to use the latest snapshot version of Spoon. Let's just close these when the PR related to these are merged into spoon/master.

[algomaster99]

Fixed in INRIA/spoon@454dada.

[algomaster99]

@khaes-kth

-    private final LaneSegment laneSegments[];
+    private final LaneSegment laneSegments;

We accidentally closed this issue because the above is still not fixed. This is also an issue in Spoon. INRIA/spoon#4315

[khaes-kth]

Thanks for reopening it.

[khaes-kth]

Related to INRIA/spoon#4315

[algomaster99]

• private final LaneSegment laneSegments[];

• private final LaneSegment laneSegments;

Fixed in INRIA/spoon#4341.