Feature ASH language server features

[fredg1]

Can you add a patch for the null check here? I'd prefer not to have intermediate states that change functionality, even if they make the diff slightly longer.

Sure, here...

It doesn't conflict with the error_conditionals_1

It's a simple addition of a null check.
(The other conditions *still check for null*. That's intentional, because, again, the code will end up moving forward instead of throwing an error.)

 

[heeheehee]

Thanks. That's basically identical to the change I had locally (well, you added the word "when"). I'll merge that in together with the error conditionals patch.

Working through the diff in parseJavaFor now...

 

[heeheehee]

r20883 contains error_conditionals_1 and parseReturn_null_check. As test coverage goes up, these reviews should get faster as I end up needing to add fewer auxiliary tests to exercise weird edge case behavior.

(And, of course, if you want to supply the tests in the first place, that makes my job even easier.)

 

[fredg1]

Here's the 2nd one, which is way shorter.

One thing you'll probably notice is something in parseAggregateType:

        else if ( this.parseIdentifier( this.currentToken().content ) )
        {
            indexType = scope.findType( this.currentToken().content );

            if ( indexType != null )
            {
/*>>>*/         if ( !indexType.isPrimitive() )
                {
                    throw this.parseException( "Index type '" + this.currentToken() + "' is not a primitive type" );
                }
            }
            else
            {
                throw this.parseException( "Invalid type name '" + this.currentToken() + "'" );
            }

            this.readToken(); // type name
        }

You'll most likely wonder if there's a way to not have these nested "if"s. The answer is no.
A later patch will add code between the two, like so:

            if ( indexType != null )
            {
                [...]

                if ( !indexType.isPrimitive() )
                {
                    throw this.parseException( "Index type '" + this.currentToken() + "' is not a primitive type" );
                }
            }

which is why they need to be (or, I guess, "currently appear") nested.

 

[heeheehee]

More surprising existing behavior:

record r {void a; void b;}; new r(,);

is a valid parse, I guess it was a conscious decision to make an empty param skip a field, if you didn't want to specify some placeholder value. Undocumented, though.

I'm still not sure why you'd want to define a void variable. A symbol with no value?

(Tangentially related: one of my minor pet peeves is when people conflate "arguments" and "parameters". But, it happens frequently enough that I don't fight it anymore.)

r20889.

 

[fredg1]

@heeheehee clover doesn't like multiline strings

            {
                "Basic function with forward reference",
                // vvvv line 478 vvvv
                """
                void foo();
                void bar() { foo(); }
                void foo() { return; }
                bar();
                """,
                null,
                Arrays.asList( "void", "foo", "(", ")", ";",
                               "void", "bar", "(", ")", "{", "foo", "(", ")", ";", "}",
                               "void", "foo", "(", ")", "{", "return", ";", "}",
                               "bar", "(", ")", ";" ),
            },

   [clover] C:\Users\Frederic\Desktop\KoLmafia-workspace\kolmafia\test\net\sourceforge\kolmafia\textui\ParserTest.java:478:8:expecting '"', found '\n'
   [clover] C:\Users\Frederic\Desktop\KoLmafia-workspace\kolmafia\test\net\sourceforge\kolmafia\textui\ParserTest.java:478:8:expecting '"', found '\n'
   [clover] ** Error(s) occurred and the instrumentation process can't continue.

 

[fredg1]

Small patch which removes a redundant condition in parseInvoke

            if ( !this.parseIdentifier( current.content ) )
            {
                throw this.parseException( "Variable reference expected for function name" );
            }

            name = this.parseVariableReference( scope );

            if ( !( name instanceof VariableReference ) )
            {
                throw this.parseException( "Variable reference expected for function name" );
            }

parseVariableReference checks for if ( !this.parseIdentifier( this.currentToken().content ) ) at its very start, returning null if true, which ends up satisfying the conditional places afterwards, which has the same error message, making the first one redundant.

 

[heeheehee]

clover doesn't like multiline strings

no, that's a java 13 feature (finalized in java 15), and for reasons our codebase is still building java8 sources / targets

 

[fredg1]

no, that's a java 13 feature (finalized in java 15), and for reasons our codebase is still building java8 sources / targets

oh, ok...

Anyway, here's some more tests/coverage

 

[fredg1]

@heeheehee The coverage... it's over NINE T(y p)E(r)CEEEEEEEEEEN(t)

(94,2% )

 

[fredg1]

Patch which adds documentation to parseAggregateLiteral, and which moves the reading of the opening bracket ("{") inside of it (it was previously necessary to read it *before* entering the method).

This is done because, later on, every value/symbol/...thing will be assigned a "location", and the opening bracket is part of aggregate literals' location.

 

[fredg1]

Patch which removes the second parameter of parseType( final BasicScope scope, final boolean aggregates, final boolean records ), i.e. "aggregates".

This parameter was used to determine whether or not declaring aggregates was allowed, but ever since the parsePluralConstant change, cases where they are prohibited no longer exist.

 

[fredg1]

Patch which streamlines parseSingleCommandScope, removing the need for Scope( final ParseTreeNode command, final BasicScope parentScope ).

 

[fredg1]

Patch which streamlines parseCatchValue.

 

[fredg1]

Patch which streamlines parseStatic.

This is possible because of how parseCommandOrDeclaration actually simply returns the same scope it was supplied as his first parameter, making

Scope scope = [...];
Type type = [...];

return this.parseCommandOrDeclaration( scope, type );

and

Scope scope = [...];
Type type = [...];

this.parseCommandOrDeclaration( scope, type );
return scope;

functionally identical.

 

[fredg1]

Patch which delegates the discovery/reading of the initial token of parseNewRecord and parseTypedConstant to these very methods, rather than having to do it *before* going in, conforming to the rest of the method calls in parseValue.

 

[fredg1]

The methods parseRecord, parseFunction, parseAggregateLiteral, parseBasicScript, parseNewRecord and parseParameters all do something like this:

while ( !this.currentToken().equals( ")" ) )
{
    [...]
}

this.readToken(); // read )

which assumes that the loop will always be exited while in front of the wanted token.

However, with the current goal being to keep parsing even when things don't go as expected, this assumption no longer always hold.

This patch fixes this by putting the reading inside the loops.

 

[heeheehee]

(I'll try to get to these either tomorrow or the day after. Thanks for bearing with me.)

 

[fredg1]

Currently, if you submit both a File and an InputStream to Parser( File, InputStream, Map ) (not that it's currently done), the InputStream will be discarded in favour of the File (in part because it is assumed that both are never submitted together).

With the language server, this assumption no longer hold; it is possible to submit a File, since we know where the file is, but also an InputStream, as its content was modified.


Additionally (while I'm here), there is currently a bug regarding imports. If you submit a File to the method, that file isn't added to the imports, nor is there checks before importing files, allowing that same file to be imported a second time.

This patch fixes these things (test added)

 

[heeheehee]

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.