KSP-KOS / KOS

Fully programmable autopilot mod for KSP. Originally By Nivekk
Other
691 stars 229 forks source link

DEFINED binding order unexpected in boolean logic. #1507

Open surge9000 opened 8 years ago

surge9000 commented 8 years ago

[http://ksp-kos.github.io/KOS_DOC/language/variables.html#defined] mentions a DEFINED reserved word however using it in boolean logic can cause errors in non-sensical ways:

SET bla TO "foo".
if (DEFINED bla) print "so far, so  #good...".
if (NOT (DEFINED bla)) print "this works too".
// now for the fun...
if (NOT DEFINED bla) print "what madness is this? NOT is supposed to bind to an EXPRESSION, not a keyword".
// or
if (NOT DEFINED(bla)) print "its not a function, I guess".

Tested on kOS 0.19, KSP 1.0.4

BTW, there are a several misplaced full stops in the documentation example.

Dunbaratu commented 8 years ago

There's an available workaround (the parentheses) and changing order of operations is not a trivial change to squeeze in at the last second when we're about to release in about a half hour so it will have to wait for the next release. (Also, this was always this way and wasn't introduced in 0.19.0).

So I'm putting it off into the next update after.

The current implementation makes DEFINED into a unary prefix operator that has equal precedence to unary NOT and unary PLUSMINUS. The parser generator tool we're using isn't quite smart enough to be told "please associate these terms right-first instead of left-first" which would be one way to fix it. The only other available fix is to alter the grammar file in such a way that we insert a new level in the parse tree between existing levels, and that has effects that percolate through to other spots in the code. Thus why fixing it isn't just a quick and dirty one-line change.

AlchemistCH commented 8 years ago

Boolean operations in kOS seem to have not only the same operators as in Pascal, but also the same logic. That is, don't presume anything about order - put the ( ) wherever fit (even if it compiles without that).

surge9000 commented 8 years ago

I knew it wouldn't be easy. kOS clearly uses a LARR parser, not LALR. I laughed my arse off when it was origionally introduced with the full-stops as terminators - it was a quite funny joke.

But now that people are trying to use it properly, you have a problem... I have spent 30 years developing languages and interfaces and I still have less of an idea what to do about it than you, so...??? Blame it on Microsoft or Sony? I don't know.

It will become a huge problem if you don't at least try to fix it ASAP though. I wouldn't touch C# with a 50-foot barge pole, but just as an advisory, something like bison/yacc will do that for you in no time... and spit out obfuscated C. If you can convert it into whatever demented version of java Microsoft thinks C# is... easy as pie.

Dunbaratu commented 8 years ago

Differentiating periods as terminators from periods as decimal points is handled entirely in the lexxing rather than parsing step. (period)(whitespace|EOF) is a terminator, while (period)(numbers) is a fractional trailer to a literal number.

kOS currently uses an LL(1) parser generator called TinyPG. In its original form by Nivekk it didn't even use a proper parser at all. Using TinyPG is very similar to using JavaCC or ANTLR. Living with the restrictions of LL(1) is far less headache than trying to translate arbitrary obfuscated C code into C# (which, if you think about it, is something that itself requires parsing), which is why lex/yacc isn't the easier solution you're pretending it is.

And, no, it won't be a huge problem to wait until later to fix the fact that DEFINED has equal precedence to NOT and -(unary). The only reason it was put off was because of the timing. It also means going into the compiler and adjusting the recursive tree-walk code that reads the resulting parse tree to now expect two nodes one being a child of the other, in a rule where it previously expected just one. We had a ready to release version on-deck going through the final tests to be released that night. Making that low-level of a change right then would have invalidated testing and required rewinding the release steps. Just not worth it for a change that is super super minor because it has a workaround (an extra set of parentheses) that will NOT become wrong later on when it is fixed so using it doesn't mean breaking your future compatibility.