Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid reading type info from trees that are not being processed #385

Merged
merged 13 commits into from
Mar 1, 2022

Conversation

zcai1
Copy link

@zcai1 zcai1 commented Feb 12, 2022

Currently, all checkers in inference may crash on the following code:

public class VectorTest {
}

class Vector {
    int x;
    int y;
    int z;

    public static Vector max(Vector a, Vector b) {
        int aLength = a.length();
        int bLength = b.length();

        // test lub of polys
        if (aLength > bLength) {
            return a;
        }
        return b;
    }

    public int length() {
        return x * x + y * y + z * z;
    }
}

This is because AbstractTypeProcessor::typeProcess only receives one fully-analyzed top-level class at a time, while SlotDefaultTypeResolver tries to compute the default types for the entire compilation unit. Details about the crash:

  1. The inference checker (which implements AbstractTypeProcessor) receives a call to type process the top-level class VectorTest.
  2. Although the top-level class Vector is also under the same compilatioon unit tree, some of its fields in the internal JCTree implementation are not yet initialized (they are null).
  3. SlotDefaultTypeResolver receives a call to find the default types for the entire compilation unit tree, and it will visit all type trees in the class Vector.
  4. When SlotDefaultTypeResolver hits int aLength in Vector::max, it tries to get the ATM of int by calling realTypeFactory.getAnnotatedTypeFromTypeTree.
  5. This call will use AnnotatedTypeFactory::type(Tree node), in which we try to fetch the TypeMirror of this primitive type tree.
  6. However, this primitive type tree is not fully initialized and the TypeMirror we get from javac is always null. So the program crashses.

To fix this issue, this PR introduces the following changes:

  1. Added getCurrentTopLevelClass() to InferenceChecker so we can determine whether a tree is outside of the current processing scope.
  2. Use the top level class to refresh default types cache, so SlotDefaultTypeResolver no longer process the entire compilation unit tree.

The test case has already been introduced in PR #381

@zcai1 zcai1 changed the title Avoid annotating or reading type info from trees that are not being processed Avoid reading type info from trees that are not being processed Feb 16, 2022
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've a questions, but overall this looks good.

src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
src/checkers/inference/InferenceAnnotatedTypeFactory.java Outdated Show resolved Hide resolved
@@ -90,6 +92,15 @@ protected Factory createTypeFactory() {
return (Factory)((BaseInferrableChecker)checker).getTypeFactory();
}

@Override
public void visit(TreePath path) {
Copy link
Author

@zcai1 zcai1 Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation from its super class:

Entry point for a type processor: the TreePath leaf is a top-level type tree within root.

And this is why I choose to call slotManager.setTopLevelClass from here.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
@wmdietl wmdietl merged commit d59282a into opprop:master Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants