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

Whitelist System.ICloneable #7326

Open
rndtrash opened this issue Jan 5, 2025 · 5 comments
Open

Whitelist System.ICloneable #7326

rndtrash opened this issue Jan 5, 2025 · 5 comments
Labels
whitelist Add something to the whitelist

Comments

@rndtrash
Copy link

rndtrash commented Jan 5, 2025

What it is?

Whitelist Error: System.Private.CoreLib/System.ICloneable.Clone()

Why do you need to use this?

System.ICloneable is a standard interface for when you need to make a copy of an object, be it deep or shallow.

It is absolutely not necessary, I can make my own interface, but at the same time I don't really want to reinvent a wheel, and the ICloneable interface is useful for the generic functions:

    public static Dictionary<T, U> DeepClone<T, U>( this Dictionary<T, U> dict ) where T : class, ICloneable where U : class, ICloneable => dict
            .Select( ( pair ) => new KeyValuePair<T, U>( pair.Key.Clone() as T, pair.Value.Clone() as U ) )
            .ToDictionary();
@rndtrash rndtrash added the whitelist Add something to the whitelist label Jan 5, 2025
@github-project-automation github-project-automation bot moved this to To triage in s&box tracker Jan 5, 2025
@LeQuackers
Copy link

LeQuackers commented Jan 6, 2025

Could have sworn the C# docs said ICloneable wasn't recommended because it's not clear if the implementer is returning a deep copy or shallow copy.

edit:
It's not recommended in public APIs:
System.ICloneable - Notes to Implementers

Because callers of Clone() cannot depend on the method performing a predictable cloning operation, we recommend that ICloneable not be implemented in public APIs.

Having this whitelisted or a Sandbox.ICloneable equivalent with a DeepClone and ShallowClone method would still be nice though.

@rndtrash
Copy link
Author

rndtrash commented Jan 6, 2025

...a Sandbox.ICloneable equivalent with a DeepClone and ShallowClone method would still be nice though.

I think that it is not really possible since the built-in classes like string already implement ICloneable, and you can't even make a wrapper that implements said Sandbox.ICloneable because string is sealed.

@LeQuackers
Copy link

I've never heard of someone calling Clone() on a string; if you look at the source it just returns itself.

If you look at what's derived from System.ICloneable in the docs, you'll see that most of it is stuff you'd never touch or probably won't actually clone (with the exception of Array). The System.Net.Http.Headers stuff is probably an exception as well, but ICloneable is implemented explicitly, so it's probably not intended to be called publicly.

With the exception of Array, I can't think of any existing public types that you'd actually call Clone() on; I was under the impression that this whitelist request was for implementation purposes. I suggested a Sandbox.ICloneable equivalent to discern shallow cloning and deep cloning, which avoids the main pitfall of System.ICloneable.

@rndtrash
Copy link
Author

rndtrash commented Jan 6, 2025

I was under the impression that this whitelist request was for implementation purposes.

Indeed, I just wanted my generic method to be usable for both the built-in types and for my own classes that actually implement a copying functionality.

@LeQuackers
Copy link

After thinking about this some more, Sandbox.ICloneable should probably be used instead of whitelisting System.ICloneable. System.ICloneable implementations tend to use MemberwiseClone() under the hood, which might lead to some security issues (not a security expert, just a gut feeling).

What built-in types do you need to clone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whitelist Add something to the whitelist
Projects
Status: To triage
Development

No branches or pull requests

2 participants