75560 - JS_IsAssigning does not work for increment/decrement operators

Status: VERIFIED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[John Bandhauer]

[from news://news.mozilla.org/netscape.public.mozilla.jseng]

Subject: JS_IsAssigning and the increment/decrement operators?

In the new XPConnect code I'm using the same cloned function
object to do double duty as both a setter and a getter. I'm
determining which activity is indicated by calling
JS_IsAssigning. It turns out this does not work in today's
SpiderMonkey for the increment and decrement operators.

I'm wondering about how to deal with this. Is there some other
way to sniff out whether we are in a get or set situation? Since
only one bytecode op is used to indicate both the get and set of
this operator I don't see a strategy similar to the current
JS_IsAssigning js_CodeSpec scheme working.

We could burn the (currently) spare JSPackedBool in the
JSStackFrame struct to indicate this. But we'll have to pay the
runtime overhead of setting and clearing it.

Any other ideas?

It will suck if I need to make separate getter/setter function
objects to deal with this one anomaly. And, of course, we really
don't want JS_IsAssigning to be exposing disinformation.

Thanks,

John.

Comment 1

[Brendan Eich [:brendan]]

JS_IsAssigning merely needs to test for JOF_INCDEC as well as JOF_SET.  Can you
do the patch?  I've still got too many changes in my tree (jsapi.c has a change
from the patch for bug 72884, which I hope you'll review soon).  This one is
easy, and shouldn't bottleneck thru me.

/be

Comment 2

[John Bandhauer]

> JS_IsAssigning merely needs to test for JOF_INCDEC as well as JOF_SET.

No. That won't work. The interpreter does both a get and set while executing 
that one op code. 

Comment 3

[John Bandhauer]

Also, I'm happy to do the actual fix. I'm asking for advice. I think we need a 
flag in the frame. I'm wondering if a better solution will present itself. Also, 
can you think of other situations where checking JS_IsAssigning is not a safe 
way to tell whether a property accessor represents set and not a get? Is relying 
on this function really safe? 

Comment 4

[John Bandhauer]

Ugh. I didn't notice that brendan reassigned to me w/o adding himself to the cc 
list. Please note the 'why you're wrong' comments above.

Comment 5

[Brendan Eich [:brendan]]

Duh!  Sorry, I didn't get your meaning.  I withdraw that JS_IsAssigning idea,
I'm not convinced testing JOF_INCDEC is right.

But your shared getter/setter function is trivial to handle.  Just test argc to
decide whether the function is being invoked as a getter or a setter.  Or am I
being dense again?

/be

Comment 6

[Brendan Eich [:brendan]]

Make sure you spec the function (if native, via JSFunctionSpec or via params to
JS_NewFunction) to take 0 minimum-expected args.  Then when called as a getter,
argc will be 0.  When called as a setter, argc will be 1.  I'm curious why this
didn't occur before the thought of calling JS_IsAssigning?  Maybe the lack of
comments.... </me hides>.

/be

Comment 7

[John Bandhauer]

Yes, that works for me. Part of the reason this solution did not occur to me was 
that I've never really *understood* what the 'minimum-expected args' param to 
JS_NewFunction meant. So, I was misusing it.

So, I'm happy for my bit of code. But we are left with JS_IsAssigning 
giving a less accurate answer than some (like me apparently) might expect. Do we 
want to just document it and mark this bug invalid? or what?

Comment 8

[Brendan Eich [:brendan]]

I am not going to add yet more runtime cost to frame construction and bytecode
execution just to make JS_IsAssigning better.  I would rather deprecate this
API, which was added before JSNewResolveOp, and which the flags to
JSNewResolveOp (once I fix bug 72354) makes unnecessary.  Marking WONTFIX.

/be

Comment 9

[John Bandhauer]

I'm good with that. Thanks!

Comment 10

[Mike Shaver (:shaver emeritus)]

So what about

  nativeFunctionReturningLValue()++;

?  Don't such functions have to check JS_IsAssigning?

Comment 11

[Mike Shaver (:shaver emeritus)]

js> function f() { 
  a++;
  func()++;
3: invalid increment operand:
3:   func()++;
3: ........^

Well, that sucks.

Comment 12

[Brendan Eich [:brendan]]

Reference types suck.

/be

Comment 13

[Brendan Eich [:brendan]]

Bug 75688 asks for the "reference-type-returning native function can't be the
operand of an inc/decrement operator" bug shaver found to be fixed.

/be