Skip to content

Hollowed out public types extending from System.Security.Permissions types #869

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

Merged
merged 11 commits into from
Jun 13, 2019

Conversation

ojhad
Copy link
Contributor

@ojhad ojhad commented Jun 4, 2019

Currently this PR does not include Type Forwarding of these types

Part of Issue #241
Hollowing out public types that should be moved out of WPF to System.Security.Permissions.dll

@ojhad ojhad requested review from rladuca and vatsan-madhavan June 4, 2019 22:00
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 4, 2019
@dnfclas
Copy link

dnfclas commented Jun 4, 2019

CLA assistant check
All CLA requirements met.

@ojhad ojhad added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 4, 2019
@vatsan-madhavan
Copy link
Member

Your build is passing, but tests are failing (so, well, the build is not passing, really). Do you have a sense for what's going on?

   at DRT.DrtBase.ReportException(Object o) in /_/src/Microsoft.DotNet.Wpf/test/Common/TestServices/DrtBase.cs:line 2102
   at DRT.DrtBase.Run(String[] args) in /_/src/Microsoft.DotNet.Wpf/test/Common/TestServices/DrtBase.cs:line 296
   at DrtXaml.XamlDrt.RunTests(String[] args) in /_/src/Microsoft.DotNet.Wpf/test/DRT/DrtXaml/DrtXaml/DrtXamlBase.cs:line 99
   at DrtXaml.TestFrontEnd.<>c__DisplayClass1_0.<RunSuite>b__0() in /_/src/Microsoft.DotNet.Wpf/test/DRT/DrtXaml/DrtXaml/DrtXamlBase.cs:line 70
   at DrtXaml.TestFrontEnd.<>c__DisplayClass2_0.<StartSTATask>b__0() in /_/src/Microsoft.DotNet.Wpf/test/DRT/DrtXaml/DrtXaml/DrtXamlBase.cs:line 80
--- End of stack trace from previous location where exception was thrown ---
   at DrtXaml.TestFrontEnd.RunSuite(String[] data) in /_/src/Microsoft.DotNet.Wpf/test/DRT/DrtXaml/DrtXaml/DrtXamlBase.cs:line 70
--- End of stack trace from previous location where exception was thrown ---

@vatsan-madhavan vatsan-madhavan requested a review from ericstj June 5, 2019 17:52
@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Jun 5, 2019

@ericstj , We'd like to move these types to System.Security.Permissions, and type-forward to it. I think we'd want to also make these dangling-references from WindowsDesktop. App. Thoughts?

@ericstj
Copy link
Member

ericstj commented Jun 5, 2019

We'd like to move these types to System.Security.Permissions, and type-forward to it

Excellent, please open a PR.

make these dangling-references from WindowsDesktop. App

What kind of dangling-references? Dangling type-forwards: sure. We do the same for our mscorlib.dll facade. Dangling type-refs from attribute applications / actual usage: no-go : JIT would throw.

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Jun 5, 2019

What kind of dangling-references? Dangling type-forwards: sure. We do the same for our mscorlib.dll facade. Dangling type-refs from attribute applications / actual usage: no-go : JIT would throw.

Dangling type-forwards only.

The goal is to eliminate all actual type-refs before RTM and end up with ref assemblies that didn't require S.S.Permissions to build (module type-forwards). You'll start seeing a related PR's to get to this shortly (I'll cc you)

@vatsan-madhavan
Copy link
Member

@ojhad, once the changes are made and the test failure is figured out, let's merge the changes into WPF and then move the dummied-out impl. to corefx s.s.permissions.

@ericstj
Copy link
Member

ericstj commented Jun 6, 2019

Be aware that we're trying to lock down all API changes in CoreFx by Preview7 which means this would need to make it into corefx in the next couple weeks. For a move like this, I'd recommend adding types in CoreFx, letting them flow upstack, then swap from type-def to typeforward when the packages reach WPF. It does mean that we'll have a couple SDK builds with the types in both places, but that only breaks apps trying to compile against them which we don't expect a lot of folks to need to do.

@ojhad ojhad removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 10, 2019
@ojhad ojhad added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 11, 2019
@vatsan-madhavan vatsan-madhavan removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 13, 2019
@ojhad ojhad merged commit 8ac1093 into master Jun 13, 2019
@@ -16,6 +16,10 @@ public sealed class XamlLoadPermission : CodeAccessPermission, IUnrestrictedPerm
public XamlLoadPermission(PermissionState state) { }
public XamlLoadPermission(XamlAccessLevel allowedAccess) { }
public XamlLoadPermission(IEnumerable<XamlAccessLevel> allowedAccess) { }
[ComVisible(false)]
public override bool Equals(object obj) { return false; }
Copy link

@weltkante weltkante Jun 13, 2019

Choose a reason for hiding this comment

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

This is not a valid implementation because x.Equals(x) returns false, which will cause all sorts of bugs if anyone calls this method. You should use ReferenceEquals(this, obj) or throw an exception if you don't want to support equality.

Copy link
Member

Choose a reason for hiding this comment

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

This impl. will get deleted from dotnet/wpf shortly, and we'll consume the one from dotnet/corefx#38405. Let's make sure we get the corefx PR right.

@vatsan-madhavan vatsan-madhavan deleted the user/ojhad/securitypermissionspublicapi branch August 23, 2019 20:12
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants