Feature ASH language server features

[fredg1]

oh, wait, nvm, I see what I forgot; it has nothing to do with parseEscapeSequence

 

[heeheehee]

Yeah. You're missing the next step.

parseEscapeSequence treats this as an escaped newline, but that's actually the end-of-file. It then sets this.currentLine = this.currentLine.nextLine, which is null.

We then go back to the top of the loop of parse{String,PluralConstant}, invoke this.restOfLine(), and then NPE

 

[heeheehee]

I guess I wasn't clear. My "Looking at parseEscapeSequence" was meant as a "Moving on. Looking at parseEscapeSequence: ..."

 

[heeheehee]

Hm. The NPEs aren't actually those lines -- they're at

                    // Empty lines are OK.
                    this.currentLine = this.currentLine.nextLine;
                    this.currentIndex = this.currentLine.offset; <----

and

                this.currentLine = this.currentLine.nextLine;

                if ( this.currentLine.content == null ) <---
                {
                    throw this.parseException( "No closing ] found" );
                }

edit: I guess that's because I made it so restOfLine won't throw an NPE. Right... so of course it then throws when it goes to the next line.

Simple fix is to check this.currentLine.content != null as the loop condition, then unconditionally throw after the loop.

 

[fredg1]

yeah, first, I should make a method out of the

this.currentLine = this.currentLine.nextLine;
this.currentIndex = this.currentLine.offset;

's, and have THAT not do anything if we are at the end of file

 

[fredg1]

Ok, this fixes the NPEs in the parsePluralConstant patch

 

[fredg1]

In what circumstances do you expect to see IndexOutOfBounds? That actually only occurs in the case of "line abruptly terminates after \x" and similar, which will never actually trigger for the octal handling, as far as I can tell.

Yes, it's just for the "abrupt line termination" (or just "there's not as many characters as we expect")

which will never actually trigger for the octal handling, as far as I can tell.

?

\1 or \12 triggers it..?

these errors don't "only" trigger for \u and \x (i.e. a linebreak "right after" the special character)
they trigger for:
\x
\x1
\u
\u1
\u12
\u123
\1
\12

 

[heeheehee]

I think I had misread Java's documentation of substring, where for some reason I thought it only threw an exception if beginIndex > length(). Most likely I mixed up the documentation for the one-arg and two-arg versions.

 

[fredg1]

Anything I can do to help?

I prefer to not have too many submissions open at once , so I'm currently not really doing anything, meanwhile...

 

[heeheehee]

Sorry, I'll take another look at these changes tomorrow. I didn't see anything objectionable when I last looked, so should be straightforward -- just another pass through the code, maybe add some extra tests.

 

[heeheehee]

Okay. parsePluralConstants_2 is in r20852. Are there any other outstanding patches other than the two in this post? (I'll get to those momentarily.)

 

[heeheehee]

final_static_fields.patch is unobjectionable, so that's now r20853. The parsePostCall patch looks like it ought to be safe, but I would like to add some (many?) test cases to confirm that behavior isn't changing.

But beyond that: is there any harm in keeping parsePostCall around? Right now, that relies on parseCall / parseInvoke always being invoked in a certain place in parseValue. If we wanted to change this in the future (say, to add support for anonymous functions), then that assumption would break.

 

[fredg1]

But beyond that: is there any harm in keeping parsePostCall around?

It's not "harm", per se, but it's redundant logic that needs to be altered in the future to accommodate one of the incoming changes, meaning the change would need to be applied twice, while there's no need for it.

Right now, that relies on parseCall / parseInvoke always being invoked in a certain place in parseValue. If we wanted to change this in the future (say, to add support for anonymous functions), then that assumption would break.

Wouldn't anonymous functions still just go in parseValue? Just like how parseInvoke and parseCall are two different methods? it would just be "parseAnonymousFunction", meaning it would still be followed by parseValue's post-value handling?

Are there any other outstanding patches other than the two in this post? (I'll get to those momentarily.)

Huh, I learned a new meaning to the word "outstanding"...

Also, no. I paused at 3 (parseString, final fields, parsePostCall).

 

[heeheehee]

Wouldn't anonymous functions still just go in parseValue?

Maybe, maybe not.

If we wanted to add anonymous functions, I expect that we'd extend the type system.

Imagine some code like...

int() makeClosure(int x) {
  return int() { return x; };
}
makeClosure(5)();

(This is just to demonstrate that the result of an expression could be callable.)

 

[fredg1]

I have two diffs if anyone wants to take a look:

- ParserWithManyTests.patch is basically your changes, plus my tweaks, diffed against head.
- ParserPatch.patch is basically the diff of my tweaks vs your changes. Mostly minor changes in Parser (wording, formatting, the occasional logic change). And... 48 test cases, by my count (plus the ones you had provided earlier but commented out. And one new one that's commented out because it doesn't work).

@heeheehee btw, how'd you manage to make a patch that's "a modified version vs another modified version"?
I don't see a way to "store" a version of a file to diff. it against a later version of it, and if I make a copy of the file, and diff. that copy vs the updated original, it gives this:

--- C:/Users/Frederic/Desktop/KoLmafia-workspace2/kolmafia/src/net/sourceforge/kolmafia/textui/Parser.java    lun. août 23 21:29:44 2021
+++ C:/Users/Frederic/Desktop/KoLmafia-workspace/kolmafia/src/net/sourceforge/kolmafia/textui/Parser.java    lun. août 23 21:19:24 2021
@@ -1358,4 +1357,0 @@ public class Parser
-        if ( this.currentToken().equals( ";" ) )
-        {
-            throw this.parseException( "Missing index token" );
-        }
@@ -1403 +1399 @@ public class Parser
-        else
+        else if ( this.parseIdentifier( this.currentToken().content ) )
@@ -1436,0 +1433,4 @@ public class Parser
+        else
+        {
+            throw this.parseException( "Missing index token" );
+        }

 

[fredg1]

(i.e. it doesn't understand that they're both supposed to point to the same file)

 

[heeheehee]

I use git-svn with the Mafia repo.

I think what I did was commit the modified version to a different branch, switched back to the main branch, applied the base patch, then git-diffed against the commit.

 

[fredg1]

Welp, that was painful. Here's hoping I never do that again.

This is an update of parseAggregateType, split into 4 steps for easier review.

1. fix the unwanted behaviour we discussed earlier
2. wrap the last case in an "else" conditional
   
   In other words, transitions from
   

   if (x)
   {
       [...]
       break;
   }
   
   if (y)
   {
       [...]
       break;
   }
   
   [...]

   to
   

   if (x)
   {
       [...]
   }
   else if (y)
   {
       [...]
   }
   else
   {
       [...]
   }

3. put the "missing index token" error at the end of the conditional chain.
4. Group up the shared logic by putting it after the conditional chain.

 

[fredg1]

This should be the end of the "first" major step for the language server. Onto the second and third.

Second and third are:

• Always reach the end of the file.
  I.e. even if there's an error, continue parsing while trying to get our bearings back.
• Implement the use of Locations, i.e. allow every data type to store where in the file it is "located", and put that information to use.

These are *big*. More than the Token change. That one was just the start.

Luckily, these are *wayyy* easier to split into very small, incremental changes.
I'll start with submitting simple conditional changes, in the likes of:

[...]
if ( somethingBad )
{
    throw errorXYZ;
}

doSomething();

doSomethingElse();

to

[...]
if ( !somethingBad )
{
    doSomething();
}
else
{
    throw errorXYZ;
}

doSomethingElse();

, the goal being to making it easier to turn it into

[...]
if ( !somethingBad )
{
    doSomething();
}
else
{
    saveThatThereWasErrorXYZ();
    doAnAlternativeThing();
}

doSomethingElse();

afterwards.

 

[fronobulax]

This should be the end of the "first" major step for the language server. Onto the second and third.

Second and third are:

• Always reach the end of the file.
  I.e. even if there's an error, continue parsing while trying to get our bearings back.
  

• 
  
  Intellectual curiosity only.
  
  In my limited parsing implementation experience there is a point where Aborting is more helpful to the user than trying to continue. The classic example is cascading errors. But there are also cases where an error is isolated and blowing past it allows the user to potentially fix more than one thing when parsing is finished.
  
  So, does always reach EOF mean in the language server context there is no benefit to aborting early?