Fixes #3215 - ClassFileWorkingCopy.getFileName() does not follow API doc

[robstryker]

See #3215

What it does

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[stephan-herrmann]

where do you see a requirement to use a specific separator?

[robstryker]

The documentation for IDependent.getFileName();

In summary, the two examples provided are:

 *  "c:\\lib\\some.jar|/com/p/X.class" or
 *  "/lib/some.zip|/com/q/Y.class".

It uses a system path separator for the resource, followed by the jar separator, followed by the package with forward slashes only for both major operating systems.

/**
 * Answer the file name which defines the type.
 *
 * The path part (optional) must be separated from the actual
 * file proper name by a separator suitable for the type (java.io.File.separator for example),
 * e.g.
 *  "c:\\source\\com\\p\\X.java" or
 *  "/com/p/Y.java".
 *
 * The path to the zip or jar file (optional) must be separated
 * from the actual path part by JAR_FILE_ENTRY_SEPARATOR,
 * e.g.
 *  "c:\\lib\\some.jar|/com/p/X.class" or
 *  "/lib/some.zip|/com/q/Y.class".
 *
 * The proper file name includes the suffix extension (e.g. ".java")
 * e.g. "c:/org/eclipse/jdt/internal/compileri/env/IDependent.java"
 *
 * Return null if no file defines the type.
 */

char[] getFileName();

[stephan-herrmann]

what motivates this change in a PR addressing a problem in ClassFileWorkingCopy?

[robstryker]

The tests continued to fail will the following stacktrace. I realize this will be impossible to actually follow, as you don't have our underlying implementations. I could try to break this part out and come up with a new specific test. But anyway, here goes:

Util.getPackageFragment(char[], int, int) line: 1011	
Util.getClassFile(char[]) line: 810	
Util.getUnresolvedJavaElement(TypeBinding, WorkingCopyOwner, Util$BindingsToNodesMap) line: 1611	
TypeBinding.getUnresolvedJavaElement(TypeBinding) line: 566	
TypeBinding.getUnresolvedJavaElement() line: 557	
TypeBinding.getJavaElement() line: 539	

getPackageFragment(char[] fileName, int pkgEnd, int jarSeparator) appears to be set up to handle inside-jar paths, but, for some reason, it wasn't working for me. In this case, the values passed in were:

fileName=/home/rob/apps/eclipse/workspaces/jclMin1.8.jar|java/lang/RuntimeException.class
pkgEnd=57
jarSeparator=47

The call to PackageFragmentRoot root = (PackageFragmentRoot) JavaCore.create(jarMemento); was returning null when jarMemento has a value of /home/rob/apps/eclipse/workspaces/jclMin1.8.jar

I'm not familiar enough with this code to know why it's doing things this way. It appears to me that the currently implementation simply did not function for this specific case. But I did know i had browsed similar code that did properly turn a path of this structure into a PackageFragmentRoot without error, and so I copied it over, and it worked.

I still maintain that this change to locating the PackageFragmentRoot is at the very least closely related to changing the getFileName() implementation from returning the path of the jar to the full path of the resource inside the jar. If you prefer, I can try to break this specific issue out and try to develop a test case for it.

[robstryker]

The most recent build of this PR still has 2 failures, but I am unable to replicate those failures locally :|

[robstryker]

The two failing tests are not replicatable locally.

Breakpoints set at the following locations:
ClassFileWorkingCopy [line: 88] - getFileName()
CompilationUnit [line: 243] - getFileName()
Util [line: 1007] - getPackageFragment(char[], int, int)

Only the breakpoint at CompilationUnit.getFileName() appears to be hit in these two tests, however, the new implementation appears to return the exact same results as the superclass / previous results. So I'm really not sure what could be happening here.

Any advice is appreciated.

[robstryker]

I really believe this issue is worth looking at, though, because the current code is out of spec with its own documentation. With only two failures (that I can't replicate locally), I think it's unlikely these changes have significantly altered the code or results, especially since the two failing tests don't even seem to enter any of the changes I've made except 1.

[robstryker]

Is anyone else able to reproduce the 3 failures? I can't reproduce them on my local machine.