Support for ICU API Status

[NightOwl888]

I have been mulling over how to support the ICU API statuses for quite some time and think it is time to have a discussion about it. This is a very important thing to get done right before the production release. The documentation for API and Docs as well as the API Compatibility docs are pretty detailed and it looks like they have some automation built around this to investigate.

Internal Status

One thing that is really annoying is that if we port @internal in Java to [Obsolete] in .NET, we end up with a bunch of suppression code to get rid of the warnings everywhere. So, this is definitely not the answer. Marking them internal also doesn't really seem to be the best answer as some APIs cannot be marked internal (for example, when a subclass needs to be public).

• Should these be internal?
• Should these be public, but hidden from the IDE?

This really depends on how much extensibility a user needs. I am leaning toward making them internal and relying on the new UnsafeAccessorAttribute for any users that dare to call the functionality.

If it is made public (even if hidden), we would ideally have a custom Roslyn code analyzer warning if a user accesses such an API. This would allow us to globally suppress the warning internally without also suppressing valid warnings for obsolete APIs that we may be calling.

Draft Status

These APIs are released in "preview" called draft status. This means that the API is unstable even though it is in a stable library release. My thought here is to make them both visible and public, but provide a custom Roslyn code analyzer warning.

Deprecated and Obsolete Status

These both sound like valid things that [ObsoleteAttribute] should cover, although I don't know offhand why there are 2 different statuses.

Impl Namespace

One thing I found interesting is that when I looked through the javadocs for ICU4J, they don't list any of the classes in the impl package. This kind of makes me think that the right choice for these is to hide them from the IDE at least and perhaps mark them internal. Marking them internal would significantly cut down on the number of public APIs that we need to prepare for release, though.

Conditional Compilation

Microsoft contributed some compilation constants upstream from their copy of ICU4C. They set it up so all of these unstable APIs would be compiled out of the release to prevent their team from accidentally calling them.

This solves the problem quickly, but to me it seems like the draft APIs should at least be made available so users can start exploring the experimental functionality.

Technically, much of what we are doing is experimental, so if we went that route on our first release, the API would be so barren it would probably be hard to use.

Custom/Built-in Attributes

IMO, this feels like the right choice in .NET rather than burying API statuses in the API docs. Attributes can be made to drive both the Roslyn code analyzer warnings and control how the API docs are generated. But it would probably be worth digging into the automation that ICU built to see if there is more to this that we should be aware of.

[rclabo]

Thanks Shad for pulling this all together and laying out the lay of the land.

My first thought is that I would want to be able to call the experimental APIs unless there was a chance it's core underlying functionality might be going away. If the API changes form in the future, that's fine, as long as there is some way for me to migrate my code to the changed API.

I personally like the attribute approach to marking the experimental APIs. I'd also like the XML summary of the method to indicate it's "Experimental:" or something like that. If I had that, then I don't personally care if there is a Roslyn code analyzer to warn me. In fact, I might even prefer there wasn't.

It's probably worth noting that this approach is a bit like how LuceneNET starts the XML summary of a method with Expert: for many of it's advanced methods. I found this approach very helpful when I was first learning LuceneNET. For an example see the NumericTokenStream class.

[rclabo]

By using a new ExperimentalAttribute instead of [ObsoleteAttribute] there shouldn't be warnings. I think the warning happen with a developer calls method that has the [ObsoleteAttribute] on it.

[NightOwl888]

The ExperimentalAttribute does seem to be a good fit for the draft scenario. And if you read further, it specifies that the warning triggers a build failure unless it is suppressed. Personally, I am fine with that. The user has been warned and they suppressed the warning so are willing to accept the consequences if the API is redesigned later.

On one hand, I really don't like reinventing the wheel when something already exists that is useful. On the other hand, if we use this attribute, we would either:

1. Suppress the warning on every API we have in the project that calls it
2. Suppress the warning in MSBuild at the file level
3. Suppress the warning globally (at the project or solution level)

The 3rd option seems like the right choice to keep from polluting the codebase until you consider that this upstream functionality may be used in 3rd party libraries. So, if we suppress the warning globally, we may end up inadvertently calling an experimental API that we don't own.

On the other hand, if we had a custom attribute that we own, we would still get the warnings from 3rd party APIs if they are suppressed globally.

