RFR 8172443: Change use of tree.pos to line:col in rawDiagnostics

[Maurizio Cimadamore]

On 05/10/17 15:27, Vicente Romero wrote:
>
>
> On 10/05/2017 10:12 AM, Maurizio Cimadamore wrote:
>>
>>
>> On 05/10/17 15:05, Vicente Romero wrote:
>>>
>>>
>>> On 10/05/2017 09:51 AM, Maurizio Cimadamore wrote:
>>>>
>>>>
>>>> On 05/10/17 14:49, Vicente Romero wrote:
>>>>> Hi,
>>>>>
>>>>> +100 to this effort, that will save us a lot of time. Regarding 
>>>>> the patch, sometimes new lines need to be added to a test, 
>>>>> sometimes in the jtreg header, sometimes somewhere else. How bad 
>>>>> would be to use the column number only instead of line:col?
>>>> I understand the concern. I think the goal of this patch is to fix 
>>>> the #1 offender. As you say there could be other offenders. But 
>>>> getting rid of them all is hard - if you just use 'col' you 
>>>> basically lose uniqueness, which makes looking at the golden file 
>>>> more obscure, I think.
>>>
>>> well it's true that uniqueness could be a problem for errors 
>>> reported at statements that spans more than one line, but I'm not 
>>> sure if it is worthy to still keep a source of brittleness because 
>>> of that. I mean what could be the probability of introducing a 
>>> change that will swap two identical column positions in the 
>>> diagnostics and still produce the same diagnostics such that we 
>>> won't detect it. In the worst case we can add the line if we see 
>>> that the particular statement we are generating the diagnostics for 
>>> spans more than one line and use only the column for all other cases
>> I suggest with stick with the proposed approach and then see how bad 
>> it is in the real world?
>
> I think that the proposed approach will be better than the current 
> situation, that's for sure, but I think that this is the right moment 
> to consider if the proposed approach can be still better. If you want 
> to go for it with the current version, I'm OK with that but you know 
> how long it takes for stuff like this to be revisited.
I believe the ultimate fix for these kind of issues would be to revisit 
the code so that instead of generating line or column info, we generate 
a position that is relative to the start of the enclosing statement. In 
a way this generalizes over the 'using only col' idea, and works even 
for functional expressions scattered across multiple lines (but part of 
the same statement).

That said, doing that would not only require changes to the raw 
diagnostic backend (as done here), but would need deeper changes in how 
functional expression trees are communicated to the diagnostic backend. 
Right now we just pass trees as diagnostic arguments. Trees do not 
provide any contextual info (e.g. what is the parent of the tree?) - so 
to do this kind of treatment (and also the treatment you suggest where 
we only use column 'if needed') requires a contextual info that the 
current diag arguments do not have.

While this is all solvable, as with all fixes/enhancements, it's about 
trying to achieve some kind of compromise between complexity and 
benefits. Having fixed many source pos related issues, I can say with 
confidence that this patch addresses almost all of them.

When line number changes in a test file (e.g. because more stuff is 
added), your golden file will have to change anyway, as all the errors 
generated _after_ the insertion are going to have stale info in the 
golden file. That's why I did not prioritize the 'vertical' adjustments 
so much (as they are typically breaking anyway - modulo few lucky cases).

Maurizio
>
>>
>> Maurizio
>
> Vicente
>
>>>
>>> Vicente
>>>
>>>>
>>>> Maurizio
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> On 10/05/2017 09:29 AM, Maurizio Cimadamore wrote:
>>>>>> Hi,
>>>>>> this is a fix to our raw diagnostic machinery that will allow for 
>>>>>> more robust compiler negative tests. Currently, when a functional 
>>>>>> expression is found in a diagnostic, it can sometimes be 
>>>>>> represented using the absoluted cursor position of the expression 
>>>>>> in the source file. This makes the golden files for such tests 
>>>>>> extremely unreliable, as simply adding/removing chars from lines 
>>>>>> before the functional expression would result in golden file 
>>>>>> mismatches. This is extremely annoying when e.g. the jtreg header 
>>>>>> of a test must be change for some reason (e.g. to add an extra 
>>>>>> bug id).
>>>>>>
>>>>>> The solution is to use a more robust encoding with line:col - so 
>>>>>> that there's less dependencies on what happens on previous lines.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/8172443/
>>>>>>
>>>>>> Cheers
>>>>>> Maurizio
>>>>>>
>>>>>
>>>>
>>>
>>
>