Feature ASH language server features

fredg1

Member
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)?
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)

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?
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.

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?
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.
 

heeheehee

Developer
Staff member
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.
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

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.
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.
 

fredg1

Member
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
WTF are those...

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
Those are just used for the "construction" of their jars (they use XText)

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.
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...
 

heeheehee

Developer
Staff member
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.
 

fredg1

Member
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 }
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.

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.)
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
 

heeheehee

Developer
Staff member
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.
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.
 

heeheehee

Developer
Staff member
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.
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.)
 

fredg1

Member
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.)
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.
 

heeheehee

Developer
Staff member
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?)
 

heeheehee

Developer
Staff member
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]);
 

fredg1

Member
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]);
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...
 

heeheehee

Developer
Staff member
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

  • recordCoercion.patch
    1.1 KB · Views: 2

fredg1

Member
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...
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.
 

heeheehee

Developer
Staff member
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).
 

fredg1

Member
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:

fredg1

Member
Update: pruned the "modify the signature of CompositeType.getDataType( Object )" part of the patch. I'll look into that issue an other time.
 

Attachments

  • parsetree_abstracts_2.patch
    3.4 KB · Views: 0

fredg1

Member
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)
 

heeheehee

Developer
Staff member
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.)
 
Top