383674 - Statement that implicitly calls toString is incorrectly optimized away as a "useless expression"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

options("strict");
var x = {toString: function() { print('toString called'); } }; 
var f = function() { var j = x; j + ""; }
f()

Result: There is a "useless expression" warning and toString is never called.

Expected: toString should be called.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Yoiks, CheckSideEffects was way over-aggressive about binary and unary operators.

/be

Comment 2

[Jesse Ruderman]

With the patch, += with const is still incorrectly eliminated instead of turning into a +:

js> (function() { const a = ({toString: function(){print("called")} }); a += "" })()
typein:1: strict warning: useless expression
js> 

Should I file a separate bug for that?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Should I file a separate bug for that?

No, new patch coming here to address that.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 5

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsemit.c 3.257

/be

Comment 6

[Bob Clary [:bc] (inactive)]

I would like a bit of guidance on where to place tests such as this. I would like to begin placing tests into locations which are only tested by the branch where the fix occurs. This will cut down on failures on older branches which we don't care about.

If this is only going to be fixed on the trunk, I would like to put these in js1_8 where it will only be tested by the trunk and I won't have to worry about failures on the 1.8 branch. If it might be fixed on the 1.8 branch, I would put it in js1_7. Any objections?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> This will cut down on failures on older branches which we
> don't care about.

Actually those failures are informative since they tell that that the bug is no yet fixed and in fact the test case is expected to fail. Moreover, if unrelated commit suddenly makes the test to pass, it would be nice to know.

So I think a good way to solve this is to have per-branch list of expected failures and put there the tests that are not applicable to the branch or that will not be fixed.

Ideally jsDriver can be modified to do something smart with the list but even without patching the driver one can take advantage of it simply via passing the list as -L argument.

Comment 8

[Bob Clary [:bc] (inactive)]

Ok, that is reasonable. Is js1_7 ok even though the tests don't require it or should I keep dumping stuff into js1_5?

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> Ok, that is reasonable. Is js1_7 ok even though the tests don't require it or
> should I keep dumping stuff into js1_5?

If test does not require js1_7, then lets continue to put it into js1_5 so the directory structure reflects the language features, not a branch policy.

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-383674.js,v  <--  regress-383674.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows