[core] Introduce a new @DeprecatedUntil annotation

[jsotuyod]

Describe the PR

• This should be a better and more stable solution to the old @DeprecatedUntil700 we used during PMD 7's development.
  • It is not tied to a specific major.
  • It allows us to give more than 1 major to remove something if we ever want to…
  • It allows us to explicitly document what we want to do in the next major.
  • It doesn't force us to immediately apply other annotations such as @internalapi which lead to inconsistency (ie: is is already private or will be made private in the next major?)

Related issues

• See [core] Add japicmp #4926 (comment)

Ready?

• Added unit tests for fixed bug/feature
• Passing all unit tests
• Complete build ./mvnw clean verify passes (checked automatically by github actions)
• Added (in-code) documentation (if needed)

[pmd-test]

	1 Message
📖	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact

Generated by 🚫 Danger

[adangel]

Moving the discussion from #4926 (comment) to continue:

For now, I only listed the internal packages. Changes in impl packages would be picked up by the plugin. In case we change something there, we need to decide then: Change it or not. Currently, we don't provide guarantees for impl packages (https://docs.pmd-code.org/latest/pmd_projectdocs_decisions_adr_3.html#criteria-for-public-api) as we consider both internal and impl as private. But with this tool in place, we can maybe keep even impl stable?

Even if impl is private API, it's intended for language developers, so I'm ok with attempting to keep it stable. Also, it would be consistent to how we treat javadoc, skipping all internal packages, but providing docs for impl packages.

This setup would require us to do some things differently moving forward however… During PMD 7 development, whenever we wanted to internalize a public API we would annotate it both with @InternalApi and @DeprecatedUntil700. We can't do that anymore moving forward, as the @InternalApi would instantly stop checking for changes on that API.

You can apply @InternalApi also on the method level I think. But true, as soon as you (accidentally?) apply this annotation for a whole class - even if the class was public before - japicmp would be completely disabled. So, using @InternalApi should be done with great care. Not sure what we can do about this other than reviewing code before merging.

One thing we could do however is: Remove the annotations from the japicmp configuration and force us to explicitly list the members/classes that are internal in the excludes of the japicmp (you can also exclude individual members). We would then use XML comments in pom.xml to explain, what is internal api and why this or that is excluded etc.

A nice alternative would be to supersede @DeprecatedUntil700 with a generic @DeprecatedUntil(major = "8", future = REMOVAL / INTERNALIZATION) (both attributes required) and use that sole annotation moving forward, leaving the eventual inclusion of @InternalApi solely for the next major branch. Thoughts?

Instead of creating another own annotation, we could also use https://github.com/apiguardian-team/apiguardian : the single annotation API might be all we need. I'm not sure about tooling support. I guess, with japicmp we would then need list the excluded members explicitly, as you can't just exclude @API(status = INTERNAL) (I guess).

If we introduced apiguardian now, we would still need to use the old annotations @InternalApi and @Experimental in addition to @API(status = INTERNAL) or @API(status = EXPERIMENTAL), since the existing annotations are our API for PMD 7. Btw. for deprecated members, we probably also always want to add both @Deprecated and @DeprecatedUntil / @API(status = DEPRECATED). We could create a dogfood PMD rule to enforce both annotations.

Whatever we do, we need to update https://github.com/pmd/pmd/blob/master/docs/pages/pmd/projectdocs/decisions/adr-3.md .

Overall, I'm in favor of better API documentation. I'm not sure, whether we need to distinguish for deprecated members whether they will be removed or internalized - in any case, you shouldn't use it anymore. My idealistic understanding of deprecation is, that it will be gone with the next major version - so IMHO we don't need repeat major = "8" here.

[oowekyala]

I'm not sure, whether we need to distinguish for deprecated members whether they will be removed or internalized - in any case, you shouldn't use it anymore.

The distinction is useful for us maintainers though, to know what to do when cleaning up before a major release. Maybe we should use a new annotation @WillBeInternalized or so, and only use that one when deprecating something (for internalization) between major versions. Since it wouldn't be excluded from japicmp we would keep taking care of compatibility. When comes the time for a major version

• Api marked with @Deprecated @WillBeInternalized is tagged with @InternalApi and therefore hidden from then on from japicmp. Maybe it is internalized as in moved to an internal package or made package private or whatever.
• Api marked with just @Deprecated is removed.

The point is that between majors, we only use @InternalApi on new API (or not use it at all), so that we don't hide stuff from japicmp if we still have to commit to compatibility to it.

I also agree with Andreas here that we don't need to mention the major in our documentation annotations. I would like to keep the simple model that API is always deprecated until the next major, whichever that is.

[adangel]

The distinction is useful for us maintainers though, to know what to do when cleaning up before a major release.

Why can't it be that simple that we simply delete all deprecations for a major release?
This is of course only possible, if we refactored before the major release all our own usages of the deprecated API. IMHO, when we mark something as deprecated, it shouldn't be used by us anymore (we have in our codebase zero usages). The API is only kept for compatibility and just calls the internalized/renamed API.

I would strongly advice against introducing something like @WillBeInternalized - this sounds like a postponed task that will never be done. If we deprecate something, we IMHO must have the internal alternative already available and in use. IMHO the major version is just deleting every deprecated method (cleanup) and a version bump.
If we do the refactoring immediately we don't need to remember, what to do. So, IMHO, this annotation is not needed.
In that sense, the code base would be almost always ready for a major version release (aside from deleting the deprecated methods).

[oowekyala]

