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

[Proposal]: Checked Operators #4665

Open
1 of 4 tasks
Tracked by #829 ...
tannergooding opened this issue Apr 16, 2021 · 26 comments
Open
1 of 4 tasks
Tracked by #829 ...

[Proposal]: Checked Operators #4665

tannergooding opened this issue Apr 16, 2021 · 26 comments
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@tannergooding
Copy link
Member

tannergooding commented Apr 16, 2021

Checked Operators

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

Specification

https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/checked-user-defined-operators.md

Design meetings

@tannergooding
Copy link
Member Author

CC. @MadsTorgersen, @AlekseyTs, @stephentoub

Please let me know if you believe anything from the meeting was not covered here or if there are any additional changes or clarifications that I can make to the proposal.

@tannergooding
Copy link
Member Author

#686 should be closed once this is championed.

@mburbea
Copy link

mburbea commented Apr 17, 2021

Should it be the other way around, that a user could optionally implement an unchecked operator? Many of the existing types that implement these operators in the bcl seem to employ checked conversions.

And obviously, this could lead to breaking changes if a BCL type implemented a different checked/unchecked operator on an existing type.

@tannergooding
Copy link
Member Author

The proposal here has regular and checked operators because the default compilation settings result in a global unchecked context. However, the proposal as is also doesn't change the meaning of existing code and so any case that exposes a regular operator today that contains checked behavior will not break unless the library/app author explicitly decides to change their code (even if the author adds an explicit checked operator).

IntPtr in particular is interesting and I'd say it isn't a good representative example of the rest of the ecosystem. The only reason these operators exist is because the language wasn't interested in exposing nint and nuint at the time. Had nint/nuint been exposed first or had checked operators existed at the time, the design would almost certainly have differed and worked like the rest of the integer primitives.

@FiniteReality
Copy link

FiniteReality commented Apr 29, 2021

I think that instead of using operator + and a checked operator +, it may be better to have users write unchecked operator + and checked operator +. That way, a naked operator + could be a warning if a checked or unchecked variant is present, or vice-versa.

@tannergooding
Copy link
Member Author

unchecked operator + and operator + need to be the same thing for back compat on existing types and code. So erroring would just force you to add an unchecked keyword if you declared checked operator + and already had operator +

@FiniteReality
Copy link

