Fix field access related CI and other problems

[maximilianruesch]

Fixes several non-critical problems related to field access information:

1. Fixes the Integration Test CI currently failing as not enough field access information analyses are scheduled
2. Fixes formatting of some code
3. Removes superfluous MethodField[...]AccessInformation objects from the property store if no information is contained in them

During research for this pull request, it was noticed that PointsToAnalysisState.dependees might be unused. We may have to investigate this.

[errt]

Any idea where the broken tests come from?

[maximilianruesch]

Nope not yet

[maximilianruesch]

Is there no keyword with which i can execute the integration tests in my PR? I thought from reading the run_it_test_on_comment.yml workflow it worked something like this...

[errt]

Good question. I thought it was run, but it seems it was scheduled but then skipped. I have no idea why. Maybe try again?

[maximilianruesch]

/it:test

[errt]

/it:test

[errt]

Still had one failing it:test. Any idea why? It has one field fewer than before, that is odd.

Note that both the Assignability and FieldImmutability of that field are gone: boolean[] access // in scala.tools.asm.tree.analysis.Subroutine => EffectivelyNonAssignable boolean[] access // in scala.tools.asm.tree.analysis.Subroutine => NonTransitivelyImmutableField

[maximilianruesch]

I have no real idea yet, but the old value seems odd too, given that the field access (from the decompiled class file in the tested jar) seems pretty assignable to me:

[maximilianruesch]

I found the issue: The field access is now corrrectly designated as Assignable, but this is filtered out (from the file and checked EPKs) as the test thinks it was created with the fallback value.

Thus the disappearing access is actually correct, as it follows form the field now having the correct assignability value.

[errt]

No, that doesn't look assignable to me. First one is in the constructor, second one is a clone-like method which our assignability analysis is specifically designed to recognize. The arraycopy and the last method only change the array's contents, not the field itself (which should be NonTransitivelyImmutable then). So the question is why this is not still recognized as non-assignable.

[maximilianruesch]

I narrowed down the issue some more and still think the previous value (or at least how it was obtained) was faulty.

A difference between the implementation [1] (before the migration to field accesses) and the one after [2] is observed when checking non-dominated field reads in the method to be investigated if it updates a field. The implementation is as follows:

[1]:

https://github.com/opalj/opal/blob/5fd8e4b22415aac5f16115fe37902fc4c9fb4e79/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala#L179-L185

[2]:

https://github.com/opalj/opal/blob/d0fcdc99d2e7aec60941af606828215fe41fd29a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala#L250-L266

The crucial difference is that [2] correctly checks if more than one read exists in the method, while [1] only effectively checks if reads are in that field (as its read-pcs are grouped by method). The lines affected:

[1]:

https://github.com/opalj/opal/blob/5fd8e4b22415aac5f16115fe37902fc4c9fb4e79/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala#L180

[2]:

https://github.com/opalj/opal/blob/d0fcdc99d2e7aec60941af606828215fe41fd29a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala#L251-L253

The check fieldReadsInMethod.size > 1 from [1] never triggers in the entire package (analysing the scala/tools/asm only to make testing times bearable). Changing it to fieldReadsInMethod.nonEmpty && fieldReadsInMethod.head.size > 1 as a correction to the imo intended check directly shows the field in question as assignable.

I think this dominance check for field reads is broken in general and needs to be investigated on its own. The only reason it worked until the migration is that the precondition above was faulty itself.

[errt]

Yes, I remember fixing (or at least improving) that check. But regardless of the actual implementation, just from looking at the code you showed, that field looks non-assignable to me. Or what am I missing?

[maximilianruesch]

As mentioned:

I think the previous value (or at least how it was obtained) was faulty.

While it may be that the previous value was correct, it was not obtained correctly if a check doesn't really make sense imo. Is there an test case asserting that field read dominance is computed correctly? What i mean is: While we will want to fix this, the issue is likely not to be connected to faulty field access information, but to faulty computation of read dominance.

If you know at which point exactly the "copy" methods like in this case should be discovered and handled separately, please show me. I can only guess, currently i think its connected with the read dominance i linked.

[errt]

I fully agree.

That should go via https://github.com/opalj/opal/blob/d0fcdc99d2e7aec60941af606828215fe41fd29a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala#L122

and thus

https://github.com/opalj/opal/blob/bcac33ba39d16ae36ea7f8b94aca3906b6af45e5/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/AbstractFieldAssignabilityAnalysis.scala#L213-L237

[maximilianruesch]

I will have a look later, thanks 👍

[maximilianruesch]

In your sent code links, it the cloning pattern is (or should be) detected. As i see it, for a clone like method, the receiver reference should not escape (except via a return), so this referenceHasNotEscaped exactly captures this and returns true for such methods.

This then correctly leads to methodUpdatesField skipping the "early return" with true since in our sense it is not yet clear if the method updates the field. This then leads to write dominance checking, queue problems described in my comment here, so i conclude that the problem with the clone detection must be somewhere else than the link you showed (imo it works correctly).

[maximilianruesch]

I further confirmed this by adding a test case myself:

public class CloneMethod {
    boolean[] access;

    private CloneMethod() {}

    public CloneMethod copy() {
        CloneMethod result = new CloneMethod();
        result.access = new boolean[this.access.length];
        System.arraycopy(this.access, 0, result.access, 0, this.access.length);
        return result;
    }
}

In this class, access is marked as Assignable, whereas when removing the System.arraycopy it is marked as EffectivelyNonAssignable.

If the clone detection you linked above works as intended, then imo this finding hints at broken read dominance again (solely by some "read" being involved).

[maximilianruesch]

/it:test

[maximilianruesch]

/it:test

[maximilianruesch]

/it:test

[maximilianruesch]

/it:test

[errt]

Did you check that the remaining changes in the integration test are valid?

[maximilianruesch]

I have found the following differences since the last refresh:

private javafx.geometry.Rectangle2D bounds // in javafx.stage.Screen => EffectivelyNonAssignable
private javafx.geometry.Rectangle2D visualBounds // in javafx.stage.Screen => EffectivelyNonAssignable
[...]
private javafx.geometry.Rectangle2D bounds // in javafx.stage.Screen => NonTransitivelyImmutableField
private javafx.geometry.Rectangle2D visualBounds // in javafx.stage.Screen => NonTransitivelyImmutableField
[...]
ObjectType(javafx/stage/Screen) => NonTransitivelyImmutableClass
[...]
ObjectType(javafx/stage/Screen) => NonTransitivelyImmutableType

When taking a look at the javafx/stage/Screen class, we see that each non-static field is only written in the constructor and the clone-like method. So imo its all fine.