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

JDime crashes without output #26

Open
mernst opened this issue Oct 24, 2023 · 4 comments
Open

JDime crashes without output #26

mernst opened this issue Oct 24, 2023 · 4 comments

Comments

@mernst
Copy link

mernst commented Oct 24, 2023

I ran this command:

$JDIME_DIR/build/install/JDime/bin/JDime -r --accept-non-java --mode linebased,semistructured,structured \
--output checker-framework-fork-smillst-branch-java8inference-7b64ac8a86a828195dd8438b698bdff955e7d0dd-jdime-merged \
checker-framework-fork-smillst-branch-java8inference-9be94d8675 \
checker-framework-fork-smillst-branch-java8inference-c237f52596524124e44ca79dcb6c3a9fb4656e11 \
checker-framework-fork-smillst-branch-java8inference-93f64d9f29

The output is in the attached file output.txt.
output.txt

You can obtain the input files from http://homes.cs.washington.edu/~mernst/tmp33333/jdime-bug-report.zip (warning: big file).

@GSeibt
Copy link
Collaborator

GSeibt commented Oct 26, 2023

That's expected. --accept-non-java will cause JDime to attempt to parse all files (even if they don't end in .java). From your output it appears that parsing then fails for some Bash script. You should omit the option, JDime will then only consider .java files. Additionally, you should make sure to set your working directory to the dir containing the JDime script. This will ensure that JDime.properties and the logging config file are taken into account.

Doing that we get a bit further. Merging your directories fails for checker-framework-fork-smillst-branch-java8inference-9be94d8675/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java in Line 941.

940   @Nullable DeclaredType typeToCheck(
941       @UnknownInitialization(BaseTypeVisitor.class) InterningVisitor this) {

Hard to read but the parameter appears to be named "this"? This is invalid afaik and leads to JDime refusing to parse the file. I remember we implemented something to skip scenarios for which errors like this occur. I'll check whether we can use that here.

@mernst
Copy link
Author

mernst commented Oct 26, 2023

Thank you for your response.

--accept-non-java will cause JDime to attempt to parse all files (even if they don't end in .java).

That is my goal. Do you mean JDime parses non-.java files as Java? That is not documented. Why would one want to do that?

You should omit the option, JDime will then only consider .java files.

Is JDime able to do a full merge, or do I need to use some other mechanism to merge non-Java files, in addition to JDime?

Hard to read but the parameter appears to be named "this"? This is invalid afaik

Using this as the first formal parameter of an instance method has been legal Java since Java 8 (March 2014). Does JDime only work for Java 7 and earlier?

@GSeibt
Copy link
Collaborator

GSeibt commented Oct 27, 2023

Yea, documentation could be improved. JDime can (in anything other than linebased mode) only handle Java files. Therefore, it refuses to merge files not ending in .java (or ignores these when in recursive mode). The --accept-non-java option is for cases in which Java files are passed without the .java suffix, i.e., cases in which you want to disable this sanity check. Most notably, this is the case when using JDime as a git merge driver where the versions to be merged are passed with names generated by git. In summary, when using --accept-non-java, you have to make sure that you are only passing Java files to JDime (unless you are exclusively in linebased mode).

As for the this question, I actually didn't know this was legal. You learn something new every time you look at Java :) JDime uses ExtendJ for generating ASTs. ExtendJ has always claimed Java 8 support but in the end it's not javac, there are edge-cases where it's not compliant. I guess this is one of them. We haven't updated ExtendJ (or JDime significantly) in a good while, maybe this is fixed. Updating ExtendJ is always a difficult thing because the structure of the AST tends to change.

And P.S.: A note on the recursive mode: I personally don't use it for studies, I use JDime as a git merge driver. (I.e., not a lot of bug-fixing has gone into it). The recursive mode may look innocent but what's actually happening there is that JDime considers the files/directories as a tree and applies the same algorithm used for merging ASTs to find triples (or pairs in two-way mode) of files to be merged. Once these merge scenarios are found, the files are parsed and the algorithm is applied to the ASTs.

@mernst
Copy link
Author

mernst commented Oct 27, 2023

Thank you for your helpful responses. All of this would be great to document, to prevent misunderstandings like mine in the future.

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

No branches or pull requests

2 participants