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

fix: fetchin module from the JDT compiler #3549

Merged
merged 4 commits into from
Sep 1, 2020

Conversation

Strum355
Copy link
Contributor

@Strum355 Strum355 commented Aug 25, 2020

Hi, I'm a new code intel SWE at Sourcegraph, and I'll be owning the https://github.com/sourcegraph/lsif-java project for the foreseeable time :)

We came across an issue with Java 9 modules (tracking here), and I discovered a relevant TODO https://github.com/INRIA/spoon/blob/d7da4c6684/src/main/java/spoon/support/compiler/jdt/JDTBatchCompiler.java#L94

The following screenshot gives some insight into the state leading up to the exception, where it seems to extract the wrong module name from the file folder

VSCode Java Debugger state

image

I havent tested this PR in multi-module scenarios, this was the simpler path instead of sorting this.filenames and this.modNames and determining the most recent module name (from the sorted list), given non module-info.java and package-info.java entries in this.filenames have null set for their index in this.modNames.
If this doesn't work as expected in multi-module scenarios (which I will follow up on after this PR is made), this PR can be amended or a new one made for it.

Thanks :)

Note: I'm relying on CI tests to run as I'm unable to run them locally as of yet

@monperrus
Copy link
Collaborator

Could you move the change to .gitignore in a separate chore PR? Thanks!

@monperrus
Copy link
Collaborator

@Strum355 still draft here? Would you be able to add the corresponding test case?

@Strum355 Strum355 marked this pull request as ready for review September 1, 2020 11:43
@monperrus
Copy link
Collaborator

Thanks a lot for the test case!

In Spoon, we try to always have a contract in natural language in each test, see the other // contract: lines in test files.

Could you add this? Then we merge!

@Strum355
Copy link
Contributor Author

Strum355 commented Sep 1, 2020

Added some contracts, let me know if you'd like any amendments to them :)

@monperrus monperrus changed the title Fetching compilation unit module from the JDT compiler fix: fetchin module from the JDT compiler Sep 1, 2020
@monperrus monperrus merged commit 896eeac into INRIA:master Sep 1, 2020
@monperrus
Copy link
Collaborator

Thanks a lot @Strum355

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