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

[Bug] The role in parent of java.util.ArrayList is wrongly null #4698

Closed
dhmoclex opened this issue Apr 20, 2022 · 6 comments · Fixed by #4709
Closed

[Bug] The role in parent of java.util.ArrayList is wrongly null #4698

dhmoclex opened this issue Apr 20, 2022 · 6 comments · Fixed by #4709

Comments

@dhmoclex
Copy link
Contributor

dhmoclex commented Apr 20, 2022

Describe the bug
The role in parent of a java.util.ArrayList is null.
Strangely, sometimes, when printed to the console it is "CONTAINED_TYPE" which is correct, but later in my code, or in debug mode, it is wrongly null.

To Reproduce

Any ArrayList in some code may reproduce this bug.

Input:

import java.util.ArrayList;

public class B {

  ArrayList<String> test;

}

Processing with Spoon:

spoon = new Launcher();
spoon.addInputResource(path);
spoon.getEnvironment().setComplianceLevel(11);
spoon.getModel()

Output:
When parsing the model with a custom scanner, calling arrayListTypeReference.getTypeDeclaration().getRoleInParent() returns null instead of CONTAINED_TYPE.

The problem is that the CtClassImpl representing the ArrayList is not the same instance of the one present in the CtPackage java.util.

Replacing the element wise comparison in the CtElementImpl.getRoleInParent() may correct the problem. I'm not sure if this fix is valid or may introduce performance issue.

	@Override
	public CtRole getRoleInParent() {
		if (isParentInitialized()) {
			EarlyTerminatingScanner<CtRole> ets = new EarlyTerminatingScanner<CtRole>() {
				@Override
				public void scan(CtRole role, CtElement element) {
					if (element.equals(CtElementImpl.this)) { // <== instead of element == CtElementImpl.this
						setResult(role);
						terminate();
					}
					//do not call super.scan, because we do not want scan children
				}
			};
			getParent().accept(ets);
			return ets.getResult();
		}
		return null;
	}

Operating system, JDK and Spoon version used

  • OS: Windows 10
  • JDK: 11
  • Spoon version: 10.0.0
@dhmoclex
Copy link
Contributor Author

dhmoclex commented Apr 21, 2022

This pull request break 12 additional tests on my local execution (43 -> 55), but I'm not sure how these tests are related to my fix.

List of new broken tests:

  • SpoonifierTest
    • testSpoonifier()
    • testGeneratedSpoonifierCode()
  • EvalTest
    • testDoNotSimplify()
    • testVisitorPartialEvaluator_if()
    • testTryCatchAndStatement()
    • testStringConcatenation()
    • testScanAPartiallyEvaluatedElement()
    • testDoNotSimplifyCasts()
    • testVisitorPartialEvaluatorScanner()
  • MainTest
    • testTest()
    • testGenericContract()
  • FactoryTest
    • factoryTest()

@algomaster99
Copy link
Contributor

Hi @dhmoclex !

Thank you for the bug report. The type declaration's (instance of CtClass) role should indeed be CONTAINED_TYPE as it belongs to the java.util package. The fix that you have suggested does make sense. Could you report your investigations on why other tests are failing? You can select any number of them. I think it would help us understand better how roles are computed.

@dhmoclex
Copy link
Contributor Author

Hi @algomaster99 !

As said by @slarse on the PR, my correction wasn't sound (#4703). I push another PR (#4703) that add a test covering this bug.

@MartinWitt
Copy link
Collaborator

@dhmoclex the issue should be fixed by #4709.

@I-Al-Istannen
Copy link
Collaborator

@dhmoclex It would be nice if you have a look and verify it works for you as well, and then maybe close your test case PR as I integrated your test case into my PR. That probably wasn't ideal as it doesn't properly give credit, but this issue is referenced at least.

This bug was a bit of a weird one and probably not a great first issue to debug... Your short, sweet and terrifying test case was very useful though :)

@dhmoclex
Copy link
Contributor Author

@I-Al-Istannen
It works perfectly, when the next version gets out I'll test on my project.
Thank you very much for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants