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

[Maurizio Cimadamore]

On Thu, 10 Apr 2025 20:45:21 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> In the sketch I was doing a few days back, I was peeking at forcing some kind of `EndPosTable` argument to `TreeInfo.endPos`, but I shied away as it seemed to complicate things. Here I see it is not as bad as I thought, but still it spreads a bit.
>> 
>> I see the value in avoiding special field and code path for the AST nodes for which the end pos is mandatory, in addition to the optional EndPosTable.
>> 
>> I think I would still slightly prefer the specialized fields, esp. if the impact on mostly unrelated outside code (like TransPatterns and Gen) could be eliminated. I would like to investigate if we can drop the EndPosTable altogether, although that is unlikely to happen before JDK 25 RPD1 (maybe 2). But, there is, of course, a possibility that the removal of EndPosTable won't happen, at which point we would have the duplication.
>> 
>> But, I can live with a patch long the lines here adding EndPosTable to `TreeInfo.endPos` if needed. Should we drop the EndPosTable as some point, we can drop the new EndPosTable argument. If not, we would preserve it. It is not what I would do, but I don't think either of the solutions is clearly wrong.
>
> @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, optionally, we might want to rename `TreeInfo::endPos` to something like `TreeInfo::blockLastPos` or something like that.

In the future, we might revisit, at some point, whether storing end positions in a separate side-table is still the most effective way to do things -- but that discussion can happen separately from this PR.

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

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