workaround or fix for the npe during Rascal testing

[jurgenvinju]

This behavior was consistenly observed on the syntax-role-modifiers branch, but not when running the tests from the REPL, only when running mvn test:

java.lang.NullPointerException
	at org.rascalmpl.types.ModifySyntaxRole.equals(ModifySyntaxRole.java:941)
	at io.usethesource.vallang.util.WeakReferenceHashConsingMap$LookupKey.equals(WeakReferenceHashConsingMap.java:103)
	at java.base/java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:940)
	at io.usethesource.vallang.util.WeakReferenceHashConsingMap.get(WeakReferenceHashConsingMap.java:202)
	at io.usethesource.vallang.type.TypeFactory.getFromCache(TypeFactory.java:130)
	at io.usethesource.vallang.type.TypeFactory.externalType(TypeFactory.java:187)
	at org.rascalmpl.types.RascalTypeFactory.modifyToLexical(RascalTypeFactory.java:134)
	at org.rascalmpl.semantics.dynamic.SyntaxRoleModifier$Lexical.typeOf(SyntaxRoleModifier.java:51)
	at org.rascalmpl.semantics.dynamic.Type$Modifier.typeOf(Type.java:148)
	at org.rascalmpl.semantics.dynamic.Expression$TypedVariable.typeOf(Expression.java:2849)
	at org.rascalmpl.semantics.dynamic.Formals$Default.typeOf(Formals.java:42)
	at org.rascalmpl.semantics.dynamic.Parameters$Default.typeOf(Parameters.java:35)
	at org.rascalmpl.semantics.dynamic.Signature$NoThrows.typeOf(Signature.java:50)
	at org.rascalmpl.interpreter.result.RascalFunction.<init>(RascalFunction.java:91)
	at org.rascalmpl.semantics.dynamic.FunctionDeclaration$Expression.interpret(FunctionDeclaration.java:144)
	at org.rascalmpl.semantics.dynamic.Statement$FunctionDeclaration.interpret(Statement.java:546)
	at org.rascalmpl.interpreter.result.RascalFunction.runBody(RascalFunction.java:384)
	at org.rascalmpl.interpreter.result.RascalFunction.call(RascalFunction.java:293)
	at org.rascalmpl.interpreter.result.ICallableValue.call(ICallableValue.java:50)
	at org.rascalmpl.interpreter.result.AbstractFunction.call(AbstractFunction.java:208)
	at org.rascalmpl.values.functions.IFunction.call(IFunction.java:48)
	at org.rascalmpl.interpreter.TestEvaluator.lambda$1(TestEvaluator.java:125)
	at org.rascalmpl.test.infrastructure.QuickCheck.test(QuickCheck.java:112)
	at org.rascalmpl.interpreter.TestEvaluator.lambda$0(TestEvaluator.java:123)
	at org.rascalmpl.debug.IRascalMonitor.job(IRascalMonitor.java:68)
	at org.rascalmpl.interpreter.TestEvaluator.runTests(TestEvaluator.java:94)
	at org.rascalmpl.interpreter.TestEvaluator.test(TestEvaluator.java:54)
	at org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.lambda$0(RascalJUnitParallelRecursiveTestRunner.java:216)
	at org.rascalmpl.debug.IRascalMonitor.job(IRascalMonitor.java:68)
	at org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.runTests(RascalJUnitParallelRecursiveTestRunner.java:202)
	at org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.run(RascalJUnitParallelRecursiveTestRunner.java:196)

It is always during the second run that the bug triggers:

[ERROR] Errors: 
[ERROR] org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.dataMatchesSyntaxTreesToo2: <2506,523>
[INFO]   Run 1: PASS
[ERROR]   Run 2: RascalJUnitParallelRecursiveTestRunner$ModuleTester.run:196->runTests:202->lambda$0:216 » NullPointer
[INFO] 
[ERROR] org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.dispatchingOnSyntaxRole: <379,250>
[INFO]   Run 1: PASS
[ERROR]   Run 2: RascalJUnitParallelRecursiveTestRunner$ModuleTester.run:196->runTests:202->lambda$0:216 » NullPointer
[INFO] 
[ERROR] org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.namePreservation1: <1155,641>
[INFO]   Run 1: PASS
[ERROR]   Run 2: RascalJUnitParallelRecursiveTestRunner$ModuleTester.run:196->runTests:202->lambda$0:216 » NullPointer
[INFO] 
[ERROR] org.rascalmpl.test.infrastructure.RascalJUnitParallelRecursiveTestRunner$ModuleTester.namePreservation2: <1798,617>
[INFO]   Run 1: PASS
[ERROR]   Run 2: RascalJUnitParallelRecursiveTestRunner$ModuleTester.run:196->runTests:202->lambda$0:216 » NullPointer
[INFO] 

It is unclear to me what this "Run 2" means and why it is triggered. It is clear that the garbage collector seems to have cleaned up the type that we are looking for when we run the test again.

[jurgenvinju]

• The given fix makes the tests succeed and does not throw the NPE anymore
• It is unclear to me how to write a unit test for this failing situation
• I'm working with a SNAPSHOT vallang for the moment on this experimental branch of rascal; so there is no rush. It is a breaking bug in the long run.

[github-actions]

Test Results

     99 files       99 suites   5m 51s ⏱️
244 535 tests 244 534 ✅ 1 💤 0 ❌
733 605 runs  733 602 ✅ 3 💤 0 ❌

Results for commit 07ac82e.

[DavyLandman]

Shouldn't the fix be that the equals method should return false for null? That's the contract of equals.

Since this change now keeps references alive more frequent. (The get would normally only he done after an equal hashcode)

[jurgenvinju]

Yes, but the wrapper obj is not null, only the obj.get() is null so that does not fall under the equals contract.
However, we can implement the same contract also for the reference, as the patch does while testing obj.get() != null, and then it indeed works.

My question is: do you also think this code is brittle for the case of garbage collected references, and that the fix is therefore correct?

[DavyLandman]

I meant that org.rascalmpl.types.ModifySyntaxRole.equals(ModifySyntaxRole.java:941) is broken.

[jurgenvinju]

Since this change now keeps references alive more frequent. (The get would normally only he done after an equal hashcode)

Can you explain that?

I meant that org.rascalmpl.types.ModifySyntaxRole.equals(ModifySyntaxRole.java:941) is broken.

So that should return false on null instead of throwing NPE?

[DavyLandman]

Since this change now keeps references alive more frequent. (The get would normally only he done after an equal hashcode)

Can you explain that?

Everytime you do a get, the reference is marked as in use, and that's what we want to avoid.

I meant that org.rascalmpl.types.ModifySyntaxRole.equals(ModifySyntaxRole.java:941) is broken.

So that should return false on null instead of throwing NPE?

Exactly, since the equals method should always be safe for a null value as parameter. In 99% the instanceof at the start of the equals method takes care of this case.

[jurgenvinju]

Ok thanks; I'm going to fix the equals method instead. This is exactly the feedback I needed.

[jurgenvinju]

Everytime you do a get, the reference is marked as in use, and that's what we want to avoid.

The original code does the get too. So is there a way to avoid that?

[DavyLandman]

Everytime you do a get, the reference is marked as in use, and that's what we want to avoid.

The original code does the get too. So is there a way to avoid that?

The get is only done at the very last moment, when the hash is equal. Not much to be done about that. All weak/soft containers behave this way.

Let's close this PR?