I checked with ChatGPT and there does seem to be an option for suppressing warnings in MSBuild at the file level, which may work with globbing expressions. If it does, that seems like it could be a solution to using BCL attributes for this. - On second thought, this really doesn't help because the calls to the 3rd party APIs will be in those same files. This is effectively no different than suppressing at the project level except to control the specific files to suppress.

[NightOwl888]

Note also in the docs for the ExperimentalAttribute, Microsoft mentions there a [Deprecated] attribute, although I don't see it mentioned in the API docs as of .NET 9.

The API docs for ExpermentalAttribute indicate that it was added in .NET 8. But if we have to add an internal attribute for backward compatibility support, that seems like a good approach. Then we could either make a code analyzer to make it function on older TFMs or leave it only as a declaration. The latter probably works, as I don't think there will be any reason to provide different APIs signatures on older TFMs.

[rclabo]

I'd likely opt for choice 3. Suppress the warning globally (at the project or solution level). To me the main thing that needs done is to make it clear the method is experimental. Marking it with an attribute and ALSO marking it with "Experimental:" in its description certainly accomplishes that.

[paulirwin]

Agreed with @rclabo

[NightOwl888]

I think based on the feedback that the explanation of API Status was not broad or deep enough. Part of that was due to my lack of knowledge of the subject, so I dug a bit deeper.

ICU4J Doesn't Provide Conditional Compilation Like ICU4J

I suspect the reason for this is because the main distribution channel is through Maven. While it is possible for front-end developers to work with custom builds of libraries in some cases, it presents problems when the consumer also has a library distributed through a package manager such as Maven. So, the APIs are either released publicly or not. In the case of @internal, they are always public so nobody has to use a custom build of ICU4J to use these APIs.

It seems we are in the same boat when deploying through NuGet.

API Status is a Subset of Javadoc Tags

ICU4J uses javadoc to create various reports about tags including API Status. None of these tools seem to do any updates. Apparently they are done manually.

The tools are in this directory: https://github.com/unicode-org/icu/tree/36061ccee26bd54b2e72e11e0c662034e8f4511c/icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/docs

There is a README here: https://github.com/unicode-org/icu/blob/36061ccee26bd54b2e72e11e0c662034e8f4511c/icu4j/tools/build/README.txt

The valid tags are:

• @internal (All public APIs that are not @internal are considered to be external)
• @draft
• @stable
• @since
• @deprecated
• @author
• @see - Covered by <seealso/> in XML doc
• @version
• @param - Covered by <param/> in XML doc
• @return - Covered by <returns/> in XML doc
• @throws - Doesn't apply in .NET
• @obsolete
• @exception - Covered by <exception/> in XML doc
• @serial
• @provisional- Usually (but not always) used in conjunction with @draft
• @icu - shows up in the API docs
• @icunote - shows up in the API docs
• @icuenhanced - Used to indicate a copy of a JDK API that provides additional functionality, shows up in the API docs

The valid API Statuses are:

• @internal
• @draft (followed by a version number)
• @stable (followed by a version number)
• @deprecated (followed by a version number and possibly a new replacement API)
• @obsolete (followed by a version number and possibly a new replacement API)

There are basically 3 things that need addressing, and they have different priorities.

1. Enforcing API Status
2. Determining the visibility and/or accessibility for each status
3. Reporting the API Status to users

API Status is a policy that applies to the entire public API surface (with a few exceptions). Every public API must have a status. It is important that users are aware of the status of each API and so they are aware of the implications if they don't use a stable part of it. Once an API is marked stable, it will always exist and will always function (as far as I can tell), even if it is later deprecated.

Enforcing API Status

As far as I can tell, there are only 2 options for this:

1. Create a command line tool to generate a report and then update the classes manually
2. Create a Roslyn code analyzer and code fix to enforce the policy, which will provide warnings both on the command line and in the IDE

Frankly, given how important this is, I think the code analyzer is the only way to go here. It will help to ensure the policy is always adhered to. It will provide quick links to the exact spots in the code that need to be cleaned up and one or more code fixes can insert the snippet to fix the issue.

