fix silent NumberFormatException in SourceMapper #1834

[jukzi]

#1834

[jukzi]

List of tests where the change is executed:

 org.eclipse.jdt.core.tests.dom.ASTModelBridgeTests.testBinaryMemberTypeFromAnonymousClassFile1
 org.eclipse.jdt.core.tests.dom.ASTModelBridgeTests.testBinaryMemberTypeFromAnonymousClassFile2
 org.eclipse.jdt.core.tests.model.AttachSourceTests.testInnerClass2
 org.eclipse.jdt.core.tests.model.AttachSourceTests.testInnerClass3
 org.eclipse.jdt.core.tests.model.AttachSourceTests.testInnerClass4
 org.eclipse.jdt.core.tests.model.AttachSourceTests.testInnerClass7
 org.eclipse.jdt.core.tests.model.JavaSearchTests.testLocalVariableReference3
 org.eclipse.jdt.core.tests.model.ResolveTests.testLocalNameForClassFile
 org.eclipse.jdt.core.tests.model.ResolveTests.testMethodDeclarationInAnonymous4
 org.eclipse.jdt.core.tests.model.TypeResolveTests.testResolveTypeInBinary3

[stephan-herrmann]

Number-parsing part of the name of a classfile looks wrong, indeed, but let's dig a bit deeper:

• while it's true that I made changes in this area via https://bugs.eclipse.org/521058 this didn't touch the case of anonymous classes, but only module-info.class
• the block in question ..
  • was introduced via https://bugs.eclipse.org/3208 (the oldest JDT bug I ever looked at :) )
  • got its handling for anonymous classes via https://bugs.eclipse.org/75885
• the same https://bugs.eclipse.org/75885 also made SourceMapper.IType getType(String) fit for anonymous classes
• given that the block in mapSource(NamedMember,char[],IBinaryType,IJavaElement) has been wrong for more than 20 years, what purpose would it serve anyway?
• I couldn't find any test failing when I simply remove the block if (isAnonymousClass) 😄

So I suggest not to fix unnecessary code, but let's give @jarthana also a chance to chime in, if he has an idea what this code might be needed for.

[stephan-herrmann]

One more observation: with or without the block in question, SourceMapper.anonymousClassName will always be 0.

Consider this block in enterType():

			this.anonymousCounter++;
			if (this.anonymousCounter == this.anonymousClassName) {
				this.types[this.typeDepth] = getType(this.binaryTypeOrModule.getElementName());
			} else {
				this.types[this.typeDepth] = getType(DeduplicationUtil.toString(typeInfo.name));
			}

if ananymousClassName is always 0, the then-branch of the above if will never be triggered, i.e., we always retrieve the type using an empty name. To check:

• do we have tests challenging this code with more than one anonymous class (to be sure that int parsing is actually tested)?
• does getType("") actually supersede (since bug 75885) the dead then-block?
• perhaps we can completely remove field anonymousClassName and all code accessing it?

[jukzi]

• do we have tests challenging this code with more than one anonymous class (to be sure that int parsing is actually tested)?

AttachSourceTests.testInnerClass3 did evaluate the number to "2"

[jukzi]

@stephan-herrmann what do we need to continue?

[stephan-herrmann]

@stephan-herrmann what do we need to continue?

Do we have any motivation to keep that code section?

See my hint:

perhaps we can completely remove field anonymousClassName and all code accessing it?

Debugging AttachSourceTests.testInnerClass3 I see the field used only to distinguish between using "" or "" as the classname :) -- The name of the classfile will always be determined inside getType().

[stephan-herrmann]

perhaps we can completely remove field anonymousClassName and all code accessing it?

RunModelTests confirms my theory.

[jukzi]

perhaps we can completely remove field anonymousClassName and all code accessing it?

I created a new PR for that: #3583