Feature ASH language server features

[fredg1]

Here's the first batch.

There's a lot of them, but again, this is just because I'm splitting them as small as possible. I'm currently making 1 patch per method.
If I'm going too small, just say it.

These are, as noted above, only small conditional changes. They are independent from each other, and can be committed in any order, together or separately (if you choose to commit them together, I'd appreciate if you were to split them into groups and commit those as you go, rather than waiting until the very, very end, so that I can tell if you're slacking off resume making changes on these methods)

 

[fredg1]

In my limited parsing implementation experience there is a point where Aborting is more helpful to the user than trying to continue. The classic example is cascading errors. But there are also cases where an error is isolated and blowing past it allows the user to potentially fix more than one thing when parsing is finished.

So, does always reach EOF mean in the language server context there is no benefit to aborting early?

some methods will abort early, because they know too little about what's in front of them to know if it's even worth trying to match what it is with what is "expected", but if you mean aborting the whole parsing, no, there's no benefit to that.

Well, actually, yes, there's one. From the language server's perspective, if we are parsing a file, and receive a new version of that file while we are still parsing the previous version, then yes, we should abort early (which will be handled in a future step).

 

[fredg1]

Well, actually, yes, there's one. From the language server's perspective, if we are parsing a file, and receive a new version of that file while we are still parsing the previous version, then yes, we should abort early (which will be handled in a future step).

Also, if we discover that one of the file we're currently parsing is being imported by another file/script.
This is a behaviour/necessity I am hoping to get rid of, but currently, yes, it's a reason to interrupt the parsing.

 

[fredg1]

2nd batch. Same thing.

 

[fredg1]

3rd batch. Same thing. Again.

 

[heeheehee]

the goal being to making it easier to turn it into

Ah, thanks for that context. Generally the former notation reduces cognitive load (per an earlier comment I think I made about guard statements). Another way around that would be to extract those two checks into a helper function, but that's potentially more unpleasant to work with if you rely on a lot of function-local state at that point.

So, does always reach EOF mean in the language server context there is no benefit to aborting early?

I think fredg1 already responded to this to some extent, but I had already typed up this response, so here you go.

