Bug fix: Only tag types implementing empty Preview interfacees

[pgovind]

Just a small change. Currently the DetectPreviewFeatures analyzer tags all types implementing empty interfaces with a diagnostic. This is a bug. We should only tag types implementing empty Preview interfaces.

[codecov]

Codecov Report

Merging #5273 (e070e4d) into release/6.0.1xx (7e82240) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5273      +/-   ##
===================================================
- Coverage            95.61%   95.61%   -0.01%     
===================================================
  Files                 1233     1233              
  Lines               283380   283403      +23     
  Branches             16971    16972       +1     
===================================================
+ Hits                270965   270981      +16     
  Misses               10139    10139              
- Partials              2276     2283       +7     

[stephentoub]

Why does it matter if there are 0 members? I see the comment below, but I'm not understanding what's special about 0 vs non-0 members... ?

[pgovind]

So we (myself, Tanner, Immo and Jeff) had a discussion a while back went through this thought process:

Assume we had the following code:

    class Program : IProgram
    {
        static void Main(string[] args)
        {
            new Program();
        }
    }

    [RequiresPreviewFeatures]
    public interface IProgram
    {
    }

where IProgram is an interface defined in version 1 of some dependency. Currently, the DetectPreviewFeature analyzer won't report any diagnostics in this code. We made an explicit choice not to report diagnostics on Program's class definition. Instead we report diagnostics whenever a preview method/property/field/event etc is used/invoked. Since IProgram has no members, no diagnostics are reported here.

1. So, in effect, Program's author has opted into preview features here without knowing about it (even though no preview features are used).
2. The other concern was if IProgram was updated in V2 to now have preview methods, and those preview methods are now called by Program, the analyzer will flag a diagnostic. Program's author now has to opt into PreviewFeatures (or go back to a previous version), and we considered that to be a breaking change. (tagging @jeffhandley / @terrajobst in case he can explain more clearly here)

TLDR: We don't want to surprise folks. We're trying to be as thorough as we can in reporting when an assembly is using PreviewFeatures

[stephentoub]

We made an explicit choice not to report diagnostics on Program's class definition.

Why? Why not warn at any mention of something preview?

[pgovind]

