`null condition member access` does not work with method call

[itlancer]

Problem Description

null condition member access does not work with method call. https://airsdk.dev/reference/actionscript/3.0/operators.html#null_condition_member_access

Tested with multiple AIR versions, even with latest AIR 51.1.3.10 with multiple OS versions, devices and architectures with different applications. The same issue with in all cases. It works fine with properties.

Related issues: https://github.com/airsdk/Adobe-Runtime-Support/issues/2450 https://github.com/airsdk/Adobe-Runtime-Support/issues/460 https://github.com/airsdk/Adobe-Runtime-Support/issues/489 https://github.com/airsdk/Adobe-Runtime-Support/issues/894

Steps to Reproduce

Launch code below.

Application example with sources attached. null_condition_member_access_method_bug.zip

var obj:Object = null;
if (obj != null) obj.test();//Works fine
obj?.test();//Cause TypeError: Error #1006: value is not a function.

Actual Result: Exception:

TypeError: Error #1006: value is not a function.

Expected Result: Correct work without exception.

Known Workarounds

Do not use ?. checks with methods calls.

[ajwfrost]

Just to add some more details:

• if the object is not null, and the method exists, this syntax works fine
• if the object is not null, and the method doesn't exist, it throws ReferenceError: Error #1069: Property fn not found on obj and there is no default value.
• if the object is null, then this TypeError: Error #1006 is throw regardless of whether the method exists or not

Not really what's meant to happen!!

[ajwfrost]

I think this may be a compiler issue. With a function:

	private function testNullMethodCall(obj : Object) : void
	{
		obj?.test();
	}

we get the bytecode:

       0    getlocal0     	// 'this'
       1    pushscope     	
       2    debugfile     	...
       5    debug         	1 236 0 0
       11   getlocal1     	// 'obj'
       12   pushnull      	
       13   debugline     	451
       16   ifne          	L1	// i.e. if ('obj' != null) goto L1

       20   pushnull      	
       21   jump          	L2
       
       L1: 
       25   getlocal1     	// 'obj'
       26   getproperty   	test //nameIndex = 244
       29   coerce_a      	// i.e. get the function property
       
       L2: 
       30   getlocal0     	// why?
       31   call          	(0)	// implies it's a call either to "this.test" or "this.null"? 
       33   returnvoid    	

If we remove the ? in the AS3:

       0    getlocal0     	
       1    pushscope     	
       2    debugfile     	...
       5    debug         	1 237 0 0
       11   getlocal1     	
       12   debugline     	455
       15   callpropvoid  	test (0) //nameIndex = 245
       19   returnvoid    	

which is a lot simpler.

I think actually, it's just that the 'L2' label is too early. The actual compiler behaviour should turn l?.r into (l == null) ? null : l.r

This works fine when the right hand side is just a member variable. But if it's a function, that's where the challenge happens - it seems the new compiler feature is working on the initial token and the parentheses/arguments are then handled separately, causing this odd jump target.

[ajwfrost]

So, we're able to fix this, by making the compiler take the right-hand token that could be a function call rather than just an identifier ... but, this exposes another issue with the parser here, which related to multiple/nested calls.

I've just been playing with the compiler in Apache Royale which has a very similar codebase, and I noticed that they handle the multiple/nested call scenario (i.e. a?.b?.c) but they too have an issue with function calls here rather than just property access.

For example their tests include things like o?.toString() where o is a false boolean, or an empty string, etc.. but they don't try the same thing when o is a null Object. When I add that case, it fails.

I've applied our fix to the Royale compiler code but this now struggles with nesting - presumably because of the changed priorities.

Adding @joshtynjala here too for his information .. fyi, the change we made to get it to recognise the function calls was to change

TOKEN_OPERATOR_NULL_CONDITIONAL_ACCESS r=accessPart

to

 TOKEN_OPERATOR_NULL_CONDITIONAL_ACCESS r=nullConditionAccessPart

and then define that new access-part which has assignmentExpression as the first option (and we can remove the XML expression which may simplify other stuff?).

Then in the handler for this, we have to check what type the right-hand operand is; if it's a function, we need to turn that into a method of the left-hand token, rather than setting it up as a member access operation.

When we try that, our test case (i.e. the original error snippet above) starts working, but the test cases fail when you have nested operations e.g.

  [junit] ASNullConditionalOperatorTests...as(12): col: 19 Error: 'a.' is not allowed here
  [junit] var result:* = o?.a?.[0];

For now, I think we can update the AIR compiler, so that it at least handles the function call mechanism (a?.fnc()), and then we'll have the restriction that you can't nest these calls. We can then look at a better way to get the parser to work out the priorities of the various structures, and ensure the property access ripples down the chain so that something like

trace(   a?.b?.c()?.d?.e()?.f()?.g );

would work!

[joshtynjala]

I don't feel like I want to use assignmentExpression here. It is pretty greedy in terms of how much it includes in the right operand. For instance, it would go beyond the last a member access and include the == equality operator and its right-hand side in the following example:

if (obj?.a == "hi")
{
    
}

I wouldn't have minded rewriting function calls only, but if most expressions need to be rewritten, that feels like too much to me.

I have some ideas that I'm playing around with, though.

[joshtynjala]

I have some ideas that I'm playing around with, though.

Following up on this previous comment of mine.

I committed an improved implementation of the null conditional operator to Apache Royale's compiler:

https://github.com/apache/royale-compiler/commit/f72dd87343468e968249192503610c2c75b1d6b8

There are still some edge cases where it can still fail, but this new implementation is much better than the old one, so I felt it was worth committing as a better foundation for additional future fixes. There are a ton of new tests, including ones that cover the issues mentioned here in this thread.

[ajwfrost]

Thanks @joshtynjala - looks good, and yes I think an approach that improves things is a good idea even if it's not covering every scenario. It's a bit more complicated than we had first thought, to cover every single potential bit of syntax for this!

[joshtynjala]

Today's commit to the Apache Royale compiler should fix the remaining edge cases with the ?. operator that I was aware of. I added another big batch of tests to go with the fixes.

https://github.com/apache/royale-compiler/commit/cc319f1a790b8ac8da1e062a1b0f41b0b325ec81