Generic semantic problem in Symboltype.isCompatible

[cal101]

    @Test
    public void testTargetInferenceShouldFail() throws Exception {

        compile("import java.util.Collections; "
                + "import java.util.List; "
                + "public class A { public static <T> void processStringList(T[] args) {}}");
        SymbolType st = new SymbolType(getClassLoader().loadClass("A"));
        SymbolTable symTable = getSymbolTable();
        symTable.pushScope();
        symTable.pushSymbol("A", ReferenceType.TYPE, st, null);

        try {
            MethodCallExpr expr = (MethodCallExpr) ASTManager.parse(
                    Expression.class,
                    "A.processStringList(Collections.emptyList());");
            HashMap<String, Object> ctx = new HashMap<String, Object>();
            expressionAnalyzer.visit(expr, ctx);
            fail("should not be reached: type=" + expr.getSymbolData().getMethod());
        } catch (NoSuchExpressionTypeException e) {
            Assert.assertTrue(e.getMessage(), e.getMessage().contains("Ops! The method call A.processStringList(Collections.emptyList()) is not resolved"));
        }
    }

The test above should pass but insteads fails with:

java.lang.AssertionError: should not be reached: type=public static void A.processStringList(java.lang.Object[])

The reason is that Symboltype.isCompatible does not execute semantic checks in this and other cases. There is some simple logical bug in the outmost "if". The type variable will be set but the collection will always be empty. Please take a look!

[rpau]

Ok, I understand the bug: The argument is a list and the parameter is an array. However, changing the condition other tests fail. I am looking to them.

[cal101]

IMHO this case is just a tip of the iceberg. I don't understand if you still working on the main problem. Sure other tests fail if the condition is enabled because IMHO the tests now test something. From a short look it seems that a few more negative test cases would helpful in addition.

[octopatch]

Yes and No. Take in mind that javalang-compiler does not verify that code compiles. It only resolves symbols and their types.

El 23/3/2017 20:06, "cal" notifications@github.com escribió:

IMHO this case is just a tip of the iceberg. I don't understand if you still working on the main problem. Sure other tests fail if the condition is enabled because IMHO the tests now test something. From a short look it seems that a few more negative test cases would helpful in addition.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rpau/javalang-compiler/issues/30#issuecomment-288828429, or mute the thread https://github.com/notifications/unsubscribe-auth/AJo3Qx1CY-x6YXDu9nIutY6SLFVJ8RbCks5rosKZgaJpZM4MhyPQ .

[cal101]

I take a look again when i get some time. As said i just took a short look and may got a wrong impression.

[cal101]

Please see https://github.com/rpau/javalang-compiler/issues/31

[rpau]

Cal, I am taking a look fondly. I have understood what you mean and I am trying to refactor the method to see more clearly what is happening.

[rpau]

Cal, I have just refactorized isCompatible() in 0b69f6a51df4d9599aa88df8b96515c24b2636e5

I do not understand clearly why testMethodReferencesToConstructors2ShouldFailForNonMatchingTypeParameter should fail. It is because HashSet::new returns a HashSet of Object instead of Integer?

Sorry, but I do not use method references expressions very often. However, take in mind that a bug should be where there are two valid methods and the semantic analysis chooses the incorrect one. The system always assumes that code compiles because it avoids extra validations.

In this test case, the system is resolving that SOURCE is a Collection and T is Integer due to there is one method that matches and the non-method references matches. Afterwards, resolves which could be the method call for the method reference expression.

[cal101]

The system always assumes that code compiles because it avoids extra validations.

That changes my picture, because I thought it should detect non-compilable stuff, too. I have to rethink #31 in light of this. The error case I defined WON'T compile so it is not a suitable test case as I understand now. Have to catch a train now...