Because, Byte (for example) is currently defined like below and we can't mark Bytes definition with the RequiresPreviewFeatures attribute. Otherwise all uses of Byte will now be considered Preview:

    [Serializable]
    [StructLayout(LayoutKind.Sequential)]
    [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
    public readonly struct Byte : IComparable, IConvertible, ISpanFormattable, IComparable<byte>, IEquatable<byte>
#if FEATURE_GENERIC_MATH
#pragma warning disable SA1001
        , IBinaryInteger<byte>,
          IMinMaxValue<byte>,
          IUnsignedNumber<byte>
#pragma warning restore SA1001

[stephentoub]

Byte isn't annotated as preview. I don't understand the example. There would be a warning where Byte implements the interface, not on uses of Byte.

[jeffhandley]

I agree that option (A) there cannot be considered--we cannot apply the attribute to existing types.

Option (B), pragma suppressing the analyzer is the explicit gesture of "I know I'm referencing a preview type here" though, and we need explicit gestures. We have plenty of dials for the suppressions too. They can be suppressed around individual references, blocks of code, entire files, projects, or solutions. And, unless I'm mistaken, through config can be dialed down from error to warning or even info. With all of these options, we have to trust our users to choose the level of suppression they are most comfortable with.

I think it is the exact opposite of how this will get used in practice and will result in a longer tail of bugs due to users having to suppress the analyzer for basic usages in practice.

I believe you're referring to bugs of over-suppressing, and later finding that we've allowed preview API usage to leak through in an implementation detail, or worse, an API definition; is that correct?

Having logic that attempts to minimize the suppressing users do requires us to essentially auto-suppress scenarios on users' behalf. As is evidenced by the quickly-identified false-negatives that @stephentoub called out, this logic incurs risk of users allowing preview API usage to leak through in implementation details as well as in API definitions--and because of logic bugs in the analyzer, vs. over-suppressing by users. I believe that to be a far worse situation for multiple reasons--one of which being that plugging a false-negative hole would be a build-time breaking change since we'd be introducing new build errors.

Altogether, I think we have room for error in suppressing in the codebase, but we have zero room for error suppressing within the analyzer logic.

[tannergooding]

pragma suppressing the analyzer is the explicit gesture of "I know I'm referencing a preview type here" though,
Having logic that attempts to minimize the suppressing users do requires us to essentially auto-suppress scenarios on users' behalf.

I don't agree. The analyzer is meant to tell users: "You are using or exposing a preview feature and are not yourself annotated as preview". The expected user action here is to annotate the relevant parts as preview using the RequiresPreviewFeatures attribute.

Suppression, in general, is a user saying something like "this warning is wrong" or "I know what I'm doing". It should not be a normal or expected action and should not be required to use the feature in "core" scenarios.

They can be suppressed around individual references, blocks of code, entire files, projects, or solutions

I believe this only works when you have logical hierarchies where a given level has clear semantics on how it impacts types. I don't think suppressions fit this bill.

For example, when you look at attributes:

• Placing it at the assembly level impacts everything in the assembly
• Placing it at the type level impacts everything in the type
• Placing it at the member level impacts the member

Therefore, if you want to treat the entire assembly as preview you put it at the assembly level. If you want to treat the entire type as preview you put it at the type level, etc.

If you take the same logic with regards to suppressions then suppressing at a given level means that the warning is invalid or should be ignored at that level. This means that suppressing at the type level, such as because you implement a preview interface, means that nothing in the type should be considered "preview" which adds risk and can lead to bugs on many levels. This, to me, means it is explicitly negative value to do so and should not be expected outside of extreme scenarios and is worse than having an analyzer that gets improved over time and which may introduce additional warnings in future SDKs. We already have plenty of analyzers that do change from release to release and which catch new issues. Likewise, it is realistically only around preview features in which case users should already be expected potential breaks when changing SDK versions or dependencies (which includes build time dependencies, such as analyzers or source generators).

---

For cases like Int32, this means that we need a mechanism through which users can successfully implement a preview interface without also annotating or suppressing the entire type. The normal scenario, as I'd expect, it is that the user can explicitly annotate each member coming from the interface. This works well, gives users a normal/expected scenario for the type, and avoids any issues.

If you have a preview interface which defines a contract for some members that are already public/exposed (e.g. IMinMaxValue for DateTime), you can avoid suppression by explicitly implementing the interface. This allows the existing public member to be non-preview but the interface implementation to be preview.

The remaining case is that of marker interfaces. Marker interfaces are already fairly rare but a decision has to be made around whether they need the type to be marked preview or only if usages/casts to that type need to be preview. In the former case (the type must be preview because no methods exist to be annotated) users have to suppress if they implement a marker interface. In the latter, you can freely implement a marker interface without being preview but need to be preview anywhere you directly handle such an interface. I believe the latter most closely matches the behavior of the first case (non marker interfaces).

We then have a couple remaining scenarios:

• Preview members
• Abstract classes

For preview members, its relatively straightforward. They are marked preview and you must be preview to invoke/access them.

However, abstract classes are a bit different due to how they are represented in the type hierarchy and how they are exposed to the user. They can contain fields, have members that are directly accessible/exposed (with no option to differ, unlike with interfaces), etc. They are distinct from interfaces and so it makes sense, IMO, for these to require the inheriting type to be marked as preview if they themselves are preview.

All of this also fits in with the normal "breaking change" rules for types and adding new members. If the entire type is preview, its safe, otherwise you have to consider the break as "normal" and that things like adding new abstract methods to an existing abstract class is "breaking", even if that single member is marked Preview (and same with going from non-preview to Preview for an existing type).

[jeffhandley]

Thanks, @tannergooding. This is good input, and I appreciate where you're coming from and what your goals are, which I believe include the following:

1. Minimize the likelihood of consumers of preview APIs from inadvertently taking dependencies on them in non-preview features
2. Balance the annoyance vs. value of the analyzer
3. Ensure that we ourselves are responsible users of the analyzer/attributes/suppression and that we protect ourselves from leaking preview features

Suppression, in general, is a user saying something like "this warning is wrong" or "I know what I'm doing". It should not be a normal or expected action and should not be required to use the feature in "core" scenarios.

I believe most of the disagreement here circles around the "I know what I'm doing" category and where that intersects with a "core scenario." For ourselves, we have preview feature usage in core scenarios where naïve analyzer would lead us to suppress, because we know what we're doing. A for suppressing (#pragma warning disable style) is when we are indicating that a type (Int32) implements a preview interface. It boils down to whether or not we should need a #pragma warning disable around the interface name.

Interestingly, those exact same code blocks already have #pragma warning disable SA1001 around them because our commas are formatted differently from usual. That's a case of "I know what I'm doing" and it's OK. Suppressing diagnostics from the preview feature analyzer is perfectly acceptable when we know what we're doing and it's a core scenario for us.

I believe this only works when you have logical hierarchies where a given level has clear semantics on how it impacts types. I don't think suppressions fit this bill.

I understand your remarks here, that suppressing a broader scopes leads to more likelihood of over-suppressing and introducing issues in the future. I don't discount that. My point with suppression being available at all of those scopes is that users can choose for themselves what is most appropriate for them; they have dials to turn. If they are getting tired of wrapping lots of tiny code blocks in #pragma warning disable, then they can choose to rearrange their code to reduce the number of #pragma statements. They can find the balance that best suits their code cleanliness and protection against leaking preview features. Ourselves, we'd likely opt for very granular #pragma statements to keep our risk minimized, as we have a high tolerance for the statements in our codebase.

The breaking change point I was trying to make is that if the following occurs:

1. We "auto-suppress" a diagnostic because we think it would be too noisy for our users
2. We later find that there are cases that this auto-suppression can lead to usage of preview APIs without diagnostics ever occurring
3. We remove the auto-suppression
4. This is now a source breaking change that must be treated as a breaking change -- we do this with other analyzers too

In that case, we've harmed the user first by letting them leak a preview API, and then again by introducing a source breaking change. This is a troublesome direction to need to go. We can also become more lenient in the future if enough customer reports come in that the analyzer is too noisy, but we can't go in the other direction. We should start off by over-diagnosing though, to make sure we're protecting our users and giving them control over how they want to best suppress for their scenarios.

I also want to echo @stephentoub's remarks about the predictability. If a user references a preview marker interface and gets a diagnostic, they'll recognize they need to handle that scenario (mark themselves or suppress). And then when they reference a non-marker preview interface but they don't get a diagnostic, they won't know why and this can lead to confusion and doubt. The behavior needs to be as simple and predictable as possible--the less nuance the better. Again, we can also introduce nuance in the future if it seems justified by customer feedback, but I don't think we should start there.

[tannergooding]

And then when they reference a non-marker preview interface but they don't get a diagnostic, they won't know why and this can lead to confusion and doubt

This isn't a scenario. They will still get a diagnostic, just in a different location. It can even be in the same location if we are concerned about them differing. The main point is that you should not be required to suppress to deal with this scenario.

I believe that implementing a non-marker preview interface on a non-preview type is a "core scenario". Not just for us, but for generic math and the RequiresPreviewFeatures attribute in general. As such, suppressing should not be the only solution to this problem. It should be equally valid to deal with the warning by explicitly placing RequiresPreviewFeatures on every member implemented for the interface (whether implicit or explicitly implemented).

[terrajobst]

I suggest we stop this threadzilla and chat about this on Thursday in our meeting. Otherwise this will keep going :-)

[buyaa-n]

LGTM

[pgovind]

Closing this draft. The discussion here is not relevant anymore. We now tag all implementations of preview interfaces at the type definition itself.