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.

Sign up for GitHub

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

Jump to bottom

fix(FieldAccessTest): adapt FieldAccessTest to oracle jdk 11/jdt changes #2789

Merged
merged 6 commits into from
Dec 3, 2018
Merged

fix(FieldAccessTest): adapt FieldAccessTest to oracle jdk 11/jdt changes #2789

merged 6 commits into from
Dec 3, 2018

Conversation

nharrand
Copy link
Collaborator

@nharrand nharrand commented Nov 27, 2018 •
edited
Loading

Related to adding a travis build with jdk 11 See #2782

CtFieldWrite.getTarget() on array.length used to return a CtVariableWrite for some reasons... But with jdk 11 now returns a CtVariableRead...

FieldAccessTest#testFieldAccessOnUnknownType() has been changed to handle both... by expecting a CtVariableAccess.

This (combined with #2787) fixes tests for jdk 11 as can be seen here ( https://travis-ci.org/INRIA/spoon/jobs/460289160 ).
Wether this is satisfaying or not, is up to debate. WDYT?

@surli
Copy link
Collaborator

surli commented Nov 27, 2018

So apparently this test is checking the following file: https://github.com/INRIA/spoon/blob/master/src/test/resources/noclasspath/FieldAccessRes.java

But for me the following code looks really wrong:

A [] array = new A[10];
array.length = 5;

since I don't think we are allowed to change the length of an array... IMO it worth it to change the test to do a more classical check, such as:

A a = new A();
a.length = 5;

@nharrand
Copy link
Collaborator Author

For some reason a.length fieldWrite.getTarget() still returns a CtVariableReadImpl and not CtVariableWriteImpl...

@surli
Copy link
Collaborator

surli commented Nov 27, 2018

For some reason a.length fieldWrite.getTarget() still returns a CtVariableReadImpl and not CtVariableWriteImpl...

That's actually interesting. Could you try by upgrading jdt to 3.15.0: this is the last version on Maven and it has been released for Java 11, maybe it will fix it.

@nharrand
Copy link
Collaborator Author

nharrand commented Nov 27, 2018 •
edited
Loading

With:

A a = new A();
a.length = 5;

we have with jdk 11:

CtFieldWriteImpl "a.length"
└─ target CtVariableReadImpl "a"
└─ variable CtFieldReferenceImpl "l"

wheras with jdk 8:

CtFieldWriteImpl "a.length"
└─ target CtVariableWriteImpl "a"
└─ variable CtFieldReferenceImpl "l"

@nharrand
Copy link
Collaborator Author

That's actually interesting. Could you try by upgrading jdt to 3.15.0: this is the last version on Maven and it has been released for Java 11, maybe it will fix it.

Ok, I'm on it

@nharrand
Copy link
Collaborator Author

nharrand commented Nov 27, 2018 •
edited
Loading

Migrating to jdt 3.15.0 does solve the issue. But CompilationUnit.setModule(CompilationUnit module) does not exist anymore, so it breaks JDTBatchCompiler#getCompilationUnits(). Any ideas? @monperrus I see that you wrote the line in question.

nharrand
nharrand commented Nov 27, 2018
View reviewed changes
src/main/java/spoon/support/compiler/jdt/JDTBatchCompiler.java Outdated
@@ -108,7 +108,7 @@ public JDTBatchCompiler(JDTBasedSpoonCompiler jdtCompiler) {
} else {
for (Map.Entry<String, CompilationUnit> entry : pathToModCU.entrySet()) {
if (fileName.startsWith(entry.getKey())) { // associate CUs to module by common prefix
compilationUnit.setModule(entry.getValue());
//compilationUnit.setModule(entry.getValue());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the broken api call.

@monperrus
Copy link
Collaborator

Any ideas? @monperrus I see that you wrote the line in question.

No, sorry 💭

@monperrus monperrus mentioned this pull request Nov 28, 2018
Merged
@pvojtechovsky
Copy link
Collaborator

@nharrand I pushed the potential solution for 3.15.0 compatibility problem

@nharrand nharrand force-pushed the field-access-test-jdk11 branch 2 times, most recently from da0ca50 to f3459cf Compare December 2, 2018 19:30
@nharrand
Copy link
Collaborator Author

nharrand commented Dec 2, 2018

This should work, thanks to @pvojtechovsky !
Ready for review.

@surli surli merged commit ee5167e into INRIA:master Dec 3, 2018
monperrus added a commit that referenced this pull request Dec 4, 2018
@monperrus
community: Nicolas Harrand becomes integrator
620f0a5 
Dear all,

I'd like to nominate Nicolas Harrand (@nharrand) to become an integrator in Spoon. Nicolas has done several key contributions over the past year:

* he has added support for paths in order to uniquely identify AST nodes (#1874), this feature is unique (to my knowledge, it does not exist in any other source code analysis library for Java), and enables original source code analysis over time and versions.
* he has added support for analysis of binary files with decompilation (#2455)
* he has laid down the foundations of a modularized Spoon, which will likely be key in the future
* he has ported Spoon to JDK 11 (#2789). This kind of infrastructure contribution is also very important in the long term.

In addition, Nicolas has proven his ability to communicate constructively and respectfully over issues and pull-requests.

What do you think?
@monperrus monperrus mentioned this pull request Dec 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvojtechovsky pvojtechovsky pvojtechovsky left review comments

@surli surli surli left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nharrand @surli @monperrus @pvojtechovsky