Unexpected match of Structures

[slysherz]

This Structure:

var $t = _;

draw = function() {
    // You can add whatever code you want here, it still passes
    var s = "My cat is fluffy";
};

matches true to:

//  For loop has to be before draw
for (var x = 0; x < 1; x++) {
    var a = 0;  // This line is not necessary on the demo
}

// Var is required here
var draw = function() {

};

The problem is present in both KA and the demo.

[k4b7]

Thanks for the bug report. We'll look into it.

[k4b7]

Verified. That it very strange behavior. Not sure when we'll get to this, pull requests are welcome.

[MrMagma]

This issue and #21 seem to be caused by the same thing, namely, that structured js looks into control statements when it's checking for a match. This type of recursive check makes sure that a variable declared inside of an if statement will match a pattern looking for just a variable declaration. Unfortunately, this recursive checking doesn't take into account the types of nodes being checked.

The simplest solution to this issue would be to eliminate the check. Doing this would, however, break the variable declaration equivalence mentioned above. Is this equivalence an essential feature?

Personally, I don't think that a variable declared outside of a statement should be considered equivalent to one declared inside of a statement, the reason being that the first structure will always result in a variable being declared while the second may not in some cases. On top of that, I don't see many situations where people would want the two structures to match.

[slysherz]

Thanks for taking the time to look into this @MrMagma. I don't know if I understood your explanation, so I would like to ask you to clarify a few things.

I don't quite understand how does the recursive type check makes one thing match against the other.

I tried to understand what is going on by adding a bunch of logging to Structured, and this is how I interpreted it: the first variable assignment is matched against the variable inside for, as expected.

Then it tries to match var draw = ... expression, which fails because there is no such expression in the user code. The weird things happens after: even though it failed, it stops as if it had matched.

Placing a variable wildCard inside draw () {} and checking it's value inside a constrain yields an empty object (Structured never gets to fill it in). This seems to happen everytime Structured matches something that isn't there, so I have been using it as an work-around to distinguish real matches from false positives (testing if the variables only come out as empty objects).

Again, thank you!

[MrMagma]

Ah, sorry @slysherz I believe that I misunderstood the problem you were having. I thought the variable in the for loop was what was mismatching. However, the issue I mentioned with variables and your problem seem to be related.

The code and structure you provided do not match (expected) in my local version of structured.js where I've fixed the issue with variables that I mentioned above.

I don't have sufficient understanding of what's going on to explain why, but the line that seems to be causing the issue is this one. In my local version, I changed peersToFind to an empty array which fixed both your issue and the issue with variables. The only tests that broke were the ones that dealt with things like:

var _ = _;

matching to

if (a) {
    var b = c;
}

which is what I expected to happen.

[k4b7]

Personally, I don't think that a variable declared outside of a statement should be considered equivalent to one declared inside of a statement, the reason being that the first structure will always result in a variable being declared while the second may not in some cases. On top of that, I don't see many situations where people would want the two structures to match.

@MrMagma good point. You're probably right, but I'd like to be sure that none of the challenges are relying on this behavior before making this change.

[slysherz]

I'm pretty sure it would brake some challenges. Let me give you an example where the current behaviour is useful:

Suppose the user is expected to make a new variable, and use it inside an if
var myVar = _; if (...) { doSomething(myVar); }

If it doesn't make any difference for the variable to be inside or outside the if, it's common to omit the if when pattern matching against it, because it allows to match multiple possibilities in one go.

Another problem I see with it is that it would make var ... expressions behave differently from other expressions, which would be confusing for anyone learning Structured.

But not everything are bad news. I think I found a simpler way to replicate the bug, which might help understanding why it works the way it does.

Structure:
rect(); ellipse();

Matches true against Code:
draw = function () { ellipse(); rect(); }

I didn't yet had the time to run this one through my logging Structured, but I will when I can.

[slysherz]

After some time thinking about this, I decided to completly rewrite the StructuredJS core. The reasons for this were:

• Making the code simpler to reason about, debug, extend and modify;
• Make it faster;
• Make it consistent;
• Fix all the pesky bugs;

The core part of the rework is complete (still needs polish), so I would like to share it with you and ask for your opinion. Here's what I did:

Instead of having a crazy amount of state information and change it as we traverse the tree (from top-down), I took a more functional approach: I pass in the code tree and the structure tree to a function and ask: what are all the possible ways these trees match each other?
The answer for each node is built from the answer we got from matching the child nodes. Because the limited number of arguments, I can build a cache with every result to speed things up.

I think I succeded in all my goals, except for the speed one. Running all the tests takes ~7% more time across all browsers, which isn't bad considering that:

• There are still a few dramatic optimizations to make, mostly related to how the cache works;
• Some of the StructuredJS bugs are speeding it up by a lot, by not looking for things it should look for;

What I do next depends on what you guys think. I see three options:

• Accept that the bug fixes are breaking changes and take the oportunity to complete overhaul the interface. This would allow us to add pretty cool stuff to Structured, like returning the reason for failed matches and use code as constraints;
• Use the old interface with the new core. This requires minimal maintenance but limits what we can do with Structured going into the future;
• My rework is bad and it's not worth using over what we have;

Currently, my version passes most of the tests we had. Some tests at the end fail because they test injectData which I don't yet know what it's supposed to do. For all the others, the result is the expected one, even though some return failures - it's because I'm using a different format to return results. This will be fixed depending on the going forward objectives we set.

Test 15:2 is the only one where the new version disagrees with the old. I think it should return pass to be consistent with the behaviour from 15:1.

You can find the updated version on my Structured branch, at https://github.com/SlySherZ/structuredjs

Please let me know what you think, thanks! ^ . ^