Labeled statements have wrong source position

After the GROOVY-4071 fix, the labeled asserts don't get rendered correctly.
For the code snippet

mylabel:
assert true == false

Here is how it looks now:

Caught: Assertion failed: 

:


	at Try.run(Try.groovy:2)

Should a separate JIRA be opened for this assert message rendering issue?

As noted in GROOVY-4071, both the source location and message rendering issues are interlinked and once we get the source position right on the AssertStatement, the error message should get its empty portions filled again.

Hi Jochen, you seem to be back on the mailing lists, so here is a pending question for you.

I was comparing the source position of assert statement case with some other statements and noticed that for other cases the position of the ':' token was getting set on the statement but the actual position was getting carried by the expressions inside them. Whereas in case of AssertStatement, it was having an impact because Power-Assert code uses the source position from the AssertStatement itself, which is wrong.

So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression.

Do you think it is a worthwhile thing to try that change? Or, do you see a simpler solution?

> So, by comparison, one potential solution could be to have a AssertExpression wrapped inside a AssertStatement, and make assert code use the source position from AssertExpression instead of AssertExpression.
Not sure what you are saying here, but why not just fix the source position of labeled statements? It's never good to have wrong source positions in the AST.

For now, I am just checking it as an alternative to avoid modifying the ANTLR AST to groovy AST conversion code. I haven't checked it yet, but I think there could be an overlap in the AST formation/conversion code with the map key:value scenario, and I want the changes to be as confined as possible.

What does making an expression fix the issue? Also such a change has the problem of possibly breaking code, or not?

@Jochen, If instead of AssertStatement we have ExpressionStatement wrapping a AssertExpression, then initially both AssertExpression/ExpressionStatement have the same source location, then when ":" token sets its source information on the ExpressionStatement, the wrapped AssertExpression remains isolated and still continues to have the correct source information. That's how it is working fine for various other labeled expressions.

Regarding the possibility of breaking code, yes, it will impact the users that hook deep into groovy compiler.

Let me know if you think that this direction of solving it is not worth exploring. The only reason was to avoid AST formation code because the place where ":" sets its source information on the following statement seems like a generic place.

Well, then no AssertStatement wrapping a AssertExpression would be needed, instead ExpressionStatement could warp it... still.. What you write here will essentially be a problem for all statements, or not? I prefer fixing the issue in the current setup, to not to break code

AssertStatement wrapping a AssertExpression was a typo that I later corrected - I did mean ExpressionStatement doing the wrapping.

A basic doubt - does the source information on the statement matter much? Let's take some example - of a ForStatement or BlockStatement or IfStatement, etc - is it not good enough in general if the expressions carry the source information correctly? When does the source info on the statements matter? Don't statements act as holders for expressions?

It is surely correct that all the labeled statements are affected - but I couldn't reproduce a usage highlighting an issue with some other labeled statement - probably because ACG seems to be taking correct source information from the expressions, or because I am not clear where statement source info really matter.

I am not trying now to push that we should go the AssertExpression way - just trying to understand how much and where Statement source info matters.

May be it is best to solve this issue through some grammar or AST formation change.

> A basic doubt - does the source information on the statement matter much?
I can't see why the source position of a statement should be less important than the source position of an expression.

if {
foo()
} else {
bar()
}

No one expression captures the extents of the statement.

> When does the source info on the statements matter?
For example, power asserts intentionally use the assert statement's source position s.t. the full statement appears in the output, and not just the boolean expression. Spock also uses statement source positions.

>or because I am not clear where statement source info really matter
As long as statements have a source position, it should be correct. Are you proposing to remove source position for statements?

Peter - I am not saying one is less important or the other is more, nor I am proposing removal of source information from statements. Why do you have to read between the lines for no reason? I am just trying to become clearer about their use.

My doubt is about what you totally forgot/skipped to explain in the if-else that you typed - that if source information of 'if boolean expression' or 'if block expressions', or 'else block expressions' are all intact - then I am just not clear how the source info on the if BlockStatement or else BlockStatement or IfStatement matters.

I can ask questions about what I am not clear about, can't I?

Maybe it's too silly a question. Let's please move on. Maybe for tools/IDEs/other frameworks source information on statements is just no less important than the expressions. May for general programming usage, source information on expressions is more visible in error messages and so, and may be that's what was confusing me.

I agree that it's best to get the source information correct on the statements. Let's move on to see how we can solve it now.

Thanks.