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

Immutable/readonly structs (e.g. NSRange) #5110

Open
Therzok opened this issue Nov 8, 2018 · 6 comments
Open

Immutable/readonly structs (e.g. NSRange) #5110

Therzok opened this issue Nov 8, 2018 · 6 comments
Labels
breaking-change If an issue or a pull request represents a breaking change enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS macOS Issues affecting Xamarin.Mac
Milestone

Comments

@Therzok
Copy link
Contributor

Therzok commented Nov 8, 2018

This is not really a bug, but more of an open discussion about how some things are bound.

Take for example, NSRange: https://github.com/xamarin/xamarin-macios/blob/master/src/Foundation/NSRange.cs#L27

The struct is guidelines are for the struct to be immutable. (https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/struct)

Cocoa API doesn't seem to let it be mutated either: https://developer.apple.com/documentation/foundation/nsrange?language=objc

In case of structs, when a struct is in a readonly field/property or is passed as arguments to in parameters, the compiler generates a defensive copy.

This happens because this is mutable by default.

C# 7.2 has some nice-ness that allows for structs to be marked as readonly, thus not allowing the this variable to be set, but that requires the property to be fully readonly.

Why was NSRange designed mutable? Might be the case for most structs bound

@chamons chamons added the breaking-change If an issue or a pull request represents a breaking change label Nov 8, 2018
@chamons
Copy link
Contributor

chamons commented Nov 8, 2018

We can't change the API of NSRange to be immutable until XAMCORE_4_0 I believe.

@chamons chamons mentioned this issue Nov 8, 2018
44 tasks
@chamons chamons added the question The issue is a question label Nov 8, 2018
@dalexsoto dalexsoto added this to the Future milestone Nov 8, 2018
@spouliot spouliot modified the milestones: Future, XAMCORE_4_0 Nov 9, 2018
@spouliot spouliot added enhancement The issue or pull request is an enhancement and removed question The issue is a question labels Nov 9, 2018
@spouliot
Copy link
Contributor

spouliot commented Nov 9, 2018

Why was NSRange designed mutable?

@Therzok Not sure, that was done long ago (in Novell days) and not even in our git history. My guess is that the (then current) documentation and header files did not make that fact clear. E.g. headers used to include fields (not all of them being clear if they were private or not).

If it was to bound today it would be immutable but, right now, it must remain mutable since it would mean a breaking change.

@spouliot spouliot changed the title [Question] Struct definitions and readonly-ness Immutable/readonly structs (e.g. NSRange) Nov 9, 2018
@chamons
Copy link
Contributor

chamons commented Nov 9, 2018

I added it on the XAMCORE_4_0 audit list in case we ever wanted to change it, but since we're unlikely to break all platforms at once it'd be a bit of a flag day change.

@dalexsoto dalexsoto added macOS Issues affecting Xamarin.Mac iOS Issues affecting Xamarin.iOS labels Jan 30, 2019
@rolfbjarne rolfbjarne modified the milestones: XAMCORE_4_0, XAMCORE_5_0 Feb 4, 2022
@rolfbjarne
Copy link
Member

This doesn't seem to give us much for the potential pain of making perfectly good code break, things like this wouldn't compile anymore:

var range = new NSRange () {
    Location = 3,
    Length = 14,
};

which, while it might not be optimal from a performance perspective, it still works just fine.

As such I've postponed this to next time we can make a break (XAMCORE_5_0), maybe there's a more compelling reason to do this at that time.

@Therzok
Copy link
Contributor Author

Therzok commented Feb 4, 2022

I guess this should be something to be discussed with the dotnet folks as well, since the APIs are getting standardized and normalized with dotnet guidelines, I think that this would be a better time to do it than later.

In the issue description, the dotnet guidelines state that mutable value types should be avoided.

@Therzok
Copy link
Contributor Author

Therzok commented Feb 4, 2022

It also is missing IEquatable<T> implementation and possibly other things that would make the struct truly compliant with dotnet guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change If an issue or a pull request represents a breaking change enhancement The issue or pull request is an enhancement iOS Issues affecting Xamarin.iOS macOS Issues affecting Xamarin.Mac
Projects
None yet
Development

No branches or pull requests

5 participants