RFR: 8350212: Track source end positions of declarations that support @SuppressWarnings [v4]

[Archie Cobbs]

On Thu, 29 May 2025 14:28:42 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> @lahodaj, thanks for your thoughts.
>> 
>> I agree it's not ideal either way. The thing that kindof pushes it in the `EndPosTable` direction for me is this: As long as we have `DiagnosticListener`'s and `-Xjcov`, we have a (sometimes) requirement to track the ending positions of _every_ node. So as long as we are required to have code that does that, why not just use it? Then we have only one mechanism for doing something, instead of two.
>> 
>> Practically speaking: @mcimadamore are you OK to proceed with the current approach?
>> 
>> In any case, it's not a tragedy if we change our minds in the future - removing `EndPosTable` or switching back to the previous approach is easy to do (and I'm happy to volunteer if needed).
>> 
>> Thanks.
>
> Hi @archiecobbs. I took another look at how javac is structured in this area (with the help of @lahodaj). `TreeInfo` has two slightly different methods (maybe this is all obvious to you, but repeating here just in case):
> 
> * `TreeInfo::endPos` -- this is an internal method, only used by javac which, in reality, doesn't have a lot to do with end positions. It is mostly used as a way to retrieve the position at the end of a block (e.g. where a new statement should be appended), and is used mostly for code generation purposes (e.g. tells javac where synthetic code should be added), or where errors should be emitted -- this is especially the case in `Flow`, which uses `TreeInfo::diagEndPos` which uses this. This method does NOT depend on an `EndPosTable` -- it does all it needs by using a couple of dedicated fields in `JCBlock` and `JCSwitch`/`JCSwitchExpression`.
> * `TreeInfo::getEndPos` -- this is the method that accesses the `EndPosTable`, looking for the end position of a given tree. This method is used by `JavacTrees`, so used also externally. If there's no `EndPosTable` (e.g. the table is set to null), this method delegates to `TreeInfo::endPos`, but I'm not sure this ever happens, given that when we don't want end positions we tend to use the `EmptyEndPosTable` -- which simply disregards end pos information. In other words, the end pos table is never `null`, which means this method returns `NOPOS` when the end pos table is not in effect.
> 
> Now, it seems to me that the changes you are after have nothing to do with the first method. You want extra end pos information for modules, packages, classes, methods, fields -- e.g. places where a `@SuppressWarnings` annotation might go. As such, I think this PR should not touch `TreeInfo::endPos` at all. It should only define the minimal end pos table to make sure that enough end positions are captured _at all times_, but no more than that. More specifically, the part where we replace fields in `JCTree` with the use of the minimal end pos table seems bad, as the fields are there to support `TreeInfo::endPos` which strictly speaking, has nothing to do with warning suppression (not now, not in the future).
> 
> If you want to rename the `endPos` fields in the AST nodes to `bracePos` that's fine -- but I think `TreeInfo::endPos` should remain unchanged, and that your other code that needs end position information should instead use `TreeInfo.getEndPos`, which will be augmented thanks to your new minimal end pos table. Again,...

Hi @mcimadamore,

Thanks for taking a look and for the clarifying comments.

> If there's no EndPosTable (e.g. the table is set to null), this method delegates to TreeInfo::endPos, but I'm not sure this ever happens...

>From what I could tell, the `EndPosTable` is never null in the existing code. Assuming that's the case, there's no point in optimizing any code to avoid the use of `EndPosTable` just because it might not be there. So my patch includes some cleanups along these lines, for example, adding a new `EndPosTable` to the `TreeInfo.endPos()` method, thereby making it "safe" to use with any node type.

While I was at first thinking that would be a useful simplification, as you point out, given that `TreeInfo.endPos()` actually only needs to work for a handful of node types, that refactoring is unnecessary and is obscuring the goal of this PR so I'm happy to remove it.

> If you want to rename the endPos fields in the AST nodes to bracePos that's fine -- but I think `TreeInfo::endPos` should remain unchanged, and that your other code that needs end position information should instead use `TreeInfo.getEndPos`, 

Sounds good, that will make things more straightforward. I'll work on it.

Thanks.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23669#issuecomment-2920671566