wpf
wpf copied to clipboard
dotnet
Remove SecurityCriticalDataForSet
Description
Removes SecurityCriticalDataForSet which is unused since the removal of CAS attributes in dotnet/wpf#994.
Customer Impact
Should have tiny performance gain (Probably not measurable), it's mostly about removing useless code.
Regression
No.
Testing
Local build + CI + I tested a simple app.
Risk
Low.
Microsoft Reviewers: Open in CodeFlow
@dotnet/wpf-developers The tests still fail to start even though it works in #7164 (Opened a day later). Maybe it's because my PR is opened from a fork ?
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
@ThomasGoulet73, we were not able to investigate the failure yet. Will get back to you with some details.
@ThomasGoulet73 we investigated the test failure and it turns out that our test relies on SecurityCriticalDataForSet for asserting the behaviour ( well technically it is using reflection to find out the underlying value backed by SecurityCriticalDataForSet )
While we are inclined to fix our tests so they don't use reflection and we can directly access the value, however, we believe that there might be applications that may be using a similar pattern. And this change maybe a breaking change for applications upgrading to .NET 8.
Thoughts ?
@dipeshmsft I don't think changing internal members should be considered as a breaking change. In my opinion, third-party shouldn't expect internal implementation to not change because then any changes become breaking changes. Also, I took a look at the breaking change documentation for dotnet/runtime and it looks like this PR falls in Bucket 4 which means it is usually acceptable. I believe this change is still worth it but I'll let the team decide.
So happy this is getting in (and the other ones), but I'm gonna have a nice time resolving merge conflicts on most of my PRs, haha.
Hey @ThomasGoulet73, can you go ahead and rebase to fix the conflicts. I have reviewed it, and we are going to merge it. I was trying to resolve it myself but it didn't work and now I got confused in all the branches that I have here.
Thanks @ThomasGoulet73 for the quick turnaround on the rebase request. Once again, looking forward to other contributions from you.