ID:2311311
 
Resolved
Compiled code for the new "chaining" dot operator, and the conditional dot operators, behaved incorrectly in some complex cases. To alleviate this problem, they now compile with new instructions. Please note: Projects compiled from this version of 512 forward that use any of these operators won't work properly in earlier 512 versions; likewise conditional dot operators compiled in earlier 512 versions will not work correctly in the new version. It is important to recompile any 512-specific code you are using to host in the 512 beta.
BYOND Version:512.1393
Operating System:Windows 8
Web Browser:Firefox 56.0
Applies to:Dream Maker
Status: Resolved (512.1394)

This issue has been resolved.
Descriptive Problem Summary:
When using the chained dot operator, some strange behavior can be observed when using lists. From what I can tell, the list needs to be of a datum that has a reference to another datum. If you call a proc on the child datum and pass another child element that exists in that list, the proc will be called on that element instead.

Code Snippet (if applicable) to Reproduce Problem:
TestChild
proc
PrintSelf()
world << "I am object \ref[src]!"

TestItem
var
TestChild/child = new

mob
verb
TestChain()
var/list/TestItem/datumList = newlist(/TestItem, /TestItem, /TestItem)
world << "By Variable:"
world << "Element 3 is \ref[datumList[3].child]"
var/TestChild/testChild = datumList[3].child
testChild.PrintSelf()
testChild.PrintSelf("test argument", "test argument")
testChild.PrintSelf("test argument", datumList[1].child)

world << "By Chain:"
world << "Element 3 is \ref[datumList[3].child]"
datumList[3].child.PrintSelf()
datumList[3].child.PrintSelf("test argument", "test argument")
datumList[3].child.PrintSelf("test argument", datumList[1].child)


Expected Results:
All output from the above should be the same.

Actual Results:
Output using the chain operator, while passing a child element, is different.
E.g.
By Variable:
Element 3 is [0x21000006]
I am object [0x21000006]!
I am object [0x21000006]!
I am object [0x21000006]!
By Chain:
Element 3 is [0x21000006]
I am object [0x21000006]!
I am object [0x21000006]!
I am object [0x21000002]!


This happens regardless of the number of arguments passed, or the position of the argument. However, it seems to always take the last instance of one of these if there are multiple.

Does the problem occur:
Every time? Or how often?
Every time

Workarounds:
You can assign the list member to a variable as seen above, and there will be no problem.
Huh. That's an odd one to be sure. I'll take a look and see what comes up.
Lummox JR changed status to 'Verified'
I have an explanation for why this is happening, and I'm working on a fix. The problem is compiler-side.

This is the line of code that always messes up:
datumList[3].child.PrintSelf("test argument", datumList[1].child)


Basically, the operators are being chained like this:

((datumList [] 3) . child) . call(Printself)("test argument", ((datumList [] 1) . child))

The dot before PrintSelf is at the top of that tree. Everything on the left-hand side is processed first, then put in a local register var called "loc" (no relation to atom.loc), the rest of the expression is evaluated. The call is done on loc.PrintSelf. However the arguments have to be evaluated and pushed to the stack first, and the argument datumList[1].child is doing a new dot-chain instance which overwrites loc.

The correct order is this:

1) Evaluate the LHS of the main dot (datumList[3].child).
2) Push the arguments to the call instruction.
3) Pull the earlier LHS expression out of the stack, from underneath the proc arguments, and assign it to loc.
4) Lookup loc.PrintSelf to complete the call.

A slightly incorrect order (in that it has right-to-left precedence, so it'd screw up if the LHS has any side effects) that will get the right result is this:

1) Because the RHS of the main dot is a call or index instruction, evaluate its arguments first.
2) Evalulate the LHS of the dot and assign it to loc.
3) Lookup loc.PrintSelf to complete the call.

I don't like the second option because it's going to screw up in rare cases like mylist[++x].myproc(mylist[++x].name). Really the first mylist[++x] needs to be evaluated first. However, the only appropriate way to do the first one that I can think of so far is to add new instructions, which at this stage of 512 seems slightly iffy. I'm going to have to give this a lot more thought, probably over the weekend.
512 is still in beta isn't it, doesn't that make this the BEST time for new instructions?
In response to CrimsonVision
The problem is if I add a new instruction and someone on an old beta version tries to run code compiled from a newer beta with the new instruction, they'll run into problems.
Could you not issue a warning with the new version? I think it's quite understandable and expected that running a newer version of something using the older version would cause issues.
I agree. The longer this issue is left compiling wrong, the more people the upgrade will affect.

I ran into a similar bug to this last night and couldn't pare down a reproducible test case, but now that you've explained what's happening in the compiler, I'm confident this is what I was seeing.

Immediately is the best time to deal with this issue, because as of right now, 512.1393 is not only unstable, but several of its major features still do not work well enough to justify adoption.

1394 was going to be the point where I started pushing examples for adoption because that should be the fix that makes vis_contents fully functional. Unfortunately, this bug in the way that chaining works makes me concerned that it will not go well at all. Several of the snippets I've socked away for pushing vis_contents require chaining, and if it's not reliable, things will get real messy real fast.


As for the algorithm you are trying to chase down, you should be shunting. Complex mathematical operations can be approached using the basic structure of a shunting-yard algorithm. All of this can be expressed as an extension of infix notation.
Yeah, that's one reason I haven't released 1394 yet. I want to adopt a solution in 1394. The incorrect-order one, where there's an RTL association problem, is probably acceptable. The right-order solution is a lot harder to work out, although I might be able to do it without a new instruction if I can think it through properly.
I think I finally have a solution to this. It involves a couple new instructions and changes to the old ones, and also reclaiming a var index that hasn't been used in many long ages.

Basically my new solution is that instead of popping to "loc", the stack is rolled so the newest value ends up on the bottom. A new var index called dotchainIdx will act like locIdx except that it shifts from the bottom of the stack, and the conditional dot operations have likewise been altered. The compiler keeps track of whether it should output dotchainIdx or locIdx in a given situation. This means the code used is also smaller.

The downside is, obviously code compiled with the newest beta will be unusable by old betas. (Or more precisely, errors will result when trying to use the affected sections.) Probably not a big deal.
I don't think anybody should be expecting dm -> dd compatibility between beta revisions. (or between beta and stable)
Lummox JR resolved issue with message:
Compiled code for the new "chaining" dot operator, and the conditional dot operators, behaved incorrectly in some complex cases. To alleviate this problem, they now compile with new instructions. Please note: Projects compiled from this version of 512 forward that use any of these operators won't work properly in earlier 512 versions; likewise conditional dot operators compiled in earlier 512 versions will not work correctly in the new version. It is important to recompile any 512-specific code you are using to host in the 512 beta.
they not compile with new instructions.

I think you meant

they now compile with new instructions.
Good catch.