Skip to content

Remove SecurityCriticalDataForSet #7161

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 4, 2024
Merged

Remove SecurityCriticalDataForSet #7161

merged 1 commit into from
Oct 4, 2024

Conversation

ThomasGoulet73
Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 commented Oct 6, 2022 •
edited by ghost
Loading

Description

Removes SecurityCriticalDataForSet which is unused since the removal of CAS attributes in #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

@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner October 6, 2022 02:51
@ghost ghost assigned ThomasGoulet73 Oct 6, 2022
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Oct 6, 2022
@ghost ghost requested review from dipeshmsft and singhashish-wpf October 6, 2022 02:51
@ghost ghost added the Community Contribution A label for all community Contributions label Oct 6, 2022
@ThomasGoulet73
Copy link
Contributor Author

@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 ?

@singhashish-wpf
Copy link
Member

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@ThomasGoulet73
Copy link
Contributor Author

@dipeshmsft Could I get some info about what failed here ?

@dipeshmsft
Copy link
Member

@ThomasGoulet73, we were not able to investigate the failure yet. Will get back to you with some details.

@dipeshmsft
Copy link
Member

@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 ?

@ThomasGoulet73
Copy link
Contributor Author

@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.

dipeshmsft
dipeshmsft previously approved these changes Jan 19, 2023
View reviewed changes
@dipeshmsft dipeshmsft self-assigned this Feb 2, 2023
@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-SecurityCriticalDataForSet branch from ce9b92e to 712b7f0 Compare September 30, 2023 02:28
@ThomasGoulet73
Copy link
Contributor Author

I rebased to fix the conflicts.

@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-SecurityCriticalDataForSet branch from 712b7f0 to 79b2838 Compare May 1, 2024 02:05
@ThomasGoulet73
Copy link
Contributor Author

I rebased to fix the conflicts.

@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-SecurityCriticalDataForSet branch from 79b2838 to a7f5cd6 Compare September 7, 2024 04:20
@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner September 7, 2024 04:20
@ThomasGoulet73
Copy link
Contributor Author

I rebased to fix the conflicts.

JeremyKuhne
JeremyKuhne previously approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

@pchaurasia14 Another one that we should get in.

JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this pull request Oct 3, 2024
@JeremyKuhne
Remove SecurityCriticalData/Class
c267c07 
These are CAS holdovers from .NET Framework that add unnecessary complexity.

Uses more modern / safer syntax where I've modified code. I've also removed a few asserts where they then fall over on the next line anyway.

Similar to and should follow dotnet#7161 and dotnet#6561
@JeremyKuhne JeremyKuhne mentioned this pull request Oct 3, 2024
Merged
dipeshmsft added a commit that referenced this pull request Oct 4, 2024
@dipeshmsft
Remove SecurityCriticalData/Class
47acd81 
These are CAS holdovers from .NET Framework that add unnecessary complexity.

Uses more modern / safer syntax where I've modified code. I've also removed a few asserts where they then fall over on the next line anyway.

Similar to and should follow #7161 and #6561
dipeshmsft
dipeshmsft previously approved these changes Oct 4, 2024
View reviewed changes
@dipeshmsft dipeshmsft dismissed stale reviews from JeremyKuhne and themself via 43be312 October 4, 2024 18:03
@h3xds1nz
Copy link
Member

h3xds1nz commented Oct 4, 2024

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.

@dipeshmsft
Copy link
Member

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.

@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-SecurityCriticalDataForSet branch from 43be312 to 4363c30 Compare October 4, 2024 18:56
@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-SecurityCriticalDataForSet branch from 4363c30 to a1aa355 Compare October 4, 2024 18:59
@dipeshmsft dipeshmsft merged commit 9fe50c7 into dotnet:main Oct 4, 2024
8 checks passed
@dipeshmsft
Copy link
Member

Thanks @ThomasGoulet73 for the quick turnaround on the rebase request. Once again, looking forward to other contributions from you.

@ThomasGoulet73
Copy link
Contributor Author

Thanks @dipeshmsft.

@ThomasGoulet73 ThomasGoulet73 deleted the remove-SecurityCriticalDataForSet branch October 4, 2024 19:18
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this pull request Oct 4, 2024
@JeremyKuhne
Remove SecurityCriticalData/Class
50810e5 
These are CAS holdovers from .NET Framework that add unnecessary complexity.

Uses more modern / safer syntax where I've modified code. I've also removed a few asserts where they then fall over on the next line anyway.

Similar to and should follow dotnet#7161 and dotnet#6561
dipeshmsft pushed a commit that referenced this pull request Oct 7, 2024
@JeremyKuhne
Remove SecurityCriticalData/Class (#9882)
278c355 
* Remove SecurityCriticalData/Class

These are CAS holdovers from .NET Framework that add unnecessary complexity.

Uses more modern / safer syntax where I've modified code. I've also removed a few asserts where they then fall over on the next line anyway.

Similar to and should follow #7161 and #6561

* Address feedback
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dipeshmsft dipeshmsft dipeshmsft approved these changes

@JeremyKuhne JeremyKuhne JeremyKuhne left review comments

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

Assignees

@ThomasGoulet73 ThomasGoulet73

@dipeshmsft dipeshmsft

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ThomasGoulet73 @singhashish-wpf @dipeshmsft @h3xds1nz @JeremyKuhne