Why can't it be that simple that we simply delete all deprecations for a major release?
This is of course only possible, if we refactored before the major release all our own usages of the deprecated API. IMHO, when we mark something as deprecated, it shouldn't be used by us anymore (we have in our codebase zero usages). The API is only kept for compatibility and just calls the internalized/renamed API.

Maybe it's that simple, but I don't know. I'm not sure whether we will run into the same problems during the PMD 7 release cycle as during the PMD 6 one. For instance there were many classes which we wanted to hide in PMD 7 but not delete from the codebase. In that case I think in many cases it doesn't make sense to copy the class into an internal package and deprecate the old one. It might be plain impossible because it would introduce new types that are not necessarily compatible. Like I said I don't know if we will run into this scenario often, but we might chance into it at least eg to internalize XPathRule.

If we deprecate something, we IMHO must have the internal alternative already available and in use. ... If we do the refactoring immediately we don't need to remember, what to do. So, IMHO, this annotation is not needed.

This is ideal but it is not always possible... I think there is still a distinction to be made between 'we want to hide this' and 'we want to delete this', but maybe just a comment is enough, coupled with tracking in an issue. And of course first try to do what you say, that is, refactor as soon as possible.

[jsotuyod]

Exactly, there will be scenarios where internalizing (hiding) is not compatible with adding a new method and deprecating the old one. For instance, wanting to reduce the visibility of a field / method from public to something else. We can't overload just with a different access modifier, which would force us to come up with a different name (probably less succinct / meaningful) than the original one.

If it were a public non-final field (ugly, I know) we can't duplicate it, as we can't ensure to keep both attributes in sync.

Yes, the vast majority of cleanups are effectively going to be deletes (and it has been so in PMD 6 and PMD 7), but I don't think it can ever be reduced to solely that without hindering or limiting the changes we can do.

As for using API Guardian, I see 2 issues with it:

1. japicmp works on marker annotations, not considering values AFAIK
2. it doesn't differentiate the current guarantees (ie: it's maintained and we will not break this API during this major) from the future (ie: it will be internalized), which brings us back to your other suggestion…

One thing we could do however is: Remove the annotations from the japicmp configuration and force us to explicitly list the members/classes that are internal in the excludes of the japicmp (you can also exclude individual members). We would then use XML comments in pom.xml to explain, what is internal api and why this or that is excluded etc.

I'd rather not have to manually add each class and member to the pom… we have seen in PMD 7 development that list can be massive, and there is a strong chance we miss some.

I think we will need this annotation, along with the current uses of @Experimental, @InternalApi and @Deprecated to meaningfully and consistently handle this across javadoc, japicmp and the source itself.

I'm open to drop the major attribute if you think we will never ever need it… to me it's still meaningful in making explicit to users when that member is going away.

[adangel]

Maybe it's that simple, but I don't know.

I also don't know. But let's be optimistic. Let's not try to solve a problem, that we don't have yet. Let's not already give up trying to keep it simple without even having it tried at least once.

Exactly, there will be scenarios where internalizing (hiding) is not compatible with adding a new method and deprecating the old one. For instance, wanting to reduce the visibility of a field / method from public to something else. We can't overload just with a different access modifier, which would force us to come up with a different name (probably less succinct / meaningful) than the original one.

If it were a public non-final field (ugly, I know) we can't duplicate it, as we can't ensure to keep both attributes in sync.

Yes, the vast majority of cleanups are effectively going to be deletes (and it has been so in PMD 6 and PMD 7), but I don't think it can ever be reduced to solely that without hindering or limiting the changes we can do.

Honestly, this wasn't the case. We kept using the deprecated stuff all over the place. In this regards we must become better - only deprecate stuff once we refactored. Don't deprecate with a note: "PMD 8 will provide an alternative implementation".

One thing we could do however is: Remove the annotations from the japicmp configuration and force us to explicitly list the members/classes that are internal in the excludes of the japicmp (you can also exclude individual members). We would then use XML comments in pom.xml to explain, what is internal api and why this or that is excluded etc.

I'd rather not have to manually add each class and member to the pom… we have seen in PMD 7 development that list can be massive, and there is a strong chance we miss some.

I don't think, we will have the problem to miss something: We don't configure the includes - that means - japicmp will consider everything as public API. We only configure excludes. And if we miss something to exclude (e.g. some method we created as internal API and we change that method) then japicmp will complain. So, I don't see the problem that we might miss entries there.

Does the list become too big? I don't know - I can't predict the future. If we consequently keep internal API in internal packages, the list won't grow so much. If the list becomes too big - maybe that's even an indication of another problem (not separating API and internal impl enough?) or it's time for some cleanup (what ever this means - as I said, I'm making stuff up now)

I think we will need this annotation, along with the current uses of @Experimental, @InternalApi and @Deprecated to meaningfully and consistently handle this across javadoc, japicmp and the source itself.

I'm open to drop the major attribute if you think we will never ever need it… to me it's still meaningful in making explicit to users when that member is going away.

For reference, the only occurrence of a deprecation in PMD 7 is right now this:

pmd/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java

Lines 103 to 104 in 7da809b

	@Deprecated //(since = "7.1.0", forRemoval = true)
	public static boolean mayBeSingular(ModifierOwner varId) {

I fully agree that we need something (annotation, tool, documentation) to better steer the API evolution/maintenance. However, right now, we are talking completely in theory, don't we? I would suggest to wait with a decision until we have a very concrete problem to solve. Then we can take this problem and maybe try so solve it in different ways and compare the results. I find it hard to solve right now a problem, that I need to make up at the moment.