Generating the text report in the build may also be useful, especially if the release procedure is setup to commit these reports to be compared across versions. However, it is much more work to use reports to enforce policies than analyzers and code fixes. The analyzer should take priority, IMO.

Determining the Visibility and/or Accessibility for Each Status

These are my opinion:

@stable

Public. Visible. Enough said.

@draft

Public. Visible. But with a build failure that must be suppressed to use with a custom message.

Many of the new and redesigned APIs that were added to ICU4N have been marked @draft. But after further consideration, these should probably be marked @stable. Our "draft" period for these should technically end when we make the first stable release of ICU4N.

But should we have a way to differentiate between our new APIs and original ICU APIs that are @stable?

I don't think we should use ExperimentalAttribute for the same reason we don't want @internal APIs to use ObsoleteAttribute. It would mean either polluting our codebase with suppression code or suppressing warnings globally, which would make us vulnerable to calling BCL or 3rd party APIs that are experimental. We need these warnings to happen on code in assemblies we don't own, but we should not have to deal with them regularly in code that we own.

@internal

Public. Not Visible. But with a build failure that must be suppressed to use with a custom message.

However, I think there is less risk and less work for the first release if we make these internal and roll them out later.

We should definitely not be marking these with ObsoleteAttribute because they are directly meant for use internally by ICU code. Instead, we should have a Roslyn code analyzer with a custom warning message instead.

@deprecated

This status produced a confusing warning message in Java that “This is a draft API and might change in a future release of ICU.” so was discontinued from use in ICU4J 3.4.3. So, we can treat these the same as @obsolete. But @obsolete is a custom ICU tag and @deprecated is built-in in Java.

@obsolete

So far, these have been mostly marked with internal accessibility. To me it doesn't make much sense to release deprecated functionality in a new piece of software (we do in Lucene.NET, but that is because it is mostly a line-by-line port).

I am on the fence here. On one hand, we probably could provide these APIs for compatibility with upstream ICU. But on the other hand, it is more work to prepare a larger set of APIs for release. We can always mark them public later if upon the initial release they are internal, but we cannot do the reverse, as it is a breaking API change.

Using an [Obsolete] attribute would not provide a confusing error message in .NET like there is in Java. It correctly indicates the API is deprecated and allows custom text to be put in place.

Reporting the API Status to Users

There are 4 things to consider here:

1. Build Warnings and Errors
2. Intellisense
3. Reporting Tools
4. API Documentation

Build Warnings and Errors

These are of course generated primarily by Roslyn code analyzers in practice.

In my opinion, any API that is marked public, whether visible or not, that has an API status other than @stable should have a build warning. There should be no need to rely on documentation to inform a user that the API they are calling is anything other than @stable, the compiler must complain about this. But the user should also be able to suppress the warning if they accept the risk.

Whether to fail the build or not is up for debate. My thought is to not fail the build for @obsolete and fail the build for @draft or @internal. We don't want projects that already call APIs that are now @obsolete to continue building. But callers to @internal and @draft APIs should have to think twice.

Intellisense

It is also important to show the status in Intellisense, I think. But I discovered that the only way to accomplish this is to put the API Status Tag in one of the sections that Intellisense displays. There is no way to apply an attribute and have it show up in Intellisense.

Given this fact, I am now thinking that we should be using the same tagging style (starting with @) as they do in ICU. This policy can be enforced pretty easily with a Roslyn code analyzer and code fix. I can't imagine analyzing thousands of APIs manually to make sure they have an API status in the code comment, especially when you consider this will need to be done continually.

Unfortunately, during the port the tags have been converted to XML tags instead of placing them inside of the <summary> or <remarks> section where it would appear in Intellisense.

ICU4N/src/ICU4N.Collation/Text/RuleBasedCollator.cs

Lines 1903 to 1904 in fee1b3a

	/// <draft>ICU 2.8 (retain)</draft>
	/// <provisional>This API might change or be removed in a future release.</provisional>

I really like the idea of putting individual pieces of data into XML elements, but given that this is useless to get them into Intellisense, perhaps another Roslyn code analyzer and code fix should be made to convert them from XML elements back into Tags that are placed at the bottom of <summary> or <remarks> section.

API Reporting Tools

