Feature ASH language server features

heeheehee said:
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)?
Click to expand...
generator is just a dependency of the other lsp4j jars
lsp4j.jsonrpc is way less used than lsp4j. You can point to it if you want, but what we use from it is be simple enough that you can understand what it is from context.
Where it could help would be for those trying to dig into how lsp4j works. (its inner workings and the likes)

heeheehee said:
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?
Click to expand...
If you mean lsp4j.debug, lsp4j.jsonrpc.debug, javax.websocket, jakarta.websocket, lsp4j.websocket and lsp4j.websocket.jakarta, then yeah, those are not relevant here.

heeheehee said:
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?
Click to expand...
No. There is way more than just auto-completing interfaces and constructors. One of the main reason to keep the source is how lsp4j's methods (that we will use for the language server) use lsp4j classes, which *we* need to instantiate.
Many of them have multiple constructors, because they have optional arguments.

That, on top of being supplied some of those classes, which we need to figure out how to use.
 
fredg1 said:
If you mean lsp4j.debug, lsp4j.jsonrpc.debug, javax.websocket, jakarta.websocket, lsp4j.websocket and lsp4j.websocket.jakarta, then yeah, those are not relevant here.
Click to expand...
I actually meant:

animal-sniffer-annotations-1.17.jar
checker-qual-2.5.2.jar
error_prone_annotations-2.2.0.jar
failureaccess-1.0.1.jar
j2objc-annotations-1.1.jar
jsr305-3.0.2.jar
listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar
org.eclipse.xtend.lib-2.24.0.jar
org.eclipse.xtend.lib.macro-2.24.0.jar
org.eclipse.xtext.xbase.lib-2.24.0.jar

fredg1 said:
No. There is way more than just auto-completing interfaces and constructors. One of the main reason to keep the source is how lsp4j's methods (that we will use for the language server) use lsp4j classes, which *we* need to instantiate.
Many of them have multiple constructors, because they have optional arguments.

That, on top of being supplied some of those classes, which we need to figure out how to use.
Click to expand...
Isn't that what javadocs are for? You shouldn't need to see the implementation to know how to use these interfaces, but rather have a handle on their inputs / outputs.

Similarly, I would expect that you can build an implementation by directly interfacing the classes that contain the interfaces.
 
heeheehee said:
I actually meant:

animal-sniffer-annotations-1.17.jar
checker-qual-2.5.2.jar
error_prone_annotations-2.2.0.jar
failureaccess-1.0.1.jar
j2objc-annotations-1.1.jar
jsr305-3.0.2.jar
listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar
Click to expand...
WTF are those...

heeheehee said:
org.eclipse.xtend.lib-2.24.0.jar
org.eclipse.xtend.lib.macro-2.24.0.jar
org.eclipse.xtext.xbase.lib-2.24.0.jar
Click to expand...
Those are just used for the "construction" of their jars (they use XText)

heeheehee said:
Isn't that what javadocs are for? You shouldn't need to see the implementation to know how to use these interfaces, but rather have a handle on their inputs / outputs.

Similarly, I would expect that you can build an implementation by directly interfacing the classes that contain the interfaces.
Click to expand...
I have to admit that I've never really used javadocs.
Mostly because I use VScode (with the java extension(s)), and I don't know if it even recognizes them, let alone is compatible with them.
All I've ever known are the "go to definition", "go to implementations" and "go to references" links...
 
Regarding parseNewRecord_initializers...3: does it make sense to change
Java: 
while ( true )
{
    if ( this.atEndOfFile() )
    {
        throw this.parseException( "}", this.currentToken() );
    }

    if ( this.currentToken().equals( "}" ) )
    {
        if ( fieldTypes.isEmpty() )
        {
            throw this.parseException( "Record field(s) expected" );
        }
            this.readToken(); // read }
        break;
    }
    ...
}
to this?
Java: 
while ( !this.currentToken().equals( "}" ) )
{
    if ( this.atEndOfFile() )
    {
        throw this.parseException( "}", this.currentToken() );
    }
    ...
}

if ( fieldTypes.isEmpty() )
{
    throw this.parseException( "Record field(s) expected" );
}

this.readToken(); // read }

I personally feel that the construction

while (true) { if (bar) { break; } ... }

is an antipattern that can be more simply expressed as

while (!bar) { ... }

