Bug: Protected members are exposed publicly

[sylveon]

Version

2.0.230524.4

Summary

If you declare a protected member on a WinRT class, the projection type will directly expose this protected member to all consumers, meaning it is trivial to accidentally do things you shouldn't.

Reproducible example

[default_interface]
runtimeclass Class
{
    Class();
    protected Int32 MyProperty;
}

Class a;
int b = a.MyProperty();

Expected behavior

A build error saying MyProperty doesn't exist or is protected

Actual behavior

It correctly builds and will actually call implementation::Class::MyProperty.

Additional comments

No response

[sylveon]

It should also be the same thing for overridable interfaces, from the type system spec:

A composable class may declare zero or more of its member interfaces to be overridable. An overridable interface may only be queried for, and used within, a composition chain—similar to the rules about accessing protected interfaces detailed previously.

I have a PR for this I can send tomorrow.

[github-actions[bot]]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Bump. Waiting on PR.

[kennykerr]

I've tagged a few project maintainers. If one of them is available to offer guidance and mentorship for this issue, they will reach out according to the contributing guide to discuss and agree on an approach.

@microsoft/cppwinrt-maintainers

[kennykerr]

Note that visibility is just an illusion in WinRT and callers can always QI for protected interfaces so this isn't all that beneficial IMO. This is also likely to result in a large number of breaking changes, but probably manageable since the workaround would be to cast explicitly.

[sylveon]

Right, everything we do is a best effort only, but we don't need to make perfect the enemy of good ;)

Hiding protected members from Intellisense prevents accidentally relying on protected members without inheriting, makes people think twice before doing this, and easily flags such uses for potentially later fixing (because the cast is obvious). Depending on members that weren't intended to be public could cause surprising behavior and potential bugs, so it's a good thing:tm: to catch these at compile-time.

Since cppwinrt is a library that one must explicitly update, breaking changes will only happen once somebody voluntarily updates, so it is fairly manageable, especially with a clear workaround. I'm curious if this change would reveal any incorrect use in the Windows repo, as that's probably the heaviest user of cppwinrt.

FWIW, cswinrt even hides the protected interfaces (makes them internal to the projection assembly), so it's even "harder" to access protected members in C#.

[jonwis]

I suggest we do an "enforce author intent by default" here (where protected members are not projected to consumers) with an opt-out flag that can be applied on a per-namespace basis. That way we lock in as much goodness by default as possible, with the escape hatch for people who need to pick up the new cppwinrt but can't yet update all their code.

[kennykerr]

I'm not fond of such options mainly because they're hard to maintain. The more toggles we added to cppwinrt the harder it becomes to maintain and support due to the test/support/debug matrix getting ever larger. That's why @oldnewthing and I have steadily tried to remove stuff like WINRT_NO_MAKE_DETECTION and just let cppwinrt "do the right thing". In this case, the workaround is easy enough - object.Method() just becomes object.as<IProtected>().Method() and no escape hatch is needed - so if honoring "protected" is goodness let's just do it and not sit on the fence. Folks who can't yet update can just stay where they are.

[sylveon]

If that's the alignment, somebody can reopen the PR I did - I can't reopen it myself if a repo maintainer closed it.

[github-actions[bot]]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Waiting on PR review.