Syntax bug

[Philosopher]

Lol, typing away on my REPL I randomly typed something like this:

Code: Select all

>>> [][][][]

and got this:

Code: Select all

[[[[]]]]

What???

Some further experimentation:

Code: Select all

>>> var vec = []; vec[]
[[]]
>>> vec = [1,2]; vec[]
[[1,2]]
>>> vec[:] # slice syntax, expected behavior
[1,2]
>>> vec[1,0] # same
[2,1]
>>> [[1,2],[3,4][]][]
[[[1,2],[[3,4]]]]
...


So I think I determined what happens, where it gets confused. The slice/index syntax is postfix brackets, with the brackets on top; for example, «vec[i]» results in a tree with "[" on top, "vec" on the left, "i" on the right. Something similar happens with the vector creation syntax; for example, the parse tree of «[1,2]» has "[" on top with "," on both the left and right and "1" on the left of "," and "2" on the right of ",". The weird syntax is the intersection of those: «vec[]» is technically a postfix expression, as it has the left bracket after «vec», yet it somehow appears the same as «[vec]». The only different is that «vec[]» should have no right side, only a left side pointing to "vec", and it should have a postfix precedence assigned (versus a precedence of none). However, BINARY() gets confused by the lack of right side in the tree of «vec[]», and since the vector list-generator only uses the left side, everything parses "fine".

It should be a simple matter of precedence to detect the erroneous expression, but the real question is: what should I do? Leave it there as a quirk, throw an error if it happens, or make a new behavior for it?

[Hooray]

new behavior would be kinda cool if we can come up with a use-case where this construct would make sense (which I am having a hard time with, but will think about it), I guess it could/should be some form of extended list-comprehension a la LISP - I am specifically thinking in terms of foreach-loops (e.g. map/reduce), but also anything else where we could use this to simplify complex expressions.

[Philosopher]

The problem, though, is that we can't accept a right side (because that would be a slice or index expression), so it would have to be some static operation on the left side.

[galvedro]

what should I do? Leave it there as a quirk, throw an error if it happens, or make a new behavior for it?

My opinion is that it should definitively not be left as a quirk unless it is difficult to fix. Now, error or behavior? Well, "vec[]" is certainly not an indexing syntax (there is no index there), although I could easily see it as a short cut for "vec[:]". However "vec[:]" is already so terse and explicit that I don't really see an advantage in giving it that meaning.

But. This is a potential source for bugs that can be detected by the compiler. I think it is quite easy that a "vec[]" construct will be the result of a typo made by the author, who forgot to add an index or slice specification. So I vote for reporting an error and help people detect their bugs early

[Hooray]

given that we can apparently not use it for any useful stuff, I agree with galvedro - we were planning to work on better diagnostics anyway ...

[Philosopher]

Okay, sounds good - I'll throw an error. Patch will come ASAP (can't do them on my iPod anymore... but I'd want to test it first).

[Philosopher]

Mkay, here it is (sent it to James already):

Code: Select all

diff --git a/simgear/nasal/codegen.c b/simgear/nasal/codegen.c
index 8636fbb..4cbc0ce 100644
--- a/simgear/nasal/codegen.c
+++ b/simgear/nasal/codegen.c
@@ -8,6 +8,7 @@
 #define LEFT(tok)   ((tok)->children)
 #define RIGHT(tok)  ((tok)->lastChild)
 #define BINARY(tok) (LEFT(tok) && RIGHT(tok) && LEFT(tok)->next == RIGHT(tok))
+#define SINGLE(tok) (LEFT(tok) == RIGHT(tok))
 
 // Forward references for recursion
 static void genExpr(struct Parser* p, struct Token* t);
@@ -634,11 +635,13 @@ static void genExpr(struct Parser* p, struct Token* t)
         else genExpr(p, LEFT(t));
         break;
     case TOK_LBRA:
-        if(BINARY(t)) {
-            genExtract(p, t);
-        } else {
+        if(SINGLE(t)) {
             emit(p, OP_NEWVEC);
             genList(p, LEFT(t), 1);
+        } else {
+            if(!RIGHT(t))
+                naParseError(p, "missing index or slice expression(s)", t->line);
+            genExtract(p, t);
         }
         break;
     case TOK_LCURL:


[galvedro]

Great, thank you!

Edit: how many bugs do you think are going to be exposed by the fix? Anyone?

[Philosopher]

Lol, as I mentioned to James in the email, my first "solution" would have segfaulted if this syntax was detected - yet FG still started . So none here.