(I also feel that while (true) loops are an antipattern in themselves, but don't feel quite so strongly about that.)

I can make this change on my own, no need to supply a v4 patch. Just wanted to make sure it's compatible with your later changes.
 
heeheehee said:
Regarding parseNewRecord_initializers...3: does it make sense to change
Java: 
while ( true )
{
    if ( this.atEndOfFile() )
    {
        throw this.parseException( "}", this.currentToken() );
    }

    if ( this.currentToken().equals( "}" ) )
    {
        if ( fieldTypes.isEmpty() )
        {
            throw this.parseException( "Record field(s) expected" );
        }
            this.readToken(); // read }
        break;
    }
    ...
}
to this?
Java: 
while ( !this.currentToken().equals( "}" ) )
{
    if ( this.atEndOfFile() )
    {
        throw this.parseException( "}", this.currentToken() );
    }
    ...
}

if ( fieldTypes.isEmpty() )
{
    throw this.parseException( "Record field(s) expected" );
}

this.readToken(); // read }
Click to expand...
Putting the "records field(s) expected" check/error outside the loop? Yes.

The rest? No.

This is the exact point of those latest few changes; there will be more ways to exit the loops so we can't assume there's a specific token waiting for us.

heeheehee said:
I personally feel that the construction

while (true) { if (bar) { break; } ... }

is an antipattern that can be more simply expressed as

while (!bar) { ... }

(I also feel that while (true) loops are an antipattern in themselves, but don't feel quite so strongly about that.)
Click to expand...
The "true" constant is actually a placeholder!
This may be why it looks off.

The value that will end up going there is a method making sure we don't get stuck in an infinite loop.Code_Vd1j9WSJA0.png
 
fredg1 said:
The "true" constant is actually a placeholder!
This may be why it looks off.

The value that will end up going there is a method making sure we don't get stuck in an infinite loop.
Click to expand...
Acknowledged, which is why I wanted to check that this was just a placeholder intermediate state.

The patch looks fine otherwise; I'll try to commit some more of these changes later today.
 
fredg1 said:
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.
Click to expand...
Something that's always bothered me about record handling in ASH is that anonymous records can't be directly stored in variables without serializing / deserializing via map_to_file / file_to_map, or using constructs like foreach loops. This is especially visible with built-in functions like item_drops_array.

I suspect this would be a matter of allowing (anonymous?) records to coerce to each other provided they have the same field types + names, although I would then expect anonymous record function parameters to be allowed.

(This is a longwinded way of saying, I wish we could do this, so I'm hesitant to codify this as illegal behavior.)
 
heeheehee said:
Something that's always bothered me about record handling in ASH is that anonymous records can't be directly stored in variables without serializing / deserializing via map_to_file / file_to_map, or using constructs like foreach loops. This is especially visible with built-in functions like item_drops_array.

I suspect this would be a matter of allowing (anonymous?) records to coerce to each other provided they have the same field types + names, although I would then expect anonymous record function parameters to be allowed.

(This is a longwinded way of saying, I wish we could do this, so I'm hesitant to codify this as illegal behavior.)
Click to expand...
Mmh... We could change the error message to say "Record creation is not currently allowed here"? So that people don't get a "this isn't possible, and will never be" vibe from the error message?

Other than that, it's more of a "trying to clarify what is going wrong here" than a "here, let me show you how we want you to fall in line" thing (if that makes sense).
In other words, the error message shows that we *can* see that they are making a record (unlike the previous error message), but we're (at least currently) not supporting it.
 
I think in general I'd prefer a little more context than "not allowed here" -- perhaps "not supported in function declarations"?

Either way, I think I'm nitpicking on the text of the error message itself. (While I'm nitpicking there: is "record creation" the right wording? Record type definition?)
 
The anonymous record situation evidently has more nuance:

Entirely valid ASH:
Code: 
record {int a;} a;
record {string a;} b;
a = b;

Runtime error:
Code: 
record {int a;} a;
record {int a;} b;
a = b;
print(a.a);

Parse error:
Code: 
record a{int a;};
record {int a;} b;
b = new a();
(or other variations thereof, just because the names don't line up.)

Also a parse error:
Java: 
record {item drop; int rate; string type;}[] drops = item_drops_array($monster[beanbat]);
 
heeheehee said:
The anonymous record situation evidently has more nuance:

Entirely valid ASH:
Code: 
record {int a;} a;
record {string a;} b;
a = b;

Runtime error:
Code: 
record {int a;} a;
record {int a;} b;
a = b;
print(a.a);

Parse error:
Code: 
record a{int a;};
record {int a;} b;
b = new a();
(or other variations thereof, just because the names don't line up.)

Also a parse error:
Java: 
record {item drop; int rate; string type;}[] drops = item_drops_array($monster[beanbat]);
Click to expand...
That *does* seem interesting...

If we were to look into the future, at a point were it was possible to coerce records, one may want to make a method where they have something like
Code: 
record foo
{
    string a;
    int b;
    float c;
}

record bar
{
    string a;
    item d
    location e;
    monster f;
}

void f( record { string a; } param )
{
    [...]
}

where f would accept any record with a string a field, which, here, could accept either foo or bar...
 
The records-in-function-prototypes is evidently a very conscious decision for the time being (per second arg passed to parseType), likely because it was deemed unusable (as a result of anonymous records not being meaningfully coercible to each other).

Apparently the second case (runtime error on print(a.a)) is because parsetree/RecordValue.java is trying to look up the key, and not finding it, because parsetree/RecordType.java uses == instead of .equals().

There's more nuance around making sure that we can't just assign any anonymous records to each other, so I'm not just going to commit that without thinking about it harder... but it is a small patch...
 

Attachments

heeheehee said:
The records-in-function-prototypes is evidently a very conscious decision for the time being (per second arg passed to parseType), likely because it was deemed unusable (as a result of anonymous records not being meaningfully coercible to each other).

Apparently the second case (runtime error on print(a.a)) is because parsetree/RecordValue.java is trying to look up the key, and not finding it, because parsetree/RecordType.java uses == instead of .equals().

There's more nuance around making sure that we can't just assign any anonymous records to each other, so I'm not just going to commit that without thinking about it harder... but it is a small patch...
Click to expand...
I think we should put this aside for now, like parsePostCall. Mainly, because IIRC, boolean Type.equals(Type) is used to find *that exact* type (because we'll end up having to find types/variables/functions in Scopes' TypeList/VariableList/FunctionList, to fetch their references/definition locations, and we don't want to get the wrong one), so we'll most likely have to add an additional method that will be called in parseFunction, instead.
 
I agree. It was an aside of "hey record coercion is totally broken, name is not the right thing to check considering unnamed records"

I'll try to get around to some more patches later this week (or if you'd prefer to post a pull request on github, I can review code there, with the understanding that nothing will get merged until after the switch).
 
A few appetizers while we wait for the rest:

- The classes CompositeType, CompositeValue and AggregateValue should be made abstract.

- The method CompositeType.getDataType( Object ) can be made abstract.
Additionally, its parameter can be turned into a Value (which cascades into doing the same thing for AggregateType.getDataType( Object ), RecordType.getDataType( Object ) and CompositeValue.initialValue( Object ) )

- Rather than having Type.toString() and RecordType.toString() both be return this.name, just make Symbol.toString() { return this.name; } (this also incidentally adds the method to Variable and Function, which helps in the debugger)


EDIT: currently causes an error in the tests. I'm trying to find out the source.
 
Last edited:
May I get a heads up on the situation? Is it that we're waiting for the transition to github to continue, or is it that A) you're busy with something else (dealing with bugs being more important than new features), or B) there's a problem with the patches/something missing? (or C), something I didn't think of)
 
A few things, but here are the main ones:

1. We're internally discussing whether we'd like to spin one last point release before making the transition to github, and possibly dropping point releases entirely. We'd like to make the source as stable as reasonably possible before that. (So, bug fixes instead of new features.) Even if we don't create said release, we'd still like to iron out any kinks so that when the transition happens, we can focus on it at least for a few days, rather than diverting our attention to deal with more urgent bugs / regressions.

2. We hope that moving to github will allow for a smoother, better-integrated review flow for both of us. I encourage you to migrate these changes to a pull request, including the extant patches that you've posted (since i won't review them in this thread before then). We can start the review process there before the actual transition, with the understanding that no PRs will be accepted prior to the switch.

In particular, being able to conduct conversations on the actual commits is much easier than quoting the relevant bits of code, and it's much easier to provide appropriate context.

Additionally, tracking all the patches and making sure I don't miss anything is a non-issue under that model.

3. Regarding pulling in new jar dependencies: I'd prefer not to add them until we start actually using them. When we fully deprecate and remove the Ant build, it'll be trivial to add the new dependencies to the Gradle build, when needed. I think it may ultimately make sense to define a separate source set in the Gradle build so we don't pull in language server dependencies (and ultimately bloat the jar) for users who aren't otherwise using it. I think the Parser refactors thus far are still broadly relevant, but once we start adding LSP support, that should be bundled separately.

(Fixing broken record coercion behavior is unrelated to this change, and was an unnecessary aside. Sorry about that. But, that said, I'd like not to enshrine any of that bad behavior in tests, because I think it might be broadly useful to allow coercion of records that identically match field names + types.)
 
You must log in or register to reply here.
Back
Top