-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Champion "Method Contracts" #105
Comments
Ok then I’ll bite. First up, the common refrain is don’t worry Non-Nullable is coming, that’ll sort out 90% of your problems. With fresh eyes I'd like frame method contracts another way:
I’m going to avoid talking syntax and implementation immediately because I think the initial conversation should be scope and vision. Previous threads seem to derail with some arguing over base concepts while others charged ahead with wildly differing syntax. However, I will lay out the key traits I’d like to see.
On that last point, obviously everyone has different ideas of the key pieces but for me they are:
I really liked CC and would love to bring forward our old code, but perfection is the enemy of good and I just don’t see CC coming over to .Net Core. Everyone can start arguing about invariant again now :) |
Here is a variation of the syntax that has been missed in the original proposal (taken from #511): public void Foo(object o)
requires o is Bar b else throw new ArgumentException(nameof(o)))
{
// do something with `b`...
} |
If we're going to do this I'd absolutely love to be able to omit the public void Foo(object o) requires o is Bar b
{
// do something with `b`...
} is the same as this: public void Foo(object o)
requires o is Bar b else throw new ArgumentException(nameof(o)))
{
// do something with `b`...
} |
@Richiban if |
@alrz It probably makes more sense for you to do something like this: public void Foo(int age)
requires (age > 16)
else throw new ArgumentOutOfRangeException("Thou shalt not pass!", nameof(age))
{
// ...
} However, sometimes you need an object as a parameter, like when defining an event handler: public void OnLeftButtonClick(object sender, MouseButtonEventArgs args)
requires (sender is Control control)
else throw new ArgumentException("Wasn't a control.", nameof(sender))
{
// ...
} |
I'm not sure the value proposition is there if contracts boil down to moving existing guard code outside the method body. I'd like to see something that is more strict, less magic and leverages existing c# terms and constructs. Namely:
So taking the above example:
We can then translate this into a predicate based on a helper method (and potentially any predicate... although I'm trying to keep it simple here) and then use some sort of compiler directive to say when guard code is generated (i.e. public methods only, debug only). By default the above code be translated into something like:
For return (ensures) values we can reuse the keyword return (which obviously wouldn't previously be valid here). The use of return means that we can avoid bringing in concepts such as requires and ensures.
To somebody coming to the language fresh I think the idea that you can use the same syntax for generic constraints as well as method constraints makes a lot of sense. I've not talked about checks that apply outside the scope of the method here because: I'm not sure they are as valuable as we move towards side effect free code and I think they might be better served with a separate construct. |
@Lexcess I like the syntax, but how does the compiler know to throw an public static Exception Range(int value, int inclusiveMin, int inclusiveMax); Then the compiler would output the following for the example. public void Foo(int age)
{
// generated:
var exception = MethodContracts.Range(age, 0, 16);
if (exception != null)
throw exception;
// implementation...
} I use a variable name of |
@TylerBrinkley That is a pretty neat solution. I was originally thinking you'd want to separate the predicate from the exception but if we assume that contracts are either applied by exception or non-existent (except perhaps by some documentation attributes) then it makes a lot of sense. I'll think about it a bit more but it does significantly reduce what the compiler needs to know/do. Also one thing from @MovGP0 I didn't address. On "is" checks. Ideally you'd simply change the method signature to the more specific type. Sure sometimes you don't own the signature, but I wonder if it is valuable enough to do the work to support a sub-optimal approach (you can still just do it in the method itself). One thing I'd love to achieve here is that the syntax can be applied to interfaces and injected into implementations. If I saw even one person define an interface then have the contract define a subclass I'd feel like they were led down the wrong path. Interface support is the other reason I'd like to keep the contract options super simple, at least at first. |
I disagree with the public (int result, int rest) Divide(int numerator, int denominator)
require numerator > 0
require denominator > 0
ensure result > 0
ensure rest >= 0
{
// implementation...
} For range checking, you could in principle do something like require Enumerable.Range(16,120).Contains(age)
else throw new ArgumentOutOfRangeException("Must be between 16 and 119 years old.", age, nameof(age)) But it would be way less performant to enumerate and compare a few dozen numbers than to test just two inequalities: require age >= 16
else throw new ArgumentOutOfRangeException("Must be at least 16 years.", age, nameof(age))
require age < 120
else throw new ArgumentOutOfRangeException("We do not accept mummies at this partey!", age, nameof(age)) |
if you want a range operator, consider an extension method: public static bool IsInRange<T>(this T value, T start, T end)
where T: IComparable<T>
{
return value.CompareTo(start) >= 0 && value.CompareTo(end) <= 0;
} and then you can use require age.IsInRange(16, 119) else throw NewArgumentOutOfRangeException("...", age, nameof(age)); |
@MovGP0 interesting, would you not at least agree that a where constraint is a similar concept? I think it would aid discoverability, especially if you are new to the language. The point on the extension methods isn't so much to shorten the line, it is a model for implementation. If you allow any code in the clauses you hit a couple of problems:
For you tuple example I'd write similar syntax to parameters with:
All that said, for me the key for Method Contracts/Constraints to have value is to be declarative. Otherwise you are just moving a block of code above the brace instead of below it. |
@Lexcess contracts may not be attached to the functions/methods/procedures directly, but may be „clued“ on. Consider the following code (taken from here): using System;
using System.Diagnostics.Contracts;
// implementation
[ContractClass(typeof(IArrayContract))]
public interface IArray
{
Object this[int index]
{
get;
set;
}
int Count
{
get;
}
int Add(Object value);
void Clear();
void Insert(int index, Object value);
void RemoveAt(int index);
}
// contract
[ContractClassFor(typeof(IArray))]
internal abstract class IArrayContract : IArray
{
int IArray.Add(Object value)
ensures return >= -1
ensures return < ((IArray)this).Count
=> default(int);
object IArray.this[int index]
{
get
requires index >= 0
requires index < ((IArray)this).Count
=> default(int);
set
requires index >= 0
requires index < ((IArray)this).Count
{
}
}
} |
@Lexcess because of the declaritive approach: I am unsure how something like public (string foo, int bar) FooBar()
where return foo : NotWhiteSpace, MaxLength(10), HasUnicodeBom16
where return bar : Max(100)
{
// implementation...
} should compile. Especially the |
@MovGP0 Yes I use those a lot in the old contracts, but if we are modifying the compiler I'm thinking lets try and get a cleaner implementation here. That model was purely down to the restrictions of using post compile weaving. On whether these are higher order functions, attributes or something else. I think this is something where we need to drive out what works best. My gut instinct is that higher order functions makes sense (perhaps fulfilling the signature as @TylerBrinkley described with a selection of typed parameters). However I could imagine cases where essentially they are special cased much like with the generic new() constraint. I'm very aware that the CSharp team hasn't seemed very enthusiastic about contracts in the past, so I do think some weighting will have to be given to what is easiest to implement and test. This is also another reason I like the reuse of where, I'm hopeful some of the compiler code can be lifted and repurposed to achieve our goals rather than start from scratch. |
@Lexcess my rationale is that functor attributes and higher order functions are harder to implement and harder to do interference with. I prefer the simpler solution in such cases. |
Even if higher order functions or attributes are harder to implement why not to use |
Today where is restrictive, not allowing the code to compile in case the condition isn't meet. The reuse of the where keyword would force the caller to check the parameters? Or would it be a runtime exception, giving a double meaning to the word? In any case, it would be nice to have static checks at compile time either as warnings or errors, not only exceptions or fail fast. |
@Flash3001 I agree that static checks would be useful. I think trying to do the same amount of work in the compiler as the old static analyzer did is unrealistic but whatever can be done towards that would be beneficial. Would you consider the fact that where is implemented as compile time only part of the 'contract' with the developer or is that just an implementation detail. I'd err more towards the latter. For example you could have an implementation of generic constraints that was extended to work with dynamic and I'd expect it to be a runtime check. |
@Serg046 "Why do we need to throw an exception manually each time of usage?" We don't. Contracts should be enforced at compile time. An error in the build log with the location of the calling code would be enough in such cases. However, sometimes you might want to provide a better error message, so you should be able to override the default by providing a custom exception. |
@Lexcess I find the |
@Lexcess, To be honest, I'm not sure! I like the idea of where begin reused, as no new keywords need to be created and it has this logical sense of restricting input parameters - if generics is considered an input parameter. My only issue with it would be the possible change of perceived meaning. On the other hand what was done to is and switch to support pattern matching was an ingenious idea. The same idea could be applied to where so it would be extended in the same way, allowing compile and runtime checks of either generic, input or return. I'm not advocating for any of the following samples, just an idea:
|
there is another reason. in cases where the public static class Incrementor
{
public static int X { get; } = 0;
public static void IncrementBy(int y)
requires y >= 0; /* validate on start */
ensures old(X) < X; /* does something at start; validates at return */
ensures X >= 0; /* validate on return */
{
X += y;
}
} public void Foo(ref int bar)
where bar > 0; // do you validate on start or on return?
{
} |
Glad I added the caveat about being wrong then! Perf was definitely an issue which is why I would always advocate for a more refined scope of contract options (dropping things like invariants, perhaps only offering pre-conditions to start with etc...). The reason I said that though, was that your contributions seemed to dismiss what people did find useful about contracts, rather than contribute on how to rework the concept into something that would meet the bar as a language feature. I think most here are realistic about why Method Contracts as it was would never make it. At a high level I like the idea of describing complex constraints inline in code. Not scattered off in separate types. Not implied by various test suites. Directly in my code. Without contracts we tend to use repetitive inline guard clauses (if you are lucky) and tests, plus we obviously do not have the same guard metadata model for use in API definitions and enforcement. Obviously, the original implementation had some other cool features (inference for example), but personally I do not consider much from the original implementation to be sacred in the process of getting the concept adopted as a language feature. So, if I could steer you, it would be towards what would a performant and robust language feature for people who like the concept of contracts look like? As opposed to simply arguing if they should exist at all or be constrained by the flaws and overambition of previous implementations. |
Trying to persuade the team some feature is good or important to you is useless. They have said above that they would like to let you wait for 15 years. One example is the recent "file private" visibility, which is a feature similar to what had been proposed by community years ago, but is only discussed seriously recently when the c# runtime team found it useful. |
I def can appreciate what people find useful. But at the same time, making a language change means we have to have practical solutions here. :-) |
Right. I don't get what's contentious about that. If the feature takes 15 years until it's viable then we shouldn't be shipping earlier. |
This feature has been discussed seriously.
This is not the case. The discussion was advanced because a reasonable proposal was put forth that actually handled a whole host of concerns that are needed. We have limited resources, so having a realistic proposal or forth that goes in depth and works to balance many concerns, addresses costs, and thinks about how it can be expanded in the future, ends up being something that here attention enough from an ldm member to champion. |
In context, I was speaking about a more restrictive check that would be triggered by an application constant. That is, the data that flowed in would never be fine, so the build would be broken regardless of whether it was due to a compile-time contract or the type's runtime check on application load.
... I mean, I'd like checked exceptions too, if I could get them (But that would require quite a bit wider changes to the exception ecosystem, a la Midori). |
I genuinely don't know. I've seen many attempts at this that have not been viable. So my default position here is that efforts would likely be better allocated elsewhere. I do think if someone in the community, or other venues (like academia) can prove this out, then that would certainly make things much more likely. I personally have no clue how to pull this off. I wish I did! :-) |
I think the challenge is more at the edges. Say one API says it generates values from the set S, and one API says it takes values from the same set. But then the second API changes to {S-1}. Now you have a contract break. However, it's entirely possible that in reality that single value that is not accepted is not actually ever generated by the first API (it's just marked that way as a reasonable, slightly broader, contract). This would now break with a contract incongruity, despite very possibly not being an actual problem for these libs. That's why I don't want to actually encode this as an abi. I'd far prefer it as just a relaxed analysis system that layers on top. This is also much more palatable as such analysis is not encumbered by the same compat rules we have. A different owner can decide a different set of rules and compat decisions that make sense to them. |
I'd love to see a design there that can work in practice. For example how would a linq system work with checked exceptions? How do you support passing delegates to things when exceptions are part of your signatures? |
I don't think I really disagree with this concern.
From the look of things, Midori would have done something like public static class Enumerable
{
public static TResult Select<TSource, TResult, throws E>(this IEnumerable<TSource> source, Func<TSource, TResult, E> selector) E
{
foreach (TSource element in source)
{
yield return try selector(element);
}
}
}
// I don't think you needed to do anything at the call site if you couldn't throw a non-abandonment exception? Blog slightly unclear here.
// Something like normal.
var n = new [] { 1, 2, 3 }.Select(n => n + 1);
var n = try new [] { 1, 2, 3 }.Select(n => throw new IOException()); ... although probably |
I think you should check out this: Or for a more complete list: |
Yeah I was already told that time is far more disgusting than I thought. Not part of the discussion. Part of the discussion now:
Internal attempts in the C# team? Or general attempts like other languages (Dafny is one that comes to my mind).
What kind of performance are we talking about? Compilation or runtime? Because compile-time contracts are what I have been dreaming of, eliminating runtime checks, further improving performance in the process, in alignment with runtime support and JIT optimizations. |
I am not sure if I have seen this proposal, nor the remaining concerns that meant it didn't pass muster. Could you link or copy both here? |
Actually I don't think the proposal I was talking about ("file private" visibility, as I don't want to create a reference I would not provide a link here) can be considered "a reasonable proposal put forth" at the time most discussion happened. Most of the content was added later. There are many examples in this repo that someone from the community wrote a much longer proposal and got no more than 2 replies. It is not a very good experience to spend time writing proposals here. As I said, it is very hard to persuade the team to do or not to do something. Such work is better left to themselves. |
Apologies, I misunderstood. From the reply I thought there was such a proposal for contracts. |
I created a proposal for a compile-time Code Contracts based on attributes that can be analyzed as part of the development experience. https://github.com/marinasundstrom/CodeContractsTest The analyzer is not there. But I give hints on what kind of errors there could be. There is also a syntax proposal. I know that someone has already been experimenting with implementing it into the compiler. |
I personally like it. Just thinking about having IntelliSense and compile-time checking of the contract specs makes me all giddy. For example: public int CalculateLevel(int userLevel)
requires userLevel >= 0,
ensures result >= 100
{
int systemLevel = 100;
return systemLevel + userLevel;
} This would compile into a C# backward-compatible spec with attributes: [Ensures("result >= 100")]
public int CalculateLevel([Requires(">=0")]int userLevel)
{
int systemLevel = 100;
return systemLevel + userLevel;
} As such, an analyzer could easily traverse the AST, extract the attributes and run the SMT solver. |
@Genbox Yes. This is the first step. Building the infrastructure. There are a lot of advanced patterns to be dealt with (like An analyzer plugin can be written quite easily to test the concept. I did create a project for attempting that in my repo. When it comes to the language syntax, I remember someone a long time ago (I think in this issue) posting a concept of implementing it in Roslyn. From my repo: // CS1234: Argument does not satisfy requirement: arg < 42
// CS1235: InvalidOperationException is unchecked.
var result = DoSomething(42);
~~
// "result" is marked as satisfying > 0
// CS1234: Argument does not satisfy requirement: input < 0
Foo(result);
~~~~~~
[Throws(typeof(InvalidOperationException))]
[return: Ensures("return > 0")]
int DoSomething([Requires("arg >= 0"), Requires("arg < 42")] int arg)
{
if(arg == 10)
{
throw new InvalidOperationException();
}
return arg;
}
void Foo([Requires("input < 0")] int input)
{
} Another example: File file = new ();
// "name" is satisying Length =< 50;
var name = file.FileName;
// This means that you get an error or warning
// CS1234: Argument does not satisfy requirement: fileName.Length >= 60
ReadFile(name);
~~~~
void ReadFile([Requires("fileName.Length >= 60")] string fileName)
{
}
class File
{
public string FileName
{
[return: Ensures("return.Length =< 50")]
get;
} = null!;
} |
@marinasundstrom For example, a "full contract" solution means that the code is seen as a proof that has to be QED before it compiles. For example: public int Add(int val1, int val2)
ensures result <= int.MaxValue
{
//This line would result in an exception when checked mode is turned on
return val1 + val2; // <- this fails the contract. int.MaxValue + int.MaxValue is larger than int.MaxValue
} public int Add(int val1, int val2)
ensures result <= int.MaxValue
{
Math.Clamp(val1, int.MinValue, (int.MaxValue -1) / 2); // <- can also be a require contract
Math.Clamp(val2, int.MinValue, (int.MaxValue -1) / 2); // <- can also be a require contract
return val1 + val2; // <- This now passes
} This kind of contract is amazing for correctness, but difficult to implement, which is why Code Contracts had Contract.Require() on the incoming data, then normal data validation checks, and finally Contract.Assume() to satisfy the SMT constraints. However, if we focus on pure API contracts and the second-order/derived values, only 2 solutions seemed to be viable. All examples below are a "require contract". Imagine [Requires("example here")]. Solution A: Explicit naming. That is the solution you have in your example. Example: "filename.Length <= 40"
Solution B: Macros. Imagine you have a macro to calculate the length of types. Example: "length(filename) <= 40"
|
The patterns or exact conditions representable aren't really the problem. The biggest problem is the ecosystem - defining the compatibility of the ABI, dealing with NuGet packages, loading libraries with potentially conflicting/no info.... |
@Clockwork-Muse Yes, that is why I propose the attributes approach just like with Nullable reference types. Letting you opt-in or out. |
@Genbox Yea. I think there is a limitation to what we, realistically, can check and what expressions to allow. We can mainly assure that locals and fields of primitive values are set to a certain value. Not evaluate that methods yield specific results. There is a special case to this approach that I proposed: The string literal “Foo” will yield a string instance that has a Length (3) annotated to it. It will help us make assumptions about that property. After a local variable has received an instance of a string with a certain Length, I argue it should Ensure that any possible future assignments adhere to the same specification. The variable now has an Ensure attached to itself. |
That is not an issue, it is a feature. No really. If a developer says "i need this input to be [1..10", but you have always given it 11 and it worked just fine, then you are relying on the developers good graces where he probably did something like When he updates the code with a contract that says
Conditions just needs to be satisfied, so if one contract says Well, not much. Your application won't compile, but the author declared his library won't work with a value of 10 anyway, so don't give it 10 or have them update their contracts. Today we mostly hope that it "just works", but if a library author explicitly declare that he does not support a value of 10, I'd much rather want to know it a compile-time than suddenly at run-time. But lets say 10 is perfectly valid and the library author won't update his contract, then it shouldn't be that difficult to add an exception to the SMT solver so that particular check is not included. For example: //Your code
static void Main()
{
// Example 1 - Ignore contract for specific value
Calculate(11!); // <- usually fails, but ! denotes that it is ignored (like with nullability)
// Example 2 - Pragma warning disabled. Might disable multiple contracts on this line
#pragma warning disable XXX
Calculate(11); // <- usually fails, but pragma compiler directive ignores it
// Example 3 - Explicit exclusion for code contracts
#DisableContract(val)
Calculate(11); // <- usually fails, but not for 'val' has it has been disabled
}
//Library code
public int Calculate(int val)
requires val >= 0 and <= 10
{ } @marinasundstrom have you thought about how to exclude one single requirement vs. disabling the analyzer for a whole line? |
@Clockwork-Muse “ABI” will always break when you recompile. That is what happens all the time. But at least it will not cause too much havoc, since this does not change the fundamentals of the CLR or the metadata. It is an opt-in compiler feature. @Genbox No, I have not. That is worth thinking about. We probably need to recognize parts that are not annotated too. Warn about passing a value from a source without a specification. And a way to tell the compiler that its OK. |
The fact that they're attributes don't make them optional.
This assumes that everything is getting recompiled, and for that, I agree with you.
Assume that the conditions are from libraries that you pull in as dependencies, so you can't recompile the code. That's the actual issue here.
It doesn't actually. If this were the case, you couldn't update transitive dependencies until any other libraries using it were also update. Major library teams spend a lot of effort ensuring that they can publish patched binaries that are usable without recompiling other things that depend on them. It would mean that Windows Update would break all your applications. Part of the "hard stuff" for this feature is even figuring out whether adding contracts at all would be a breaking change or not. |
"I'm very curious about those edge cases."
There's an interesting page
<https://nodatime.org/2.2.x/userguide/trivia> on the NodaTime website
about stuff like this.
…On 3/19/2022 4:48 PM, AlFas wrote:
There have been months that have had 32 days.
Minutes can be 61 seconds long.
I'm very curious about those edge cases.
your programming language may not allow you to represent things
that are necessary.
Either way, there can be similar constraints; I'm not the one to
design the APIs, i.e. on dates. Whoever wrote the date APIs will take
this feature seriously into consideration before applying it, let
alone the how.
—
Reply to this email directly, view it on GitHub
<#105 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5RDQLK6YNK5RNNTG45CPTVAY4QHANCNFSM4DAESNBA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this
thread.Message ID: ***@***.***>
|
Just hoping to summarize this and make sure I got everything down. I believe we're in agreement that:
There is still some debate on syntax. Major candidates appear to be:
Questions exist on whether it should be the responsibility of the developer to specify the exception, typical examples looking like Open questions - How much do we want to link this into static analysis. For instance, would this open the door to allowing // T must be a class and must either be of type Foo or type Bar.
public bool IsValid<T>(T t)
where T : class
requires T is Foo? foo or Bar? bar
{
if (foo is not null)
{
// T is Boo
}
if (bar is not null)
{
// T is Bar
}
t.GetHashCode(); // Treat T as though it is `object`.
} I'm sure the syntax can be cleaned up a bit here but I think it works well enough to show what I'm thinking. |
If I would have re-imagined C# as a language with method contracts. The use of keywords. And other changes. public open class Calculator<T>
extends Object
implements Disposable, AnotherInterface
where T is not nullable
where T implements Number
{
public void Sqr(T value)
requires value > 0
ensure return > 0
throws InvalidOperationException
{
return 0; // This will not be allowed as per "ensures".
}
public void Dispose()
{
}
} Usage: let calculator = new Calculator<int>();
let r = calculator.Sqrt(2); // Warning: InvalidOperationException not handled
let r = calculator.Sqrt(0); // Warning: Not allowed as per "requires" void Foo()
throws InvalidOperationException
{
let calculator = new Calculator<int>();
let r = calculator.Sqrt(2); // No warning: InvalidOperationException to be handled by caller
} Of course, I would rather use |
See also dotnet/roslyn#119
The text was updated successfully, but these errors were encountered: