Closed Bug 706469 Opened 13 years ago Closed 13 years ago

[ASC BUG] ASC issues strict mode errors when undefined used in conditional expression

Categories

(Tamarin Graveyard :: Tools, defect, P1)

Product:
Tamarin Graveyard
Component:
Tools
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Q2 12 - Cyril

People

(Reporter: tierney, Assigned: lhansen)

References

Details

Following code produces a strict mode error in the new ASC, used to work in old ASC:

// Works
var y:Object = undefined;
// Error in strict mode
var x:Object = true ? {} : undefined;
Found while trying to compile flex SDK with new ASC
-abcfuture is not required.  -strict is required.

The error:

[Compiler] Error #1067: Implicit coercion of a value of type void to an 
   unrelated type Object.
   /Users/lhansen/work/asc.as, Ln 5, Col 29: 
   	var x:Object = true ? {} : undefined;
   ............................^
Blocks: float/float4
Priority: -- → P1
Target Milestone: --- → Q2 12 - Cyril
This is a wrongful optimization.  Ast.condOp() performs constant evaluation if the conditional expression's boolean value is known.  But the optimization just leaves the "then" or "else" value as the expression.  That is just wrong; the resulting expression must always be cast to the least upper bound of the types of the "then" or "else" value.

That optimization has been there for a long time, but it only kicks in now because "undefined" is reliably being evaluated as a constant expression.  If instead of "undefined" we write "void(0)" then even older ASC exhibit the same problem.
Although in this case it's curious that there's an error since the type of {} should be Object and there should be no problem.  So there's something going on in addition.
To add insult to injury, neither old nor new ASC appear to actually generate optimized code, but instead generates an if/then/else form...
On the other hand...  undefined is not in the value set of Object and though there is no run-time error here (because ASC generates coerce_o instead of convert_o) it is in a sense correct to signal an error.  The more interesting question is then why the first line in Erik's example does not cause an error.
And one further observation: the wrongful optimization does not actually kick in, which is a bug in itself.
Appears related to constant expression evaluation.  Changing Erik's first line to use void(0) instead of undefined we provoke an error on that line also:

[Compiler] Error #1067: Implicit coercion of a value of type void to an unrelated
   type Object.
   asc.as, Ln 2, Col 21: 
   	var y:Object = void(0);
   ....................^

Superficially it could look like the underlying problem is that "undefined" in an initializer context is not properly resolved to a constant value in the new compiler but that's not true, abcdump shows that it is (it was not so in the old compiler).
Assignee: nobody → lhansen
Three cases (all with the new compiler):

1)  var x:Object = undefined;
2)  var x:Object = void(0);
3)  var x:Object = true ? {} : undefined;

The first is allowed, the second and third cause errors to be signaled.

The type computed for "undefined" in the first case is "*".  This is wrong.

The type computed for "void(0)" in the second case is "void".  This is correct.

The type initially computed for the expression in the third case is null.  (At best this is ambiguous.)  Subsequently, as a special case inside Context.coerce(), conditional expressions are recognized and the type of each arm is coerced to the result type.  The value and type of the "then" arm is null; this is wrong.  The type of the "else" arm is "void"; this is correct but inconsistent since the type computed for "undefined" in the second case was "*".

So in the two first cases, it is the coerce-on-init that passes and fails, respectively; in the last case, the coerce is in the "else" arm of the conditional expression and fails.

Thus it seems that ASC agrees with itself that the void type does not coerce to the Object type in strict mode (and I think this is the desirable behavior), but that it does not reliably compute the type for constant references and that this is the underlying behavior.  If it did so, then all three cases would fail, and for the same reason.
BTW, the global binding of "undefined" has type *, so computing "void" as the type of a reference to "undefined" arguably depends on constant expression evaluation and substitution having been performed already.
Indeed, by recording "constVal" as "val" in the simple case in evaluate(Context,GetExpressionNode) we get the type propagated properly early enough, and even the first case fails, as it should.

Making that fix makes me nervous, though - I need to read more of the code before I will be comfortable with it.
It's pretty clear that ASC has two rules for conversion of undefined to other types.  For default parameter values it's lenient; "undefined" converts to a lot of different things.  For assignments it's strict but inconsistent; undefined is disallowed but it's not consistently known that a value is undefined even if that value is later resolved to a constant.

The reason for the inconsistency seems to be that for initializations, as for default parameters, we use checkDefaultValue for the "assignment", not coerce, which is used in most other cases (and notably in the two arms of the conditional expression).  checkDefaultValue implements the lenient rule and coerce the strict rule.

The reasons for that discrepancy may be that the structure of ABC encourages the use of values that can be stored in traits, and checkDefaultValue plays a part in the dance around that.  (Or it could just be a bug.)
With the fix as in comment #11, we uncover more bugs:

[Compiler] Error #1176: Comparison between a value with static type Object and a
   possibly unrelated type void.
   ../../../shell.as, Ln 1380, Col 13: 
           if (o==undefined) {
   ............^

This error is correct for strict mode; the value set of Object does not include undefined so the comparison is bogus.  Woohoo!

On the other hand one wonders about the implications of shipping the fix...
I think this is likely an old ASC bug.  I don't see any reason we should allow the assignment in one case, but not the other.  

If a user is assigning undefined to something that can't hold undefined, isn't that likely an error, and then shouldn't strict mode catch it?  Certainly code that checks for undefined to see if the parameter was passed won't work, like:

function f( o:Object = undefined)
{
  if( o == undefined)  
  {  //not gonna ever happen }
}

(In reply to Lars T Hansen from comment #12)
> It's pretty clear that ASC has two rules for conversion of undefined to
> other types.  For default parameter values it's lenient; "undefined"
> converts to a lot of different things.  For assignments it's strict but
> inconsistent; undefined is disallowed but it's not consistently known that a
> value is undefined even if that value is later resolved to a constant.
> 
> The reason for the inconsistency seems to be that for initializations, as
> for default parameters, we use checkDefaultValue for the "assignment", not
> coerce, which is used in most other cases (and notably in the two arms of
> the conditional expression).  checkDefaultValue implements the lenient rule
> and coerce the strict rule.
> 
> The reasons for that discrepancy may be that the structure of ABC encourages
> the use of values that can be stored in traits, and checkDefaultValue plays
> a part in the dance around that.  (Or it could just be a bug.)
(In reply to Erik Tierney from comment #14)
> function f( o:Object = undefined)
> {
>   if( o == undefined)  
>   {  //not gonna ever happen }
> }

Actually if o comes in as a null pointer the comparison will be true.  This prints "Hi":

function f(o:Object = undefined)
{
  if (o == undefined)
    print("Hi");
}
f(null);
(Should have added:)

The reasons for that is that == treats null and undefined as equivalent, as a special case.
Anyway with the patch enabled I'm observing a bunch of failures in compiling test cases in the tamarin test suite, where they pass "undefined" to parameters that have known types whose value sets do not contain undefined, eg,

  float.sqrt(undefined)

Clearly this needs to work in standard mode so it's just another case where the test case is non-strict-only.  I'm just worried that this idiom is widespread in user code.
Oops, missed this while writing my other reply.

I agree that the error is correct, but yeah shipping it seems highly likely to require changes to user code.  

And I now realize I was wrong about my previous example - you will get into the if block, as it seems that undefined will get auto-converted to Object, and become 'null' before the comparison happens.  Of course then there is no way to tell the difference between 'null' was passed as an argument vs. no argument was supplied.



(In reply to Lars T Hansen from comment #13)
> With the fix as in comment #11, we uncover more bugs:
> 
> [Compiler] Error #1176: Comparison between a value with static type Object
> and a
>    possibly unrelated type void.
>    ../../../shell.as, Ln 1380, Col 13: 
>            if (o==undefined) {
>    ............^
> 
> This error is correct for strict mode; the value set of Object does not
> include undefined so the comparison is bogus.  Woohoo!
> 
> On the other hand one wonders about the implications of shipping the fix...
OK, here's what I'm going to do: I'm going to land this change, because it takes us in the right direction.  If we see a lot of broken code then we will not back the change out, but we will add proper type checking rules that allow the change to stay in.
Marking as "fixed" though in actual fact the test case will still fail to compile, and indeed additional cases will fail to compile with the fix.  Ie, I rule that the use of undefined in type contexts where undefined is not a part of the value set are bugs and the AS3 code in question needs to be rewritten.  At this point I'm doing nothing about default parameter values, those are still lenient.

changeset:   73:54b67009bcc6
tag:         tip
user:        Lars T Hansen <lhansen@adobe.com>
date:        Fri Dec 02 15:58:28 2011 +0100
summary:     Fix 706469 - [ASC BUG] ASC issues strict mode errors when undefined used in conditional expression.  Actually this fix does not remove the error, it makes more errors be signaled, but at least the result is a more consistent type checker (though not fully consistent, see comments in the bug).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
There's a bunch of (void 0) expressions in the builtin code, because an earlier asc would not allow (undefined) for the reasons outlined.
Work continues on this bug in bug #707661.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.