After briefly reviewing these tools, it is clear that some of them are important for maintenance of the project. So, not every problem is going to be solved with Roslyn code analyzers. One thing that seems especially important is the generation of the API Change Report, although I am not certain how that is generated.

API Documentation

Putting API status here is equally important. Since we already have a working example of scanning for tags in Lucene.NET and converting them into messages with special formatting, it is clear that we can use the tags for this.

Tags vs .NET Attributes

My original thought was to make custom .NET Attributes to drive all of the above.

Given that Intellisense doesn't support attributes or even custom XML elements, we are down to 2 options on how to specify API Status:

1. Tags
2. Both Tags and Attributes

Tags make sense to use for the documentation and Intellisense, and due to the flexibility of Roslyn code analyzers we can easily make warnings based on them. I am not sure how easy or hard it will be to make a code fix for an XML doc comment, though.

A couple of things that are probably worth looking into:

1. Can DocFx drive the same type of reporting tools that javadoc can?
2. Can we use Roslyn to build these reporting tools?

I know we could use Reflection to scan for attributes which could be used to drive tools like these, but perhaps that is not the best choice for this. Specifying attributes in addition to tags is more work. We could have warnings and code fixes that ensure they remain in sync, though.

We can use ObsoleteAttribute instead of an @obsolete tag (and it seems wrong not to). That would mean our analyzer must enforce either the ObsoleteAttribute OR a @internal, @draft, or @stable tag. Intellisense shows when an API is marked with ObsoleteAttribute, so leaving the tag out of the XML comment in this case is not a problem.

[NightOwl888]

Impl Namespace

I forgot to mention the Impl namespace this time around.

As this is at least 1/3 of the public code, we could push the production release out faster if we considered this to be similar to @internal for the time being. We will consider them unstable until some later point after the release. They would be Public and Not Visible, and have a build warning that fails the build unless it is suppressed.

Frankly, I don't even like having an Impl namespace, or any class with Impl in the name, but at least those decisions can be delayed until after the release if we treat this like @internal. It does make sense to keep all of this together in one namespace to simplify suppressing visibility in both the docs and the IDE. Perhaps Advanced or even Expert would be a better namespace name. In ICU4C, the "advanced" stuff was not split out separately, so I think we have a bit of a creative license with this.

[rclabo]

Gotcha. Seems like a reasonable approach

[rclabo]

So, the APIs are either released publicly or not. In the case of @internal, they are always public so nobody has to use a custom build of ICU4J to use these APIs.

Later you say @internal is Public. Not Visible. Can you elaborate about what Not Visible means? And the idea that something that is internal is Public seems very odd to me coming from the .NET world. I think I don't understand.

Our "draft" period for these should technically end when we make the first stable release of ICU4N.

I agree with this statement.

In my opinion, any API that is marked public, whether visible or not, that has an API status other than @stable should have a build warning.

That's fine, and certainly a conservative approach, but the first thing I'm going to do if I'm using that library is turn those warnings off. When I code against a method in the library, I'll look at intelligence for it and I expect it to tell me its status. If that's crystal clear in intellisense then I'm good to go, and I wouldn't be surprised if many other devs feel the same way. All of these alerts in tooling are like training wheels, and sometimes they are great, but often they just get in the way of being productive (my perspective). The one exception is depreciated given that it's typically added AFTER it was ok to use the method. So getting alerted to the change is helpful in my opinion.

It is also important to show the status in Intellisense, I think.

Absolutely. To me, this is far more important than build warnings.

API Documentation. Putting API status here is equally important.

Totally agree.

In my opinion, any API that is marked public, whether visible or not, that has an API status other than @stable should have a build warning. There should be no need to rely on documentation to inform a user that the API they are calling is anything other than @stable, the compiler must complain about this. But the user should also be able to suppress the warning if they accept the risk.

I'm ok with this approach, but I see it as an extra bell and whistle. It's nice. But it's the kind of thing I personally wouldn't delay a production release to implement. That said, I have no issue if you want to go this route.

We can use ObsoleteAttribute instead of an @obsolete tag (and it seems wrong not to). That would mean our analyzer must enforce either the ObsoleteAttribute OR a @internal, @draft, or @stable tag. Intellisense shows when an API is marked with ObsoleteAttribute, so leaving the tag out of the XML comment in this case is not a problem.

