Deprecate and hide from Intellisense APIs that always return null, like Window.Current

[andrewleader]

Describe the bug

There are several APIs in our API surface that always return null and confuse developers, like Window.Current and also Window.Dispatcher and more. The presence of these non-functional APIs makes it more difficult to code and more difficult to migrate from UWP, since any developer would expect these APIs to work since they're there, but they actually just return null.

These APIs only worked in UWP, which was why we had them, but we need to clean up the API surface.

Expected behavior
These APIs should be removed, or at least marked deprecated. Since we're not going to be able to remove these before 1.0 GA and removing would be a breaking change, we're planning on marking them deprecated and hiding them from intellisense in 1.0 servicing or 1.1, and then removing in 2.0 with #1776.

[mqudsi]

Can you hide them from intellisense (before then) by annotating the declarations?

[riverar]

Prior art:
microsoft/microsoft-ui-xaml#2609
microsoft/microsoft-ui-xaml#4177
microsoft/microsoft-ui-xaml#6027

[andrewleader]

Hiding from intellisense is a good idea @mqudsi, I've added that! We still won't be able to do anything any sooner though, we simply don't have the time to annotate the APIs right now before 1.0 GA. But hopefully we can annotate the APIs soon after 1.0 GA.

[dotMorten]

Perhaps a code analyzer that will error? I realize you can't remove these APIs because that is breaking binary compat, but the APIs are already "broken". One of the largest porting issues coming from UWP is all your code compiles and runs just fine until that specific line of code is called - which you might not actually hit during testing until a customer does. I don't want my app to compile at all if I'm using these broken APIs.

[sylveon]

Analyzers only support C# but this problem is present in C++ too so a solution should consider both languages.

[Scottj1s]

Removing an API from Intellisense does not hide it from Auto-Complete, which is based on the metadata. It merely removes the documentation (summary and returns). The API could be deprecated in the metadata, but there's uneven support for that in client apps (neither C++/WinRT nor C#/WinRT projections implement it). C# has the EditorBrowsableState attribute to hide an API from the editor (Intellisense and Auto-Complete both), but that's been broken for some time, with no fix in sight. C++ has no comparable facility. So, if we choose to remove these APIs, it would come as a rude shock in a version 2.0.

[dotMorten]

it would come as a rude shock in a version 2.0.

not really.

1. You can remove APIs in major versions
2. No one is using these APIs. They’re non-functional already

[sylveon]

It's certainly doable to add deprecated metadata support from the C++ side, we already have codegen based on attributes (for noexcept). One could add [[deprecated("reason")]] too.

Outright removal is the most ideal path (a build break makes sure people stop using those APIs), but my concern with outright removal would be ABI stability.

[dotMorten]

I agree ABI stability is super important but because these APIs are already non functional I think this is worthy of an exception. Anyone already calling these would have non-working code.
It’s quite a big productivity killer to be wasting time on using APIs that doesn’t work . It has made porting code from uwp really hard because we don’t know things doesn’t work until that line of code is hit, rather than getting a build error.

The alternative is to make the APIs functional.

[sylveon]

I think this is worthy of an exception. Anyone already calling these would have non-working code.
It’s quite a big productivity killer to be wasting time on using APIs that doesn’t work

Figuring out ABI mismatches in C++ is also quite the productivity killer. They don't show up as obvious runtime errors like in C#. They show up as weird runtime crashes and corruptions. And it's not just something that affects people who where using the deprecated APIs. Because it offsets the entire vtable, it affects practically everybody. Let's not break ABI.

[riverar]

These APIs are broken guys. What are we even talking about here? Fix them or delete them. No one cares about this ABI.

[riverar]

The usability win from deleting this very confusing dev UX, in my opinion, trumps the ABI break.

[sylveon]

No one cares about this ABI.

Isn't the WASDK ABI stable, or am I wrong?

[riverar]

I don't think the team has made any guarantees to that effect but I do think they try to follow semver at times. I'm okay with the team assuming the ABI stable and wait until 2.0 to delete the API for good. But we should start the process of at least signaling intent to delete this error (if fixing it is out of the question).

[Scottj1s]

Good points all. Ok, at a minimum we can update the Intellisense text to indicate the API is going away. And then we can remove in v2.0. Not a perfect solution, but should be acceptable?

[sylveon]

I feel like adding [Windows.Foundation.Metadata.DeprecatedAttribute] in metadata and updating the projections to mirror this in the generated code would be better. As C++/WinRT doesn't support IntelliSense docs, such a change would not show up in C++.

If we wanted to, we could actually make use of DeprecationType.Remove, and code both projections to leave an offset in the vtable for the method, but not project it. That way the member does not show up to consumers, without making an ABI break.

[Scottj1s]

@sylveon C++/WinRT had projection support for the deprecated attribute at one time. But there were a number of issues with the MSVC compiler not supporting it, so @kennykerr removed it. And an issue with changing projections (C++/WinRT and C#/WinRT) is that client apps would have to update to see the new behavior. So, we'd still need a story for clients that don't update projections. It's probably not worth the trouble, since as @riverar and others point out, these APIs are broken, so should not be called in the first place.

[sylveon]

Doesn't WASDK ship with the CsWinRT projection assembly? C# people would get the updated projection with a WASDK update.

As for C++, needing to update to see the change is better than not seeing it at all (which would be the case if deprecation is only signaled by Intellisense docs). MSVC's support for [[deprecated]] has improved. The STL makes extensive use of it: https://github.com/microsoft/STL/blob/3c1f3d79ea8db9f20ef0854704e3eb131972a606/stl/inc/yvals_core.h#L940

It's probably not worth the trouble, since as @riverar and others point out, these APIs are broken, so should not be called in the first place.

How are people going to know not to call those functions without the compiler telling them? Intellisense docs helps, but migrating UWP code won't have people recheck the Intellisense doc for every function. A deprecation warning or missing member is however immediately obvious.

[Scottj1s]

Yes, C# clients would get the update. The point is that no functioning app would be calling these APIs in the first place. This isn't a classic deprecation scenario, where an API continues working for some period of time and is then removed. A migrated UWP app would crash immediately. So the need to go through a deprecation phase before removing the API altogether probably isn't necessary.