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

[Jonathan Gibbons]

On 10/5/17 7:40 AM, Maurizio Cimadamore wrote:
>
>
> 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

I agree with Vicente's concern, but also agree with Maurizio's response. 
As to Vicente's comment "but you know how long it takes for stuff like 
this to be revisited", I'm sure it will be revisited if the pain is high 
enough, as demonstrated by this round of fixup.

>>
>>>
>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>