Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide protected and overridable members from public projections #1319

Merged
merged 10 commits into from
Jul 12, 2023

Conversation

sylveon
Copy link
Contributor

@sylveon sylveon commented Jun 4, 2023

According to the WinRT type system:

Protected interfaces

A composable class may declare zero or more of its member interfaces to be protected. A non-composable class may not declare member interfaces to be protected. Only code in a class that composes a composable class (directly or indirectly) may query for and use interfaces that the composable class declares as protected. Code from outside the composition chain may not query for, nor use, interfaces that the composable class declares as protected.

For example, if UIElement declares a protected interface IUIElementProtected, then only classes that compose UIElement—including both direct (Control) and indirect (Button) composition—may query for and use the IUIElementProtected interface.

Overridable interfaces

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. However, where a protected interface may only be implemented by the class that originally declared it, overridable interfaces may be re-implemented by classes that compose the class that implemented the overridable interface.

Previously, cppwinrt allowed anyone to directly call protected/overridable members, for example this code works

StackPanel panel;
panel.ArrangeOverride({ }); // works, despite this being overridable
panel.ProtectedCursor(); // works, despite this being protected

This change makes it so that only implementation types which derive from the base have access to protected and overridable members.

This change is potentially breaking, but code that this change breaks should've never compiled to begin with (and indeed the equivalents in C++/CX as well as .NET Framework and .NET Core do not build).

In worse case, if access to protected and overridable members is desired (despite this being against the WinRT type system), one can do base.as<IBaseProtected>().ProtectedMember()

There's a potential gain to compile-time when Windows.UI.Xaml is involved, since a bunch of consume_t classes for overridable/protected members no longer get instantiated.

Fixes: #1317

@github-actions
Copy link

This pull request 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
Copy link
Contributor Author

sylveon commented Jun 16, 2023

Bump

@kennykerr
Copy link
Collaborator

Unfortunately, a project maintainer is not currently available to review this pull request. Please see the contributing guide for more information. Feel free to keep the conversation going on the related issue.

@jonwis
Copy link
Member

jonwis commented Jun 23, 2023

Per discussion thread, reopening

@kennykerr
Copy link
Collaborator

@jonwis - this change is going to be harder to review as it changes code gen which can't be directly reviewed just by looking at the PR even with good test coverage. I tend to take such PRs, clone the branch, and manually verify the code gen looks sound. Its tedious but the repercussions from bad code gen can be expensive if detected later on.

p.s. for windows-rs I opted to include code gen in the repo for test verification and diffing which helps to automate this.

@kennykerr kennykerr requested a review from jonwis June 26, 2023 17:13
Copy link
Member

@ChrisGuzak ChrisGuzak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of the safety this adds.

@sylveon
Copy link
Contributor Author

sylveon commented Jun 27, 2023

I thought I had forgot about protected constructors, but it turns out protected constructors are already hidden from code which doesn't compose. I added test coverage for it as I couldn't find any.

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

This pull request 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.

@jonwis
Copy link
Member

jonwis commented Jul 8, 2023

I'll take this on Monday to compare the generated headers and some sample compiled code. I like the idea that we add some kind of "diff output" script that uses the current release version and the just-built version then your favorite directory-diffing tool to see what changed.

The source-breaking aspect has me concerned given how large the Windows OS codebase is and how widespread protected members are in WinUI3.

I know we don't like behavioral modification switches that change codegen, but this definitely is a place where it might be better to allow a "enable protected member access by namespace filter" control out.

@sylveon
Copy link
Contributor Author

sylveon commented Jul 8, 2023

how widespread protected members are in WinUI3.

Most WinUI 3 code is in C#, where protected members are hidden. I'd expect the amount of C++ developers which are actively using protected members against guidance to be fairly low.

@kennykerr
Copy link
Collaborator

enable protected member access by namespace filter

There is no precedent for per-namespace alterations of this kind. I would suggest you start without an escape hatch and see how bad the breaks are in practice by performing an OS build. You may find that they are easily manageable. Once you have a good idea of what the practical implications are, add an escape hatch as needed and only if absolutely necessary. The approach C++/WinRT has used in the past is to use a preprocessor definition like WINRT_LEAN_AND_MEAN. This will be a lot simpler to implement and manage.

https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/macros

This is how I upgraded the OS from C++/WinRT 1.0 to 2.0. But again, you need to get the OS repo up to date first so that you can easily tell the delta of this change specifically. Otherwise, you will just be fighting a slew of random breaks.

@sylveon
Copy link
Contributor Author

sylveon commented Jul 8, 2023

Yeah, and namespace filtering is non-trivial to implement.

@jonwis
Copy link
Member

jonwis commented Jul 10, 2023

OK, sylveon#1 adds new differencing tools to compare the latest release and the under-development codegen. The changes here seem expected - mostly limited to XAML types, for instance. Will need to ingest into the OS carefully.

* Add tooling to difference current and updated output

* Update readme.

Turns out the cppwinrt project depends on prebuild already

* Use powershell instead of findstr

* Speed up invoke-webrequest with less progress

---------

Co-authored-by: Jon Wiswall <[email protected]>
@kennykerr kennykerr merged commit 0958cf3 into microsoft:master Jul 12, 2023
96 checks passed
@dmachaj
Copy link
Contributor

dmachaj commented Dec 15, 2023

Unfortunately this does seem to be a breaking change, at least for some projects :(. I'm trying to build our code base with a private cppwinrt.exe that I built from the HEAD of master and I have some build breaks that seem to match this signature. (The project is currently on the most recent published release 2.0.230706.1 which narrowly missed this change).

@sylveon
Copy link
Contributor Author

sylveon commented Dec 16, 2023

Are those breaking changes perhaps caused by said projects using protected/overridable members? :P

@dmachaj
Copy link
Contributor

dmachaj commented Dec 16, 2023

Probably. They are using something from Xaml::Controls::IControlProtected. It even has Protected in the name 🤷.

Unfortunately this usage is a pattern established in an (internal) project template so there are dozens of components that will have this same break to fix. At least this particular fix is not especially difficult.

dlech added a commit to pywinrt/pywinrt that referenced this pull request Apr 28, 2024
Since microsoft/cppwinrt#1319, CppWinRT doesn't
allow calling protected methods so compiling was failing.

These should not have been generated in the first place, so remove them
from the projection.
dlech added a commit to pywinrt/pywinrt that referenced this pull request Apr 28, 2024
Since microsoft/cppwinrt#1319, CppWinRT doesn't
allow calling protected methods so compiling was failing.

These should not have been generated in the first place, so remove them
from the projection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Protected members are exposed publicly
5 participants