On the command-line, you often do want to avoid cascading errors. (Although, even then there's sometimes an argument for knowing about all the failures, so you can fix them without using `ant build` in your coding loop. For instance, if I add a dozen test cases, I might want to run all these at the same time so I can figure out what the actual expected text should be.)

But suppose you're working in Eclipse or Netbeans or IntelliJ or whatever. You probably want it to tell you about all your syntax errors, rather than just the first one. That's one place where being able to "recover" the Parser comes in handy.

 

[heeheehee]

If I'm going too small, just say it.

I think it's okay to group patches if they're logically doing the same thing, and I can mentally pattern-match that thing from a quick skim.

(e.g. foo == currentToken() -> currentToken() == foo)

When I'm having to context-switch between three or four patterns scattered throughout the file, and also dealing with some nontrivial implementation changes on top of that, that's when I'll start asking you to break things up.

 

[fredg1]

I think it's okay to group patches if they're logically doing the same thing, and I can mentally pattern-match that thing from a quick skim.

(e.g. foo == currentToken() -> currentToken() == foo)

When I'm having to context-switch between three or four patterns scattered throughout the file, and also dealing with some nontrivial implementation changes on top of that, that's when I'll start asking you to break things up.

Sooooo... was that a "it's better if you do it like this next time(s)", or was that a "could you please re-submit all of these as a single patch / group them"?

 

[heeheehee]

could you please re-submit all of these as a single patch / group them

Ideally this one, so I don't have to manually download and apply a couple dozen different patches.

I haven't forgotten about this, just been busy lately.

 

[fredg1]

Ideally this one, so I don't have to manually download and apply a couple dozen different patches.

Here

 

[heeheehee]

Thanks. Let's see what patches have yet to be submitted:

- #105: remove parsePostCall
- #158: four patches. I've reviewed and locally committed the first one (fix unwanted plural constant behavior), will look at the others shortly. Steps 2 and 4 look comparable to the error_conditionals patch. But it looks like I can't pull out step 3 to commit before the others, since these four steps depend on each other sequentially. I'll review them one at a time, that's fine.
- #170: error_conditionals_1.patch that you've helpfully consolidated.

How crucial is removing parsePostCall?

 

[heeheehee]

Parts 1-3 of parseAggregateType are now committed as r20867 - 20869. I figured I'd push that before I take another look at part 4, since it seems more involved than parts 2 / 3.

 

[heeheehee]

Impressive, the only (tested) behavior that changed from part 4 resolved an inconsistency in error messages that I had noted when adding tests earlier.

Is there value in consolidating the first branch:

if ( this.currentToken().equals( "]" ) )
{
    if ( !separatorToken.equals( "[" ) )
    {
        throw this.parseException( "Missing index token" );
    }
}

and the last one

else
{
    throw this.parseException( "Missing index token" );
}

as such:

else if ( !this.currentToken().equals( "]" ) || !separatorToken.equals( "[" ) )
{
    throw this.parseException( "Missing index token" );
}

? (per a simple application of De Morgan's laws -- !(a && b) => !a || !b)

Or the potentially more readable option:

else if ( this.currentToken().equals( "]" ) && this.separatorToken.equals( "[" ) )
{
}
else
{
    ...

I don't feel strongly about this one, just observing that if we really wanted to eliminate duplicate code, this is an option...

 

[fredg1]

How crucial is removing parsePostCall?

Looking at my finished/destination version... it definitely seems possible to work around bringing it back.

In an earlier version, there was a point where it was needed, but where I'm at now, no, it wouldn't really be a "problem".
It's really just redundant code that I can't fathom the point of.

I guess we could even just post-pone this. Wait until all this is nearly finished, or something (as in, wait for that to talk about the subject again, I'm not implying that I'll try to force that patch anyway "one day or another"). It's not that big a deal.

 

[fredg1]

Is there value in consolidating the first branch:

if ( this.currentToken().equals( "]" ) )
{
    if ( !separatorToken.equals( "[" ) )
    {
        throw this.parseException( "Missing index token" );
    }
}

and the last one

else
{
    throw this.parseException( "Missing index token" );
}

as such:

else if ( !this.currentToken().equals( "]" ) || !separatorToken.equals( "[" ) )
{
    throw this.parseException( "Missing index token" );
}

? (per a simple application of De Morgan's laws -- !(a && b) => !a || !b)

Or the potentially more readable option:

else if ( this.currentToken().equals( "]" ) && this.separatorToken.equals( "[" ) )
{
}
else
{
    ...

I don't feel strongly about this one, just observing that if we really wanted to eliminate duplicate code, this is an option...

No, not really.

While both currently only lead to a simple exception throwing, the latter will end up causing the method to be exited early, since we can't tell if we're still parsing an aggregate. With the former, we know we're still in one.

 

[heeheehee]

I guess we could even just post-pone this. Wait until all this is nearly finished, or something (as in, wait for that to talk about the subject again, I'm not implying that I'll try to force that patch anyway "one day or another"). It's not that big a deal.

Yeah. I'd rather hold off on it for the time being, assuming that it's not tightly coupled to the rest of the changes.

Anyways, part 4 of parseAggregateType is now r20871. I'll try to tackle the error_conditionals patch tomorrow.

 

[heeheehee]

I haven't forgotten about this. Looking at error_conditionals_1.patch again today, but there are lots of untested codepaths that this touches. It looks okay from inspection, although I'd like to add tests to confirm that behavior is unchanged, especially since there are some miscellaneous unrelated changes in this patch. Nothing that looks incorrect, but better safe than sorry.

I'm in the middle of parseCommand right now.

 

[heeheehee]

One bit of functionality change that I've found so far: `return;` as a top-level construct will become valid as a result of error_conditionals.

Previously it yielded a not-so-helpful error of "Return needs null value" (because the corresponding function type was, um, not populated and so defaulted to null).

I believe it's a result of this transformation:

-            if ( expectedType != null && expectedType.equals( DataTypes.TYP
E_VOID ) )
+            if ( expectedType != null && !expectedType.equals( DataTypes.TY
PE_VOID ) )
             {
-                return new FunctionReturn( null, DataTypes.VOID_TYPE );
+                throw this.parseException( "Return needs " + expectedType +
 " value" );
             }
 
-            throw this.parseException( "Return needs " + expectedType + " v
alue" );
+            return new FunctionReturn( null, DataTypes.VOID_TYPE );
         }

(I'm taking a break at parseSort... roughly a third of the way through the patch now, I think)

 

[fredg1]

One bit of functionality change that I've found so far: `return;` as a top-level construct will become valid as a result of error_conditionals.

Previously it yielded a not-so-helpful error of "Return needs null value" (because the corresponding function type was, um, not populated and so defaulted to null).

Yeah, sorry about that. I put the null check earlier in the method, but didn't add it because I wanted this patch to not have (too many) extraneous stuff, since it's pretty big.

It'll be in the next one, which will be way shorter.

 

[heeheehee]

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.