Feature ASH language server features

heeheehee said:
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.
Click to expand...
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.)
 

Attachments

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...
 
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.)
 
Here's the 2nd one, which is way shorter.

One thing you'll probably notice is something in parseAggregateType:
Java: 
        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:
Java: 
            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.
 

Attachments

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.
 
@heeheehee clover doesn't like multiline strings :(
Java: 
            {
                "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", "(", ")", ";" ),
            },

Code: 
   [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.
 
Small patch which removes a redundant condition in parseInvoke

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

Attachments

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.
 

Attachments

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.
 

Attachments

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
Java: 
Scope scope = [...];
Type type = [...];

return this.parseCommandOrDeclaration( scope, type );
and
Java: 
Scope scope = [...];
Type type = [...];

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

Attachments

The methods parseRecord, parseFunction, parseAggregateLiteral, parseBasicScript, parseNewRecord and parseParameters all do something like this:
Java: 
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.
 

Attachments

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)
 

Attachments

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.
 
You must log in or register to reply here.
Back
Top