Clarify defaulting of type variable uses

[wmdietl]

No description provided.

[wmdietl]

@Ao-senXiong Can you try to understand why the behavior for the two test cases in the PR changed?
I tried to only clean-up the code, but these two tests changed behavior.
The changes do make sense to me, but they do come as a bit of a surprise.

[wmdietl]

Hmm, also :framework:checkInterning and :dataflow:checkResourceLeak start to fail, so I must have goofed up somewhere...

[wmdietl]

It would also be nice to minimize test cases for these two type systems, to detect these problems earlier.

[Ao-senXiong]

Working on this.

[Ao-senXiong]

@wmdietl Here is the minimal test case to reproduce interning checking failure in this branch:

public class Demo {

    public <T extends Dummy<?,?>> T test(){
        T t = getT();
        return t;
    }

    public <T extends Dummy<?, ?>> T getT(){
        return null;
    }

    public class Dummy<A,B>{

    }
}

java -jar checker/dist/checker.jar -processor org.checkerframework.checker.interning.InterningChecker Demo.java
Demo.java:5: error: [return.type.incompatible] incompatible types in return.
        return t;
               ^
  type of expression: T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @UnknownInterned Void]
  method return type: T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @InternedDistinct Void]

The lower bound of type parameter is not incompatible. If I use the master branch, it is fine.

[Ao-senXiong]

A follow up here, looks like the type refinement works fine, however, the actual type of right hand side is changed.
In master:

InterningVisitor about to test whether actual is a subtype of expected (at Demo.java:4:15): actual tree = METHOD_INVOCATION getT()
     actual: TYPEVAR T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @InternedDistinct Void]
   expected: TYPEVAR T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @UnknownInterned Void]

In this branch

InterningVisitor about to test whether actual is a subtype of expected (at Demo.java:4:15): actual tree = METHOD_INVOCATION getT()
     actual: TYPEVAR T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @UnknownInterned Void]
   expected: TYPEVAR T[ extends @UnknownInterned Demo.@UnknownInterned Dummy<?[ extends @UnknownInterned Object super @InternedDistinct Void], ?[ extends @UnknownInterned Object super @InternedDistinct Void]> super @UnknownInterned Void]

[Ao-senXiong]

This can also be confirmed by this test case works fine in this branch. I will debug and see why the right hand side of return type of method invocation is changed.

public class Demo {

    public <T extends Dummy<?,?>> T test(){
        T t = null;
        return t;
    }

    public class Dummy<A,B>{

    }
}

[wmdietl]

In the previous version, I set this at the outer level, which stored an incorrect value.
The value of GATF#getShouldDefaultTypeVarLocals() changes when determining the LHS type of a local variable.
This change made the all-systems test pass again. The RegexUtil suppression is still necessary.
If no other tests fail, that seems acceptable.

[wmdietl]

./gradlew checkResourceLeak still gives errors like this:

dataflow/src/main/java/org/checkerframework/dataflow/analysis/ForwardAnalysisImpl.java:535: error: [builder:required.method.not.known] The checker cannot determine the must call methods of newThenStore or any of its aliases, so it could not determine if they were called. Typically, this error indicates that you need to write an @MustCall annotation (often on an unconstrained generic type).
                    S newThenStore = mergeStores(s, thenStore, shouldWiden);
                      ^
  The type of object is: S.
  Reason for going out of scope: regular method exit

I tried adding @MustCall to the S type parameter bound, but get the same failures.

@Ao-senXiong Can you see whether your previous test cases now pass and then see what the problem with the Resource Checker is?

[wmdietl]

#755 will look into cleaning up the ugly hack that is needed by the Resource Leak Checker.

[cpovirk]

Update: The problems I had reported with this PR have apparently been fixed by some other commit, whether to here, to main-eisop in the reference-checker repo, or to (for all I know) javac or something. I discovered this while testing the reference checker with 3.42.0-eisop4.

The upgrade to 3.42.0-eisop4 will not require any changes to any of our code checked by the JSpecify reference checker (aside from adding an explicit type argument to one call, which is well below the threshold of worrying about, especially with the type-inference revamp that landed upstream on the horizon).

[cpovirk]

Wait, no, sorry, I confused myself again: The errors are in common.collect, and we don't have nullness checking enabled for that package in our normal builds, so I wasn't seeing them. The errors remain. I'll take things back to jspecify/jspecify-reference-checker#178 (comment).