Quick Info representation for flow analysis annotations

[RikkiGibson]

When a method has flow analysis attributes it may be worthwhile to somehow display them in Quick Info.

The most obvious approach would be something like:

bool string.IsNullOrEmpty([NotNullWhen(false)] string? value)

Obviously Quick Info "real estate" is very valuable and we want to make sure this is providing more value than noise.

/cc @jasonmalinowski @sharwell

[CyrusNajmabadi]

This feels like noise to me. We haven't shown other language-relevant attributes before. I don't think we'd need that here. As someone using nullbale, this isn't a question I'm trying to find out when I look at a signature

[jasonmalinowski]

@sharwell has been working on showing additional sections in Quick Info, including the <returns> section. I wonder if we're best showing something like "returns false when value is null" or something based on the attributes.

[CyrusNajmabadi]

When/why do I care?

Honestly, this just doesn't seem to be something thought about. Is really a sublanguage to give a better experience around a few apis between the API and the compiler.

I don't see it as something users themselves need to think or care about when wanting to get info on an API.

[RikkiGibson]

At a minimum you care when you are trying to figure out why the compiler is giving some nullability warning, yet your code appears to be null-safe. The presence or absence of flow attributes tells both the compiler and users what to expect w.r.t. nullability and it’s inevitable that people dig into some of the details when troubleshooting their warnings.

this doesn’t mean that flow attributes definitely should be in quick info, but on some level it seems to me they are not that much different than showing the ‘?’ annotation for nullable reference types.

[jasonmalinowski]

I'll also say it's useful when annotating code: the extra quick info is great when you're looking at code and trying to figure out how to annotate the public API. If I see this helper calls another helper, and that helper is already annotated with these attributes, then I know I can carry them along too.

(I'm not sure if that's enough justification though, just that when doing large annotation PRs I otherwise find myself doing go to def a lot)

[CyrusNajmabadi]

and it’s inevitable that people dig into some of the details when troubleshooting their warnings.

Sure. But go-to def is a good solution for the niche case. If you want to dig-in, then do so. But that's different from deciding to elevate.

[RikkiGibson]

I feel this relates to #38329. A user was confused about why they were getting a nullable warning for an 'out' parameter.

In this scenario, if a user just "obeys" Quick Info and types the variable declaration pattern to match the out parameter of TryGetValue, they end up getting a nullable warning. So in effect we are failing to guide users into doing the right thing.

It's possible we would just want to lie to them and say the type is parameter type is 'string?'. But there might be some way to be more specific and convey "may be null if method returns false".

[jmarolf]

I feel this relates to #38329. A user was confused about why they were getting a nullable warning for an 'out' parameter.

I agree that nullable and QuickInfo are kinda weird now. Nullable is an addition to the type system that user is essentially blind to in the IDE. The point of QuickInfo (imho) is to help the user know what they should type/do-to-fix-something without needing to navigate to the source definition. Don't know if QuickInfo is where we want to show this but today I basically always need to navigate to the source definition because QuickInfo does not tell me enough by itself.

[RikkiGibson]

Another scenario that has come to mind is what return type to show if a NotNullIfNotNull method is called using an argument with a not-null flow state. It feels to me like if a user calls a method and all they see is that the return is maybe-null, they will probably go to null check it because that's what the IDE is instructing that they need to do. But the utility of NotNullIfNotNull was supposed to be that we know that the user doesn't need to null-check the return.

If a user updates to a new library version that adds a NotNullIfNotNull annotation, it may be confusing that some call sites will have a nullable warning on usage of the return value (due to a maybe-null argument) and others will not.

[dibarbet]

Resolution: We would take PRs for this with a well documented strategy for when it shows.

1. We should show things the compiler already tracks for overrides / hides / inherits.
2. We should omit attributes when we have a better option like AllowNull (could just show ?).