Crash fix for UnionType parameter in Issue #38

[tahiat]

Issue statement: Casting UnionType parameter as ClassOrInterfaceType to resolve the parameter causes a crash in UnsolvedSymbolVisitor.java.

Current Fix Since both InstantiationException and IllegalAccessException are built in Java language, they should have been resolved successfully instead of throwing an exception. To resolve this I get each element of UnionType and resolve them separately.

Threat
since InstantiationException , IllegalAccessException is built in java, resolve is successful. Custom exception types need to be handled.

[LoiNguyenCS]

I think overall this code is good. But as you say, this current version will only work if both the two types in the union type are solvable, which is not guaranteed in real life. So we should also consider creating synthetic class files if one or both of the types are unsolved.

[LoiNguyenCS]

As you can see in the original codes, the purpose of this try block is to check if a parameter is unsolved. If it is, the parameter will be sent to the catch block where Specimin will create a synthetic class file for it. So I think we also need to do the samething with param. If it is unsolved (that is to say, an exception is thrown when we try to resolve it), param should be sent to the catch block so Specimin can create a synthetic class file for it.

[tahiat]

I have pushed a commit addressing this comment

[tahiat]

package com.example;

class Simple {
    // Target method.
    void bar() {
        try{

        }
        catch (ClassNotFoundException e){

        }
    }
}

As expected, resolving ClassNotFoundException raises exception. However, the classAndPackageMap does not contain ClassNotFoundException as the key.

Adding the import java.lang.ClassNotFoundException resolves this issue. But it is unlikely to to have this import statement in the class file.

I believe this issue will occur for every java.lang types that not imported.

[kelloggm]

package com.example;

class Simple {
    // Target method.
    void bar() {
        try{

        }
        catch (ClassNotFoundException e){

        }
    }
}

As expected, resolving ClassNotFoundException raises exception. However, the classAndPackageMap does not contain ClassNotFoundException as the key.

Adding the import java.lang.ClassNotFoundException resolves this issue. But it is unlikely to to have this import statement in the class file.

I believe this issue will occur for every java.lang types that not imported.

I don't understand the problem that you are raising here. All java.lang types are automatically imported by javac at the start of each file.

Based on @LoiNguyenCS's previous comment, you should write test cases that 1) uses an exception class that's defined as part of the input, and 2) uses an exception class that is unsolvable (i.e., is from a library that would presumably be on the classpath when actually compiling the code). Please re-assign me as a reviewer once you've done this.

[kelloggm]

@tahiat #46 has been merged into this PR's branch, but the tests still fail. Can you investigate (and fix) the failures please?

[tahiat]

Looking into it

[tahiat]

Even if the CustomException class is used as part of the input, the para.resolve() throws exception in visit(Parameter parameter, Void p) method in UnsolvedSymbolVisitor.java . To mitigate this I added a check if the parameter is from Catch clause. If that check is true I used para.getType().resolve() to see if symbol solver can resolve this.

ALSO brought some change from Loi's unmerged PR.

[kelloggm]

ALSO brought some change from Loi's unmerged PR.

When making a comment like this, you should link the specific PR you're referring to rather than being vague. In fact, Loi has multiple open, unmerged PRs, so this is ambiguous (which made this harder to review).

[tahiat]

Few points we have discussed in today's meeting.

(1) Add a test case with arbitrary number of nested blocks inside catch clause and check if that test case completes successfully. --> Added a test case. It's safe to use the para.getParentNode().get() instanceof CatchClause check for resolving the parameter.

(2) When creating a unsolved class for an exception, current implementation check if the reference type is from catch block. Professor, you suggested to check if any parents of this type is Exception type. I can check that way. However, Since the current way works for nested case, i am keeping current way

(3) I observed that if parameter.getType().resolve() is used, it successfully resolve the exception type in catch clause and all test cases passed except the 'typevar-collision' testcase. Since param.getType().resolve() is specifically used to resolve the type of the parameter, It throws exception because there symbol solver can't distinguish between com.T and typed parameter. However in this case, parameter.resolve() successfully resolve that it's a type variable. Below is the input for typevar-collision testcase.

package org;
// This import isn't used.
import com.T;
public class Foo<T> {
    T useT(T t) {
        return t;
    }
}

[tahiat]

It will be good if we merge this PR after PR #57

[kelloggm]

Also, please merge from main so that you incorporate the changes from #57 before you request another review.

[tahiat]

Professor, branch is updated with main

[kelloggm]

This change is harmless, but it wasn't necessary. I'm going to leave it in, but if we were in less of a hurry-up mode right now I'd insist you PR it separately.