-
Notifications
You must be signed in to change notification settings - Fork 247
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
Hide protected and overridable members from public projections #1319
Conversation
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. |
Bump |
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. |
Per discussion thread, reopening |
@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. |
There was a problem hiding this 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.
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. |
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. |
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. |
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. |
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 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. |
Yeah, and namespace filtering is non-trivial to implement. |
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]>
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). |
Are those breaking changes perhaps caused by said projects using protected/overridable members? :P |
Probably. They are using something from 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. |
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.
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.
According to the WinRT type system:
Previously, cppwinrt allowed anyone to directly call protected/overridable members, for example this code works
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