That's all of those submitted so far. You're up to date.
You are using an out of date browser. It may not display this or other websites correctly.
You should upgrade or use an alternative browser.
You should upgrade or use an alternative browser.
Feature ASH language server features
- Thread starter fredg1
- Start date
Small patch which makes it so that
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.
Code:
foreach in xrather 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.
Attachments
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.
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.
Attachments
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:
as the equivalent of
(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
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:
Code:
new rec(1 2 3 4 5 6);
Code:
new rec(1, 2, 3, 4, 5, 6);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
Attachments
Yes.should this instead be an error?
(I imagine it won't be too hard to change your RequestLogger.printLine to a "throw this.parseException(",", this.currentToken());")
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"
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"
This logic makes tests do what I would expect:
I'll take a closer look at the rest of the patch tomorrow.
Java:
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.
Last edited:
Is this better?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.
Not sure what you mean here.I would expect the logic to look something like "read comma before reading expression, if we've already parsed something"
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?
Attachments
Yes.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"
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).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
The atEndOfFile() check in your latest patch is incorrect, as it yields that error message in the case of
C-like:
record r { int a;} new r(4;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.)
My bad. When I turned this from a warning to an error, tunnel vision made me not consider just... changing the message.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).
This one fixes this, and addresses that.(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.)
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.
Attachments
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.
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.
Attachments
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.
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.
Attachments
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.
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.
Attachments
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:
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:
- 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.
- 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...)
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.Meanwhile, it's time to submit/present the libraries that we'll be using.
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".Why do we want library sources?
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.
lib/jar: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.
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...))
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)?These are interfaces that we will implement, and classes that we will use (most notably, the "Location" and the "Range").
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?