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

Introduce ClassHierarchyFactory.makeWithRoot #377

Merged
merged 2 commits into from
Dec 24, 2018

Conversation

reddr
Copy link
Contributor

@reddr reddr commented Dec 11, 2018

Build ClassHierarchy and replace any missing superclass
with the hierarchy root. Adds necessary changes to co-exist
with makeWithPhantom.

Build ClassHiearchy and replace any missing superclass
with the hierarchy root. Adds necessary changes to co-exist
with makeWithPhantom.
@reddr
Copy link
Contributor Author

reddr commented Dec 11, 2018

Some additional note regarding the superclass issue in #335:
I stumbled across the same issue again with makeWithRoot. Apparently, ClassHierarchy.addClass
computes and checks superclasses but does not set the inferred superclass. This is deferred
until the first call of IClass.getSuperClass (for performance reasons?).

There, I explicitly set the superclass to the hierarchy root, but this needs to be done for the PhantomClass mode as well. Probably, this already fixes the issues with makeWithPhantom.

@msridhar
Copy link
Member

Sorry for my slow response here. This change looks great! But I don't see any tests. Can we enhance this test to cover the ROOT case as well?

@reddr
Copy link
Contributor Author

reddr commented Dec 21, 2018

Hmm only the maven build fails for some (unrelated) reason. Any idea?

Any thoughts on my second comment, i.e. why addClass computes the superclass but does not set it?
What is the rationale behind not setting it right away but deferring it to the first call of getSuperClass? Can't be lazy computation as it is already computed in addClass.

To me, this looks like a construct that may easily lead to new issues, e.g. the (incomplete) phantom patch seems to miss setting the superclass there.

@msridhar
Copy link
Member

Hmm only the maven build fails for some (unrelated) reason. Any idea?

I kicked the build again, hopefully it'll pass.

Any thoughts on my second comment, i.e. why addClass computes the superclass but does not set it?
What is the rationale behind not setting it right away but deferring it to the first call of getSuperClass? Can't be lazy computation as it is already computed in addClass.

To me, this looks like a construct that may easily lead to new issues, e.g. the (incomplete) phantom patch seems to miss setting the superclass there.

I don't know immediately the rationale for this, but the IClass interface doesn't have a way to set a superclass; that is delegated to the implementations. So setting it in ClassHierarchy.addClass is not straightforward. We could try updating IClass with a setSuperclass method but that will affect languages beyond Java and may require some careful thought.

@msridhar
Copy link
Member

@reddr these changes look good to me but I'm unsure if this PR is still incomplete due to the addClass() issue. Let me know.

@reddr
Copy link
Contributor Author

reddr commented Dec 23, 2018

@msridhar I think this one is good to go. Was unsure about why computing and setting the superclass is located in different methods and whether there is an easy way to fix this. I still think that this is the problem with the current phantom patch, I left a comment in the code. But that will be a separate PR.

@msridhar
Copy link
Member

@msridhar I think this one is good to go. Was unsure about why computing and setting the superclass is located in different methods and whether there is an easy way to fix this. I still think that this is the problem with the current phantom patch, I left a comment in the code. But that will be a separate PR.

Ok, sounds good. If you have time could you open an issue regarding the specific problem with phantom classes?

@msridhar msridhar merged commit e2025b2 into wala:master Dec 24, 2018
@reddr reddr deleted the feat/cha-with-root branch January 16, 2019 07:24
msridhar added a commit to uber/NullAway that referenced this pull request Sep 30, 2019
Update to the latest WALA version.  Also, switch class hierarchy construction to use `makeWithRoot` which is more fully-baked than what we used before; see wala/WALA#377.
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