I guess I don't really understand the references to using @internal, @draft, or @stable tag. This is from the java world, right, so in .NET it would need to be internal, DraftAttribute, or StableAttribute. Correct?

[NightOwl888]

So, the APIs are either released publicly or not. In the case of @internal, they are always public so nobody has to use a custom build of ICU4J to use these APIs.

Later you say @internal is Public. Not Visible. Can you elaborate about what Not Visible means? And the idea that something that is internal is Public seems very odd to me coming from the .NET world. I think I don't understand.

By "Not Visible", I mean they would be decorated with [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] which will hide it from the IDE. It is still there and can still be called, but you would need to dig around in the source code to find it or use Reflection or some other means.

In my opinion, any API that is marked public, whether visible or not, that has an API status other than @stable should have a build warning. There should be no need to rely on documentation to inform a user that the API they are calling is anything other than @stable, the compiler must complain about this. But the user should also be able to suppress the warning if they accept the risk.

I'm ok with this approach, but I see it as an extra bell and whistle. It's nice. But it's the kind of thing I personally wouldn't delay a production release to implement. That said, I have no issue if you want to go this route.

The point is there never is a delayed production release. ICU never has a release that is unstable, just APIs that are unstable. They state in the ICU API compatibility section that:

When a new API is added to ICU, it is marked as draft with a @draft ICU x.y label in the API documentation, **where x.y is the ICU version when the API signature was introduced or last changed. A draft API is not guaranteed to be stable! Although we will not make gratuitous changes, sometimes the draft APIs turns out to be unsatisfactory in actual practice and may need to be changed or even removed. ...

When a @draft ICU x.y API is changed, it must remain @draft and its version number must be updated.

So it is not a bell or whistle, it is a guardrail to keep users from falling off the catwalk into unstable API territory if they didn't intend to (and neglected to use a net). Its not avoided by not referencing a prerelease package, it is avoided by not calling the unstable methods in the stable package.

Furthermore, this won't delay the ICU4N production release. We have to go over the entire public API to give each member a status or we cannot make a production release. Creating code analyzers and code fixes to do this task will get us there a lot faster, ensure these statuses are maintained for the lifetime of the project, and ensure the users are warned when they attempt to call an unstable API. During the port, there was never a clear direction to go with this, so many APIs are either marked wrong or missing this status entirely. All of the statuses that do exist are in custom XML comments that don't show up in Intellisense, so we need to change all of them. It takes a day or two to build a code analyzer and code fix, but it will save weeks of manual tedious error-prone work. I simply don't understand why you might object to automating this work.

NOTE: If you are referring to the upcoming J2N/ICU4N/Lucene.NET release, my apologies. I don't intend to make this a part of the next prerelease. However, API status is a blocker for the production ICU4N release, so I am looking at the angles so there is a plan to do this soon. This has been on my radar for years now, but I never sat down and looked at what the options are to come up with a solid plan. I literally found out I knew less about it after my initial post here than I thought I did at the beginning of the post.

I think that Lucene follows a similar approach, but I have not seen a document with their policy. They definitely don't do prereleases. There are publicly accessible APIs that are marked @internal in Lucene. I am not saying that means we have to use this same approach in Lucene.NET, but it may be worth a look to see how often one of these @internal or @draft APIs changes in a minor release. I remember seeing a complaint or two that they weren't strictly following semantic versioning and users weren't expecting an API to break without a major release, though. Warnings would help to avoid this sort of thing.

We can use ObsoleteAttribute instead of an @obsolete tag (and it seems wrong not to). That would mean our analyzer must enforce either the ObsoleteAttribute OR a @internal, @draft, or @stable tag. Intellisense shows when an API is marked with ObsoleteAttribute, so leaving the tag out of the XML comment in this case is not a problem.

I guess I don't really understand the references to using @internal, @draft, or @stable tag. This is from the java world, right, so in .NET it would need to be internal, DraftAttribute, or StableAttribute. Correct?

My initial thought was to use attributes for these. But since there is no way to support Intellisense with attributes, I have come around to the realization that it would be better to just put the same @internal, @draft, and @stable tags into the <summary/> section (I am thinking at the bottom after all of the documentation that is in the tag). This may allow us to avoid having to specify both tags and attributes to enforce the policy. Although, we could do both if we build Roslyn code analyzers and code fixes to warn in both directions either if they don't match or one of them is missing. It also means we stay aligned with the syntax they use in ICU.