My reasoning for disliking the naked operator + is that everybody seems to have a different opinion over whether it should be checked or unchecked; IMO having unchecked operator + and checked operator + matches cleanly with the current syntax (there's both unchecked(1 + 2) and checked(1 + 2)) and also becomes part of the method contract.

I agree with needing the naked operator + for backwards compatibility, but IMO going forward it should only be used in contexts where checked-ness has no effect on the operation, e.g. in string concatenation and delegate combination.

@FiniteReality
Copy link

Another advantage of having checked operator + and unchecked operator + over a naked operator + and a checked operator + is that you can retain behavioural backwards compatibility with people who expect operator + to be checked, while still exposing the unchecked behaviour for those who wish to consume it. With the current proposal, that's impossible.

@tannergooding
Copy link
Member Author

tannergooding commented Apr 29, 2021

The main issue is trying to fit this onto 20 years of existing types. In the majority scenario, unchecked is the "default", because its the default setting for the compiler, and you get checked by explicit action.

Today we have op_Addition and that is used in both checked and unchecked contexts. The proposal is to continue using that as is, but to also support op_AdditionChecked for use in checked contexts.
Meaning that if you do checked(x + y) or if you specify /checked+ on the command line (or equivalent option in MSBuild).

Having op_AdditionUnchecked as well adds an additional layer and additional complexity. If it's expected to only be called in explicit unchecked(x + y) and not when /checked- (which is the default), then I think the feature ends up more confusing since you are in an unchecked context but the unchecked operator wasn't called. I would also then assume that /checked+ would not cause op_AdditionChecked to be called (for parity) and it would only happen in checked(x + y), which defeats what I believe is the main use case of the switch and the new operator.

While if its expected instead that op_AdditionUnchecked be called in any unchecked context (either unchecked(x + y) or /checked-), then I don't feel its necessary and the existing op_Addition fills this need. In this scenario, adding op_AdditionUnchecked effectively hides and prevents op_Addition from being used in the majority scenario and so you end up with the same potential "breaking change".

@tannergooding
Copy link
Member Author

There is nothing saying that op_Addition must overflow or not, just like there is nothing saying that op_AdditionChecked must overflow.

If this was on a common interface (IAddable<T>) then BigInteger would implement both and would have neither overflow (because overflow can't happen, just OOM). Likewise, if a type felt it was appropriate to always overflow, that is possible and allowed as well.

Some types (like IntPtr) might take breaking changes where they overflow by default today, but should really only overflow in checked contexts.

@bigredd0087
Copy link

@FiniteReality

... you can retain behavioural backwards compatibility with people who expect operator + to be checked ...

If there was no way to differentiate between checked and unchecked operators before, why would people assume operator + was checked?

@FiniteReality
Copy link

I was thinking that the expression a + b would bind to op_AdditionChecked, op_AdditionUnchecked or op_Addition based on:

  • The presence (or lack thereof) of op_AdditionChecked and op_AdditionUnchecked
  • The value of the /checked flag
  • The explicit checked-ness of the scope or operation.

So, for example:

// /checked+
var x = 1 + 2; // Binds to op_AdditionChecked (checked mode is on, no explicit scope)
var y = checked(1 + 2); // Binds to op_AdditionChecked (checked mode is on, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is on, explicit scope)
var w = delegate { } + delegate { }; // Binds to op_Addition (Delegate doesn't declare checked-ness, as it is unimportant)

// /checked-
var x = 1 + 2; // Binds to op_AdditionUnchecked (checked mode is off, no explicit scope)
var y = checked(1 + 2); // Binds to op_AdditionChecked (checked mode is off, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is off, explicit scope)
var w = delegate { } + delegate { }; // Binds to op_Addition (Delegate doesn't declare checked-ness, as it is unimportant)

@FiniteReality
Copy link

FiniteReality commented Apr 29, 2021

@FiniteReality

... you can retain behavioural backwards compatibility with people who expect operator + to be checked ...

If there was no way to differentiate between checked and unchecked operators before, why would people assume operator + was checked?

That's the problem; because there was no way to differentiate, some people chose to implement them as checked, while others chose to implement them as unchecked. Consumers of these types would expect the behaviour of their code to remain the same, even if the original type decided to expose checked/unchecked operators. As such, changing the meaning of op_Addition to perform unchecked addition when adding an op_AdditionChecked would be a breaking change, when op_Addition previously performed checked addition. Having separate checked and unchecked operators allows people to opt-in to the new behaviour by recompiling their code against a newer version of C#, while still supporting consumers who are stuck in the past.

@tannergooding
Copy link
Member Author

I was thinking that the expression a + b would bind to op_AdditionChecked, op_AdditionUnchecked or op_Addition based on:

I think its worth expanding this table a bit more.

Today we have:

// /checked+, op_Addition defined
var x = 1 + 2;            // Binds to op_Addition          (checked mode is on, no explicit scope)
var y = checked(1 + 2);   // Binds to op_Addition          (checked mode is on, explicit scope)
var z = unchecked(1 + 2); // Binds to op_Addition          (checked mode is on, explicit scope)

// /checked-, op_Addition defined
var x = 1 + 2;            // Binds to op_Addition          (checked mode is off, no explicit scope)
var y = checked(1 + 2);   // Binds to op_Addition          (checked mode is off, explicit scope)
var z = unchecked(1 + 2); // Binds to op_Addition          (checked mode is off, explicit scope)

This proposal adds:

// /checked+, op_Addition defined, op_AdditionChecked defined
var x = 1 + 2;            // Binds to op_AdditionChecked   (checked mode is on, no explicit scope)
var y = checked(1 + 2);   // Binds to op_AdditionChecked   (checked mode is on, explicit scope)
var z = unchecked(1 + 2); // Binds to op_Addition          (checked mode is on, explicit scope)

// /checked-, op_Addition defined, op_AdditionChecked defined
var x = 1 + 2;            // Binds to op_Addition          (checked mode is off, no explicit scope)
var y = checked(1 + 2);   // Binds to op_AdditionChecked   (checked mode is off, explicit scope)
var z = unchecked(1 + 2); // Binds to op_Addition          (checked mode is off, explicit scope)

This means:

  • If op_Addition was already effectively "unchecked" (it didn't overflow), then users get a behavior change if they recompile with /checked+ or already had checked(x + y)
  • If op_Addition was already effectively "checked" (it did overflow), then users only get a behavior change if the library author changes the implementation of op_Addition

You are proposing to also have:

// /checked+, op_Addition defined, op_AdditionUnchecked defined
var x = 1 + 2;            // Binds to op_Addition          (checked mode is on, no explicit scope)
var y = checked(1 + 2);   // Binds to op_Addition          (checked mode is on, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is on, explicit scope)

// /checked-, op_Addition defined, op_AdditionUnchecked defined
var x = 1 + 2;            // Binds to op_AdditionUnchecked (checked mode is off, no explicit scope)
var y = checked(1 + 2);   // Binds to op_Addition          (checked mode is off, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is off, explicit scope)

// /checked+, op_Addition defined, op_AdditionChecked defined, op_AdditionUnchecked defined
var x = 1 + 2;            // Binds to op_AdditionChecked   (checked mode is on, no explicit scope)
var y = checked(1 + 2);   // Binds to op_AdditionChecked   (checked mode is on, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is on, explicit scope)

// /checked-, op_Addition defined, op_AdditionChecked defined,, op_AdditionUnchecked defined
var x = 1 + 2;            // Binds to op_AdditionUnchecked (checked mode is off, no explicit scope)
var y = checked(1 + 2);   // Binds to op_AdditionChecked   (checked mode is off, explicit scope)
var z = unchecked(1 + 2); // Binds to op_AdditionUnchecked (checked mode is off, explicit scope)

For the case where op_Addition and op_AdditionUnchecked are defined:

  • If op_Addition was already effectively "unchecked", then users only get a behavior change if the library author changes the implementation of op_Addition
  • If op_Addition was already effectively "checked", then users get a behavior change if they recompile with /checked- (the default, so most users get a break) or already had unchecked(x + y)

For the case where op_Addition, op_AdditionChecked, and op_AdditionUnchecked are defined

  • op_Addition effectively becomes dead and only callable via reflection
  • Users get a behavior change for either op_AdditionUnchecked or op_AdditionChecked if it differs from how op_Addition was implemented

Based on the compiler defaults, I believe just having op_AdditionChecked introduces the least risk for breakage and the least confusing behavior.

  • Most types do not need to have checked vs unchecked operators and so will not need to define these or will implement them to be the same
  • For types that do need to have different checked vs unchecked operators the default /checked- and so if your operator was already checked, introducing op_AdditionUnchecked is breaking anyways and so provides no additional benefits over just having op_AdditionChecked.
  • For the case where users compiled with /checked+ by default, you end up with a behavior change only in an explicit unchecked(x + y) context or if you are now overflowing where you weren't previously. This behavioral change is likely desired.

@KalleOlaviNiemitalo
Copy link

Is this going to be supported with dynamic, too? There is CSharpBinderFlags.CheckedContext already, but if that is going to control whether the call goes to op_Addition or op_AdditionChecked, then it will need some work in Microsoft.CSharp.RuntimeBinder, and perhaps in System.Linq.Expressions as well.

@MadsTorgersen MadsTorgersen self-assigned this May 10, 2021
@333fred 333fred added this to the Working Set milestone May 20, 2021
@msedi
Copy link

msedi commented May 27, 2021

@tannergooding

The proposal here has regular and checked operators because the default compilation settings result in a global unchecked context. However, the proposal as is also doesn't change the meaning of existing code and so any case that exposes a regular operator today that contains checked behavior will not break unless the library/app author explicitly decides to change their code (even if the author adds an explicit checked operator).

But this means depending on the compiler switch the checked operator and the regular could become the same but with different implementations. Which one is used then? Or do I miss something?

@tannergooding
Copy link
Member Author

tannergooding commented May 27, 2021

But this means depending on the compiler switch the checked operator and the regular could become the same but with different implementations. Which one is used then? Or do I miss something?

That's under the "open questions". My suggestion is that if checked is defined to require an unchecked variant to also be defined and for to to explicitly be unchecked operator. It would still be emitted with the same name as the operator with no modifier (that is, it would be op_Addition not op_AdditionUnchecked).

This would hopefully help keep things simple "overall" while maintaining compat and helping with correctness.

@msedi
Copy link

msedi commented May 27, 2021

@tannergooding: That makes sense. Thanks for commenting on this.

@AndreyTretyak
Copy link

Would it make sense instead of having two separate operator methods (checked/unchecked) just add some option for the operator method to know if context is checked or unchecked? For example additional method parameter or some static property that might be useful outside of the operator method also?

@AlekseyTs AlekseyTs self-assigned this Jan 27, 2022
@333fred
Copy link
Member

333fred commented Feb 24, 2022

I've removed the specification from the issue text and have linked to the checked-in version.

@Zastai
Copy link

Zastai commented May 2, 2022

If adding metadata names for checked operators to ECMA-335, would it make sense to add matching assignment operators as well (e.g. op_CheckedAdditionAssignment)? These cannot be overloaded in C#, but neither can regular += & co and they do have a metadata name defined.

In fact, given that in the case of right shifts, ECMA-335 had op_RightShift, op_SignedRightShift and op_UnsignedRightShift already, perhaps it would also make sense to add op_UncheckedXxx metadata names for symmetry (even if C# elects not to use any of them).

@tannergooding
Copy link
Member Author

ECMA-335 itself doesn't define checked behavior for shift operators so it would make sense to allow user-defined variants.

As for Assignment variants; that would be independent of the language and would be a proposal to take to API review via the dotnet/runtime repo.

@Zastai
Copy link

Zastai commented May 2, 2022

ECMA-335 itself doesn't define checked behavior for shift operators so it would make sense to allow user-defined variants.

I only gave the shifts as an example of ECMA-335 defining metadata names for variants (signed and unsigned) despite C#/VB/... only using the regular one (until recently).

I'm merely suggesting that if the concept of checked arithmetic operators is added, it might make sense to do so in a similarly comprehensive way. So you'd have op_Subtraction, op_CheckedSubtraction and op_UncheckedSubtraction, even though C# (currently) elects to only use the plain and checked variants.

@tannergooding
Copy link
Member Author

It is explicit that there is no "unchecked" variant.

The design is such that if only op_Subtraction is defined, it is context oblivious. It has whatever behavior it has and is called from both checked and unchecked contexts.

Once op_CheckedSubtraction is defined, the intent is that it is checked and op_Subtraction is unchecked and that you are required to define both.

@AlekseyTs
Copy link
Contributor

Implemented

@AlekseyTs AlekseyTs reopened this Aug 31, 2022
@AlekseyTs AlekseyTs added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Aug 31, 2022
@ViIvanov
Copy link
Contributor

Implemented

Does it make sense to mark somehow (i.e. keep updated) this status in starting message?
Currently it marked as "Implementation: Not Started".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests