Feature ASH language server features

[fredg1]

r20898, r20899, r20902 cover:

• parseInvoke_redundant_check.patch
• ParserTest.java.patch
• ParserTest.java_2.patch
• parseAggregateLiteral_initial_bracket.patch
• parseNewRecord_parseTypedConstant_initial_token.patch

I still have to look at

• parseType_aggregates.patch
• parseSingleCommandScope.patch
• parseCatchValue.patch
• parseStatic.patch
• Parser.java_loops.patch
• parser_initialization_and_import_fix.patch

(I also still have parsePostCall_vs_parseValue.patch which we discussed we'd revisit later.) Let me know if I'm missing any other patches.

That's all of those submitted so far. You're up to date.

 

[fredg1]

Small patch which makes it so that

foreach in x

generates Key variable name expected
rather than Reserved word 'in' cannot be a key variable name
as you clearly had just forgotten to put a variable name, rather than trying to name it "in".

The impact of this patch is currently small, but will have a more important role later, as it will tell us whether to try to read the token or not.

 

[fredg1]

This turns parseVariableReference( final BasicScope scope, final Variable var )
into parseVariableReference( final BasicScope scope, final VariableReference var ).

This is because VariableReferences will hold their current location, while Variables only have the location of their initial declaration.
Using VariableReferences will allow us to place the red squiggle at the correct spot.

 

[heeheehee]

Okay, I think that's all of them. r20904 - r20910.

 

[fredg1]

This moves around a bit of logic in parseNewRecord.

While this has no impact, currently, this is because we are currently aborting as we get an error. Later, we'll be continuing the parsing, and need this to avoid a NullPointerException and an IndexOutOfBoundsException.

Additionally, this adds a warning for an oversight (should this instead be an error? Your call, I guess): We were not actually checking for commas between new record parameters.
That is, we were "checking for commas", but if we didn't see one, we were just... assuming... that we were in front of a ")" instead.
So... this allowed things such as this:

new rec(1 2 3 4 5 6);

as the equivalent of

new rec(1, 2, 3, 4, 5, 6);

(notice the very clear lack of commas).

This is... *cough*... not very... good, nor documented, let's say...

Currently, the "warning" (if we decide to leave it as such) is a simple RequestLogger.printLine, but that's because warnings are not really fleshed out in Parser, currently. They will be later on.


P.s. we're almost done with that step. Only 6 more small patches like these before we're done with the initial logical reformatting, and can move on to adding the next two features: continuous parsing, and locations tracking: one in parseFunction, parseVariable, parseType, parseAggregateLiteral and parseReturn, and one that's a little sprinkled throughout Parser

 

[heeheehee]

should this instead be an error?

Yes.

(I imagine it won't be too hard to change your RequestLogger.printLine to a "throw this.parseException(",", this.currentToken());")

 

[heeheehee]

In short: surprising and undocumented behavior that arises from implementation quirks is error-prone and should not be part of the contract.

Tangential: this patch prints that warning for the "new without closing paren" test case, which seems wrong.

I would expect the logic to look something like "read comma before reading expression, if we've already parsed something"

 

[heeheehee]

This logic makes tests do what I would expect:

while ( true )
{
    if ( this.atEndOfFile() )
    {
        throw this.parseException( ")", this.currentToken() );
    }
    if ( this.currentToken().equals( ")" ) )
    {
        this.readToken(); // )
        break;
    }
    if ( params.size() > 0 )
    {
        if ( this.currentToken().equals( "," ) )
        {
            this.readToken(); // ,
        }
        else
        {
            throw this.parseException( "Missing comma between record field initializers" );
        }
    }

I'll take a closer look at the rest of the patch tomorrow.

 

[fredg1]

In short: surprising and undocumented behavior that arises from implementation quirks is error-prone and should not be part of the contract.

Tangential: this patch prints that warning for the "new without closing paren" test case, which seems wrong.

Is this better?

I would expect the logic to look something like "read comma before reading expression, if we've already parsed something"

Not sure what you mean here.
Just a reminder/to be sure we're on the same page, the "new without closing paren" test reads
"record r {int a;}; new r(4"
Notice the "4" at the end. There already is something that was read. And the error/warning was only checked for after we parsed a first parameter, so I don't get what difference you're talking about adding?

 

[heeheehee]

Just a reminder/to be sure we're on the same page, the "new without closing paren" test reads
"record r {int a;}; new r(4"

Yes.

Notice the "4" at the end. There already is something that was read. And the error/warning was only checked for after we parsed a first parameter, so I don't get what difference you're talking about adding?ove

My point is that "Missing comma between record field initializers" is not appropriate to print (or throw as an exception, or whatever) in this case, since there's only one record field initializer present (or expected).

The atEndOfFile() check in your latest patch is incorrect, as it yields that error message in the case of

record r { int a;} new r(4;

which is also "clearly" a case of a missing ), and definitely not a case where we're missing a comma between record field initializers, since we only have one initialize. Although, even my patch doesn't quite do the right thing.

That said, the "new record with semicolon" case is also incorrect in this same sense -- the atEndOfFile check at the beginning of the stanza should compare currentToken() to ";", rather than explicitly checking for end-of-file.

(IMO we should also get rid of the weird handling for initializing void fields, since why would you want to mix void values and non-void values in a record? I could see an argument for void fields being used implement tag types although I'm not convinced anyone would ever use them in that way.)

 

[fredg1]

My point is that "Missing comma between record field initializers" is not appropriate to print (or throw as an exception, or whatever) in this case, since there's only one record field initializer present (or expected).

My bad. When I turned this from a warning to an error, tunnel vision made me not consider just... changing the message.

(IMO we should also get rid of the weird handling for initializing void fields, since why would you want to mix void values and non-void values in a record? I could see an argument for void fields being used implement tag types although I'm not convinced anyone would ever use them in that way.)

This one fixes this, and addresses that.
Also, while I'm here, it also moves the closing bracket detection to the start of the loop.
Previously, we were always expecting at least 1 field. This won't change, but if the user didn't put any, we want to be able to immediately exit the loop.

 

[fredg1]

This changes the logic of parseType to attempt to parse records even when not allowed (again, with the continuous parsing, we need to try anyway).

This is possible because the method parseRecord starts by looking for the token "record", immediately returning null if it doesn't find it.
"record" is a reserved word exclusively used to declare new records, so nothing is hindered by checking for them right away.

 

[fredg1]

Alters parseAggregateLiteral a little so that we try to read key-value pairs (components of a map literal) even once we identified that the user is supplying an array literal (which doesn't expect keys).

 

[fredg1]

This modifies the logic of parseFunction a bit so that the vararg error checks happen after the variable name was read.
This will allow us to have the full variable + type's location, rather than only the type, once those are implemented.

Additionally, this allows the "Only one vararg parameter is allowed" error message to be reached.

 

[fredg1]

A bit more complex than the last few; this patch modifies parseVariable so that the signal indicating that we're parsing parameters is a boolean, rather than scope being null, and waits until the end to throw the error.
This, again, will allow us to still keep trying to parse a parameter initialization, and will give us a full location once we throw the error.

Also shuffles the parsing logic a little, for efficiency.

 

[fredg1]

Now, finally.

There's actually one more patch, but submitting it now would create a conflict with parseNewRecord_initializers, so I'll wait.

Meanwhile, it's time to submit/present the libraries that we'll be using.

First off, I'd like to bump https://kolmafia.us/threads/build-x...d-add-a-note-about-new-jars.26273/post-163271 , because the note that I included in it is something that I would very much have liked to know when adding those libraries on my side, and may help others...

Libraries are here (they were too big to send as attachments): https://drive.google.com/file/d/1vG1oN2v5j4FksfAwext8IwPUNXzrXGej/view


Two things to note:

1. The source files for these libraries are included. However, for "some" reason, all but one of them seem to get detected by IDEs. Thankfully, it's the most helpful one (org.eclipse.lsp4j), but it's still a PITA. I tried with VScode, IntelliJ and Eclipse. All give the same result: they only see the source code for org.eclipse.lsp4j. For all the others, they think the source is missing (with IntelliJ offering me the use of its decompiler). I don't know if there's someone with more knowledge of this than me that could help with this.
2. One of those libraries is Guava. Guava is... big. A good 3 Mo, and barely any of it is used, so I don't know if it would be better to just copy the parts that are getting used in /lib ourselves. However, on top of knowing how much this practice is... not recommended/liked, I also don't know what exact part of it is getting used, due to the first note, above. (I mean, I could just open the source and navigate it in an IDE, but... I dun't wannaaaa...)

 

[heeheehee]

Meanwhile, it's time to submit/present the libraries that we'll be using.

if you want to just post a maven dep I can fetch that without a problem. In general, that's preferable to downloading from some random google drive. Provenance, and all that.

Why do we want library sources?

 

[fredg1]

Why do we want library sources?

Because these libraries (or, at the very least, the lsp4j ones. The gson and guava ones could be discarded) are not some mere "methods that we want to use".

These are interfaces that we will implement, and classes that we will use (most notably, the "Location" and the "Range").

The presence of these libraries will, first, allow us to see how these new classes work (as they will be added to a good number of our classes in textui/parsetree), but also, in the language server, are our references, our documentation about how we need to implement the interfaces.

 

[fredg1]

if you want to just post a maven dep I can fetch that without a problem. In general, that's preferable to downloading from some random google drive. Provenance, and all that.

lib/jar:

Central Repository: com/google/code/gson/gson/2.8.6

repo.maven.apache.org

Central Repository: com/google/guava/guava/30.1-jre

repo.maven.apache.org

Central Repository: org/eclipse/lsp4j/org.eclipse.lsp4j.generator/0.12.0

repo.maven.apache.org

src/jar:

Central Repository: org/eclipse/lsp4j/org.eclipse.lsp4j/0.12.0

repo.maven.apache.org

Central Repository: org/eclipse/lsp4j/org.eclipse.lsp4j.jsonrpc/0.12.0

repo.maven.apache.org

You'll need to manually open them and remove the .RSAs, though (if you're skeptical, you can try building the Jar, but, I can attest, it won't work (well, the "building" will work... the jar just won't do anything...))

 

[heeheehee]

These are interfaces that we will implement, and classes that we will use (most notably, the "Location" and the "Range").

Okay. Are we depending on interfaces outside of lsp4j.source? Namely, if I'm tinkering with the gradle build, should I bother specifying all the other lsp4j sources? (generator, jsonrpc)?

When I pointed Gradle to download org.eclipse.lsp4j: org.eclipse.lsp4j:0.12.0, it pulled in several other jars that are unlisted. Can we safely throw all of those away?

Are the sources only used for your IDE to help out with autocomplete? Can you get by with just configuring the build.gradle to feed the relevant sources into your IDE?