Since an analyzer doesn't care what type of code element it is looking at, it can be an XML doc comment as easily as an attribute. It may be harder to make a code fix in an XML doc comment, though (which is just an XML fragment). It will take some experimentation to check. I guess it would also be worth a look at what the [ObsoleteAttribute] is doing to show up in the tooltip, though.

My preference would still be to use custom .NET attributes if there is a way to show them in Intellisense without also having to specify a duplicate status in the XML doc comment. ChatGPT says no, there isn't. But since the tooltip for users is driven by the XML file that ships with the DLL, we might be able to work around this by modifying the XML after the build. It wouldn't appear while maintaining our project, but it would appear to users. Not sure whether that works for DocFx, though.

[rclabo]

Always read into my comments and perspectives a large amount of positive vibes and good will. I have nothing but the utmost respect for how you design and build systems. I tend to be a bit more on the pragmatic side. And that may be useful at times for tempering your strong engineering approach. Most of the time you end up convincing me because you know aspects I haven't yet learned about the system. I think the back-and-forth is useful, though. It definitely helps me anyway. I learn a ton. Hopefully it's not too annoying for you. :-)

By "Not Visible", I mean they would be decorated with [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] which will hide it from the IDE. It is still there and can still be called, but you would need to dig around in the source code to find it or use Reflection or some other means.

Wow, that's cool. But if that's the case then I see even less need for build warnings. The dev had to go out of their way to call the method. Any dev doing that doesn't need or want a build warning. (my opinion)

The point is there never is a delayed production release. ICU never has a release that is unstable, just APIs that are unstable.

That made me smile. I guess ICU can do what ICU does :-) Still

So it is not a bell or whistle, it is a guardrail to keep users from falling off the catwalk into unstable API territory if they didn't intend to (and neglected to use a net).

I think an unneeded gaurdrail. If the method is hidden from tooling and the method is marked with an attribute and summary that says it's not ready for prime time and they still call it. To my eye, they already decided to climb over the gaurdrail. But again, I don't have a problem with adding compiler warnings as a 2nd level of gaurdrail if you feel strongly about it but I feel like it's not really necessary.

Creating code analyzers and code fixes to do this task will get us there a lot faster, ensure these statuses are maintained for the lifetime of the project.

If that's the case then that might be the stronger justification in my mind.

It takes a day or two to build a code analyzer and code fix, but it will save weeks of manual tedious error-prone work. I simply don't understand why you might object to automating this work.

I've never built a code analyzer before, so I didn't have any idea how much work it is. The second sentence made me smile (friendly). I have absolutely no objection to automating work. I probably spend more time doing that for my own projects then I should. As devs, I think we have a visceral dislike of manual work that can be automated. It goes with the territory, I think.

My initial thought was to use attributes for these. But since there is no way to support Intellisense with attributes, I have come around to the realization that it would be better to just put the same @internal, @draft, and @stable tags into the

section (I am thinking at the bottom after all of the documentation that is in the tag). This may allow us to avoid having to specify both tags and attributes to enforce the policy. Although, we could do both if we build Roslyn code analyzers and code fixes to warn in both directions either if they don't match or one of them is missing. It also means we stay aligned with the syntax they use in ICU.

Since an analyzer doesn't care what type of code element it is looking at, it can be an XML doc comment as easily as an attribute. It may be harder to make a code fix in an XML doc comment, though (which is just an XML fragment). It will take some experimentation to check. I guess it would also be worth a look at what the [ObsoleteAttribute] is doing to show up in the tooltip, though.

super interesting. I always learn alot from you Shad.

My preference would still be to use custom .NET attributes if there is a way to show them in Intellisense without also having to specify a duplicate status in the XML doc comment. ChatGPT says no, there isn't. But since the tooltip for users is driven by the XML file that ships with the DLL, we might be able to work around this by modifying the XML after the build. It wouldn't appear while maintaining our project, but it would appear to users. Not sure whether that works for DocFx, though.

I personally don't have any issue with having a duplicate status in the XML summary but either way. (my pragmatizing coming through again)