dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.96k stars 4.02k forks source link

Proposal: Method Contracts #119

Closed stephentoub closed 7 years ago

stephentoub commented 9 years ago

(Note: this proposal was briefly discussed in #98, the C# design notes for Jan 21, 2015. It has not been updated based on the discussion that's already occurred on that thread.)

Background

"Code Contracts" were introduced in .NET 4 as a feature of the Framework, primarily exposed through the System.Diagnostics.Contracts.Contract class. Through method calls like Contract.Requires and Contract.Ensures, developers could add code to their methods to specify preconditions and postconditions, and then a subsequent tool known as a rewriter could be used to post-process the code generated by the compiler to appropriately implement the expressed contracts, e.g. whereas the developer puts a call to Ensures at the beginning of the method, the rewriter ensures that path through the method that could cause it to exit ends with a validation of the expressed postcondition.

public int Insert(T item, int index)
{
    Contract.Requires(index >= 0 && index <= Count);
    Contract.Ensures(Contract.Result<int>() >= 0 && Contract.Result<int>() < Count);
    return InsertCore(item, index);
}

Problem

Very little code actually uses these API-based code contracts. They require additional tooling to be integrated into the build, they're verbose, and it's difficult to write additional robust tooling related to them (e.g. integrating contract information into documentation).

Solution: Language support for method contracts

Basic contract support for preconditions and postconditions can be built directly into the language, without requiring any separate tooling or custom build processes, and more thoroughly integrated throughout the language.

The compiler would generate all of the relevant code to correctly validate the contracts, and the contracts would show up as part of the method signature in the Visual Studio IDE, such as in IntelliSense at call sites to the method.

public int Insert(T item, int index)
    requires index >= 0 && index <= Count
    ensures return >= 0 && return < Count
{
    return InsertCore(item, index);
}

These contracts would primarily be validated at run time, however if the compiler (or a 3rd-party diagnostic) could statically detect a violated contract (e.g. a precondition requires that an argument be non-null, and null is being passed as the argument), it could choose to fail the compilation rather than allowing the bug to make its way to run time. Similarly, if the compiler (frontend or backend) is able to statically determine that a contract will always be satisfied, it can optimize based on that knowledge.

Unlike .NET Code Contracts, which have configurable but complicated behavior provided by a post-compilation rewriter tool (and which are no-ops if such a rewriter isn’t used), failed validation of ‘requires’ and ‘ensures’ clauses would ideally result in "fail fast" (ala Environment.FailFast(...)), meaning the program abandons immediately. This is very useful for validating preconditions and postconditions, which are typically used to catch usage errors by the developer. Such errors should never make it into production.

The compiler would require that preconditions and postconditions can be validated by the caller, and as such it requires that any members used in the requires and ensures clauses are all accessible to any caller, e.g. the requires clause of a public method may not access internal or private members, and the requires clause of an internal method may not access private members.

Preconditions and postconditions should also not throw exceptions.

Virtual methods and interface methods may have preconditions and postconditions, in which case the compiler guarantees that overrides and implementations of these methods satisfy the preconditions. To make this clear to a developer reading the code, the override or interface implementation would state "requires base" or "ensures base", to indicate that there are imposed constraints, while not forcing the developer writing the code to explicitly type them out.

interface IItemCollection
{
    int Count ensures return >= 0 { get; }
    …
}

class MyItemCollection : IItemCollection
{
    public int Count ensures base => m_count;
    …
}

Alternatives: Fail fast vs exceptions

In support of migrating existing code to use contracts, or for places where an exception is really desired over fail fast, we could optionally consider additional syntax to specify that an exception should be thrown instead of fail fasting.

For example, the previous Insert example's requires and ensures clauses would result in fail fasting if a condition isn't met:

public int Insert(T item, int index)
    requires index >= 0 && index <= Count
    ensures return >= 0 && return < Count
{
    return InsertCore(item, index);
}

But, the developer could explicitly override the fail fast by specifying what should happen in case of failure:

public int Insert(T item, int index)
    requires index >= 0 && index <= Count else throw new ArgumentOutOfRangeException(nameof(index))
    ensures return >= 0 && return < Count
{
    return InsertCore(item, index);
}
aarondandy commented 9 years ago

Any possibility of getting invariants as well?

Would love to see the static analysis tools we have in Code Contracts working with these new contracts.

I really like the presented syntax to decide between exceptions and fail-fast for preconditions.

aarondandy commented 9 years ago

Could some kind of flag be provided to omit private/internal contracts from release builds if we have static analysis tools to prove them?

jaredpar commented 9 years ago

@aarondandy

In previous research we have shied way from invariants in part because it's actually incredibly difficult for developers to predict the effect it will have on the code. Typically invariants checks are validated to be true on every exit / entry point of a given method. Given that can you point out all of the places on the following method where that happens?

class C
{
  invarint Count >= 0

  int[] _array;
  int _count;

  void AddRange(IEnumerable<int> all) 
  {
    EnsureCapacity();
    foreach (var cur in all) {
      _array[i] = cur;
    }
  }
}

Here are just a few of the places where an invariant check must occur in the above code:

Sure a few of these may be able to be optimized away. But even then on this really simple method the invariant check is occurring at a very high frequency.

In the past when we looked deeply at places where developers wanted invariants we found that requires and ensures were sufficient to the task.

aarondandy commented 9 years ago

@jaredpar

That is fair considering the complexity The real reason I want invariants is just to reduce the amount of redundant contracts I would need to write. As a compromise would it be terrible to have a way to reuse some pre/post conditions? Something like [ContractAbbreviator] but probably way different?

mirhagk commented 9 years ago

@aarondandy Are you referring to reusing a pre-condition as a post-condition, or referring to reusing a precondition across several methods in a class?

For the first situation it'd be nice if requires ensures or some other syntactic sugar was supported. For the second, it could be handled with allowing the expression to be a method (which can be declared with the shorthand lambda method syntax), so you can have a CheckClassInvariants method and do requires ensures CheckClassInvariants() which isn't too bad.

aarondandy commented 9 years ago

@mirhagk

"reusing a precondition across several methods" is what I meant but I like your other thoughts too. I don't really want to have a true provable invariant for Count >=0 but I definitely want a way to prove and communicate that each method call on my type won't do something crazy to Count.

ejball commented 9 years ago

I wonder if ensures could be made to work naturally with async method results.

alexpolozov commented 9 years ago

Good to see that this is being considered!

Preconditions and postconditions should also not throw exceptions.

How are you going to verify this? For instance, are we allowed to do property access in contracts: requires id != this.Owner.Id? What if Owner is null? Is it an uncompilable code, a NullReferenceException at runtime, or a contract failure?

Just like in API-based Code Contracts, there should be some way to refer to the old (pre-invocation) value of the field/property/parameter. old(foo) looks good, but that makes old a new contextual keyword, which you don't like to introduce often.

mirhagk commented 9 years ago

@apskim

Preconditions and postconditions should also not throw exceptions.

This may or may not be enforced. It might just be a "you shouldn't do that" kind of thing rather than a compile time exception (perhaps static analyzers could look for it).

If it is a compile time thing, then regular property access would have to be disallowed in your case to instead do something like requires id <= this.Owner.?Id. This doesn't throw any exceptions and will fail the validation.

drewnoakes commented 9 years ago

To what extent can the compiler check a type's contracts for internal consistency? For example, it'd be hard to reliably solve for arbitrary expressions:

class C
{
    double Value ensures Math.log10(return) > 0 { get; }

    void Foo() ensures Value < 1 { ... }
}

There is no value of x < 1 for which Math.log10(x) > 0.

Would these constraints primarily be evaluated at run-time, with best-effort use from tooling?


Also, if arbitrary code can be executed in requires and ensures statements, side effects must be considered. Code may behave differently when compiled to evaluate contracts vs. without.

mirhagk commented 9 years ago

@drewnoakes I'm assuming that an extension will be created that checks all sorts of crazy situations that you'd never want to add to a compiler (for complexity reasons). The compiler can do very minimum checks, and leave most of the checks to extensions.

In #98 it was mentioned that there may want to be some shorthand syntax for common checks (null being the most common). Something like requires item seems like a simple shorthand for the case of null checks, and combined with the ability to separate with a , would probably lead to a wider usage (requires item, data, index would be very simple to write and there'd be little reason to not use it).

An alternative is to use requires item! which makes use of the proposed non-nullable reference syntax ! and separates it from a boolean.

Clockwork-Muse commented 9 years ago

what about situations where a compile-time check is nice, but a runtime check may impart more of a performance penalty? For example, there's this comment in System.Collections.Immutable's array:

public T this[int index]
{
    get
    {
        // We intentionally do not check this.array != null, and throw NullReferenceException
        // if this is called while uninitialized.
        // The reason for this is perf.
        // Length and the indexer must be absolutely trivially implemented for the JIT optimization
        // of removing array bounds checking to work.
        return this.array[index];
    }
}

...which states that, while you want compile-time checks for existence of the array, and index range, the penalties for runtime checks are too high. (In this case they're letting the hardware/vm throw the errors directly, a rarer occurrence than checking all accesses)

Maybe the validator could biggyback off of hardware/vm protections? So it could see something like requires this.array, index >= 0 && index < this.array.Length, note that it has built-in runtime checks (effectively) for those values anyways, and noop them. Which would probably require sitting in the compiler, than as an external tool.

jaredpar commented 9 years ago

@Clockwork-Muse

We've did pretty extensive profiling of contracts in our project and found that in general the performance hit they caused was minimal. It was usually in the 1-2% range across the system.

Sure there was the occasional check that was too expensive to make a contract. In those situations we first looked to the API itself. If verifying the invariants of the type was too expensive to put in a contract then perhaps the API itself needs to be changed.

If the API couldn't be made more efficient then we would move the check into Debug.Assert. That was pretty rare though (only a handful of occurrences).

Note: contracts do not always result in a negative performance hit. It is possible that they establish invariants (like bounds) that the JIT can optimize against and result in faster code. There were definitely places where that happened.

AdamSpeight2008 commented 9 years ago

Would it be possible to have a method contract in this position?

T Item(int index)
{
  requires (0 <= index) && (index < Size)
  get{ return _a[index]; }
  set(T item) { _a[index]=item; }
}

Using a trait ( #129)

T Item(int index)
{
  trait { _a!; range:  0 <= index < _a,Length; }
  get { _a[index]; }
  set(T item) { _a[index] = item; }
}
brandf commented 9 years ago

The part about Virtual methods and interface methods made me starting thinking about how this might be seen as an extension to the type system in a way...or rather another kind of type, similar to units of measure in F#. I'm bored, so I'm going to riff on the idea...

If we start with the first example in the solution above:

public int Insert(T item, int index)
    requires index >= 0 && index <= Count
    ensures return >= 0 && return < Count
{
    return InsertCore(item, index);
}

The 'requires' are additional constraints on the methods parameters, similar to how you have a types associated with each parameter. For fun I'm using 'where' below instead of adding new keywords. I think it's worth considering making it look LINQ'ish:

public int Insert(T item, int index where index >= 0 && index <= Count)
    where return >= 0 && return < Count
{
    return InsertCore(item, index);
}

Which then makes me think of the notion of generic type constraints. Similar to how a delegate gives a name to the shape of a function, a typeconstraint is used to give a name to a constraint.

typeconstraint Bounded<Type, Lower, Upper> = Type N where N >=0 Lower && N <= Upper

Perhaps you could then use typeconstraints anywhere you would normally use a type. The generic parameters could be bound to other types, constants, members or other arguments. Now the above can be rewritten:

public Bounded<int, 0, Count> Insert(T item, Bounded<int, 0, Count> index) 
{
    return InsertCore(item, index);
}

...which is looking pretty clean IMO.

And maybe I'm going off the deep end here, but lets consider going a step further by thinking of the traditional type syntax as being syntactic sugar for a specific where clause...

typeconstraint Bounded<T, Lower, Upper> n where typeof(n) is typeof(T) && n >= Lower && n <= Upper

In other words, 'traditional type syntax'...

int x

...is shorthand for...

var x where typeof(x) == typeof(int)

...or for parameters and returns...

x where typeof(x) == typeof(int)

Since non-nullable types are being considered at C#7 design meetings, lets see how that fits in with this:

typeconstraint NotNull n where n != null

...and union types (a bit like TypeScript has)

typeconstraint Union<T1, T2> n where typeof(n) == typeof(T1) || typeof(n) == typeof(T2)

And of course, similar to delegates, an assembly should be able to expose public typeconstraints like any other kind of type.

chrisaut commented 9 years ago

I mentioned it in the original discussion, but what about a short hand syntax for the most common scenarios:

public void M(string! x) {}

expands to:

public void M(string x) requies x != null {}
public void M(int >= 0 x) {}

expands to:

public void M(int x) requires x >= 0 {}
public int >= 0 Count() {}

expands to:

public int Count() ensures return >= 0 {}
public void M(int < y x, int > x y) {}

expands to:

public void M(int x, int y) requires x < y, requires y > x {}

(this specific case looks messy, thus probably ok to only support short hand for simple cases, like operators and constant literals (no method calls, no refering to other arguments or fields/properties etc, for that you would use the "full" syntax).

public C! M(B! b) {}

expands to:

public C M(B b) requires m != null, ensures return != null

I think of contracts as a harder "type", thus I feel it might make sense to have the requirements right there with the parameter (it also avoids double typing). For instance you don't say public M(x) requires x is int, the int requirement is right there with the parameter, so why would further requirements on x not be right along there with it?

The downside is it might look a bit messy, especially if there are lots of parameters (but that's code smell anyways), and I guess it's (much?) harder to parse for the compiler.

AdamSpeight2008 commented 9 years ago

@chrisaut public void M(int < y x, int > x y) {} can never be satisfied

I think the ( #129 ) would be a better approach.

public void M(int x, int y)
  trait { x < y; y > x }
  {}

As it doesn't mess with the parameters syntax, yet still available for the IDE to use the user and collapse the trait definition.

chrisaut commented 9 years ago
@chrisaut public void M(int < y x, int > x y) {} can never be satisfied

why not? it expands to:

public void M(int x, int y) requires x < y, requires y > x {}

x needs to be smaller than y, and y needs to be bigger than x (which comes from the first constraint, you wouldn't even need the second constraint). M(0, 1) satisfies it. But it just goes to show that for anything but trivial "< constant" stuff the syntax is confusing, hence it's probably not a good idea to allow it for anything that referes to other named parameters/fields/etc.

AdamSpeight2008 commented 9 years ago

May have misread it as x < y and y < x

sirinath commented 9 years ago

I don't see why this should be method only. Why not be able to use with variables also. In addition some distinction on runtime, compile time if unable at compile time at runtime and enforceability.

AdamSpeight2008 commented 9 years ago

Codeplex Topic

vkhorikov commented 9 years ago

@jaredpar consider binding this feature to non-nullable reference types. Although they can't be implemented as they are implemented in, for example, F# but some kind of 'global precondition' can mitigate the problem: it can state that no reference type within a class can have null value. This will help to reduce amount of '!= null' preconditions in code that tries to be more functional.

johncrim commented 9 years ago

Could you explain why run-time contract violation shouldn't throw an exception? I've shied away from using CodeContracts static analysis (build are too slow; too much work to fix all warnings), but find them great for run-time validation and documentation. It's also great to be able to dial up and down the amount of run-time checks for debug and release builds (full checks vs ReleaseRequires). In the case of release builds, you DEFINITELY don't want fail-fast behavior; and arguably you'd want consistent behavior but more checks in debug builds.

My biggest problems with CodeContracts have been:

  1. The post-processing step slows down the build a HUGE amount (CodeContracts seems to account for 80% of the build time). This is the main reason I've considered dropping CodeContracts on several occasions.
  2. The syntax could be more readable/expressive/concise
  3. The implementation is completely broken on Mono - this is the primary reason why developers with Macs are having to run Windows VMs.

I would expect that this proposal would address 1 and 2 at a minimum, so I'm a huge fan.

Another important trait to preserve is the ability to intercept contract failures and customize the response: log them, pop up a dialog, etc.

jaredpar commented 9 years ago

@johncrim

Could you explain why run-time contract violation shouldn't throw an exception?

Contract violations occur when code has called into a method and provided invalid arguments or when the type itself was in an invalid state. This represents a coding error in the calling code. It simply failed to meet the basic contract of the method in question.

The safest thing to do in the face of a coding error is to stop executing code. The program is in an undefined state and continuing to execute code could lead to all manner of issues including: data corruption, later errors which mask the original bug, etc ...

This is why contract violations use Environment.FailFast (or the like) in the event of a failure. It ensures the process will be torn down in the most expedient manner possible. Crash dumps will point directly at the bug in question. Throwing an exception is instead a way to guarantee that user code will continue executing the program in an undefined state.

ulrichb commented 9 years ago

I totally agree that in theory a developer's / coding error in the calling code should immediately stop the program.

But: Are we sure that in practice there are no legitimate use cases where we want to fail into a safe state in an exception handler (i.e. log errors using existing infrastructure; send error replies in a distributed system; ...) even in case of developer errors i.e. contract violations at run-time?

jaredpar commented 9 years ago

@ulrichb

Sure but in those case I would say that exceptions are a better choice than contracts. In my mind I view the difference between exceptions and contracts as follows:

ulrichb commented 9 years ago

@jaredpar

What I meant was, that even in the case that we have a contract violation due to a developer error / bug in the program, do we always want to FailFast()?

One example: If we implement an HTTP server and request input leads to a precondition violation (due to a bug in the program), do we want to crash the whole server application (including all other request threads)? I think we should have the possibility that the contract violation throws an exception, i.e. just aborts the current request, and logs the contract violation, like it would happen with thrown NullReferenceExceptions (which are also bugs).

aarondandy commented 9 years ago

@jaredpar

I could understand that view if static analysis tools were forced to run for everybody. Currently with the Code Contracts implementation I think it is rare for a user of a library to actually run static analysis against their project. Even if it was baked into the compiler I would think that especially during day-to-day development activities some people would want to temporarily disable static analysis if they are building and testing frequently. That is how I work anyway. In these cases it is pretty nice if I just get an ArgumentNull exception that I can see in my test and say "oops!"

I do still agree with "contract violation shouldn't throw an exception" but for a totally different reason I think. I like to see it kind of like driving with a sharp spike on your steering wheel, you would hopefully take extra care to not rear-end anybody. I find that when I am more likely to get an exception (or any failure really) due to violating a contract I am less likely to violate the contract as it makes me more careful. Static analysis tools help a bunch as well in that area.

jaredpar commented 9 years ago

@ulrichb

One example: If we implement an HTTP server and request input leads to a precondition violation (due to a bug in the program), do we want to crash the whole server application (including all other request threads)?

This is confusing user input validation with method argument validation.

User input should never be done with a contract because as you pointed out it shouldn't be a tool to crash the process. That type of sanitation should be a separate process.

jaredpar commented 9 years ago

@aarondandy

I think it is rare for a user of a library to actually run static analysis against their project. Even if it was baked into the compiler I would think that especially during day-to-day development activities some people would want to temporarily disable static analysis if they are building and testing frequently

I do want to be clear that the compiler won't be doing any static analysis on the contracts the users supplies. These are simply a tool for argument validation.

That being said the output of contracts is expressive enough that theorem provers can run over the code.

ulrichb commented 9 years ago

@jaredpar

... leads to a precondition violation (due to a bug in the program) ...

jaredpar commented 9 years ago

@ulrichb

How is that inconsistent with my argument?

aarondandy commented 9 years ago

@jaredpar I may have misunderstood your previous comments, so ignore if so. The point I am making is that if FastFail is the only way that contracts are enforced I think it would be vary inconvenient for users of those methods. I think that having a choice between a contract throwing an exception or doing fastfail is important.

ulrichb commented 9 years ago

How is that inconsistent with my argument? User input should never be done with a contract because as you pointed out it shouldn't be a tool to crash the process. That type of sanitation should be a separate process.

You again ignored my assumption. Given we have a bug in our program and user input is passed non-sanitized to a precondition-guarded method. Given this assumption, do we always want to FailFast?

jaredpar commented 9 years ago

@ulrichb

Given we have a bug in our program and user input is passed non-sanitized to a precondition-guarded method. Given this assumption, do we always want to FailFast?

Yes.

The alternative is to continue running a program which has just proven that it's not doing user input sanitation. Allowing it to continue is a recipe for attacks like SQL injections to occur.

aarondandy commented 9 years ago

@ulrichb @jaredpar

But I could also terminate execution by not catching the exception or (properly) re-throwing it. You may not agree with somebody's view on how to handle contract failures like they don't agree with yours. I think that right there shows that a choice should be allowed.

ulrichb commented 9 years ago

@jaredpar

Allowing it to continue is a recipe for attacks like SQL injections to occur.

I never spoke about continuing processing the request. My example was to enter a safe state:

... aborts the current request, and logs the contract violation ...

jaredpar commented 9 years ago

@aarondandy

I think that right there shows that a choice should be allowed.

Developers do have a choice: if believe that failure of a particular precondition represents a recoverable action then don't use contracts, use manual verification and exceptions as they are today. Contracts should only be used for places that indicate programming errors.

RichiCoder1 commented 9 years ago

@jaredpar aka. the eponymous // this should never happen?

fschmied commented 9 years ago

@jaredpar As you said, contracts are simply a tool for argument validation. Passing arguments that do not fulfill the contract is a programming error.

Programming errors exist in code during development and QA. If code currently being developed or quality-tested crashed the current process (assuming Environment.FailFast), development, testing, and bug analysis would all become very inconvenient. (Think about the unit test runner crashing all the time because of programming errors. No more red tests, just abandoned execution.)

Programming errors exist even in production code, if they weren't found during development and QA. If production code failed fast due to a bug without any chance to produce a log, production bug analysis would become even harder than it already is. If a whole web server crashed due to one request triggering a bug, a small error could suddenly become a tremendous security issue - denial of service made easy.

The .NET Framework already contains a number of exception classes used to report contract validations; see the family of ArgumentExceptions. Similarly, deferencing a null value or performing an invalid cast, which clearly are programming errors, throw NullReferenceExceptions and InvalidOperationExceptions. The same reasoning of why these exceptions exist also applies to a new hypothetic language-supported contract feature.

Exceptions indicating that a programming error occurred are not meant to be caught and handled (or even ignored). They are not provided as a way to continue execution in a broken state. They are instead meant as a means to terminate execution of the current "operation", falling back the call stack up to a safe location, where the application can decide what to do about that unexpected error. It can log it, decide to terminate the current request, etc.

jaredpar commented 9 years ago

@fschmied

They are not provided as a way to continue execution in a broken state. They are instead meant as a means to terminate execution of the current "operation", falling back the call stack up to a safe location, where the application can decide what to do about that unexpected error.

This is the root of the disagreement. How can the application make an educated decision that it is a safe location? The application has identified that the code it is calling has a coding error in it. That is all it knows.

It has no understanding of why this failure occurred, if the code was left in a recoverable state or really even what component the failure came from. The error could have occurred directly in the code that was called or even transitive dependencies. The argument exception could be anything from a negative index to passing unsatized user input to a back end. There is just no way of knowing.

The application simply cannot make an informed decision on whether or not it is safe to continue executing. This is why termination is the best option.

mirhagk commented 9 years ago

The application simply cannot make an informed decision on whether or not it is safe to continue executing. This is why termination is the best option.

There are quite a few circumstances when the application can make an informed decision to continue executing. If action is isolated and not critical to the application itself, then it can carry on. If you have a feature in your game to share your high score with your friends, it doesn't need to kill the entire application when saving the high score fails a contract.

Your POV seems to be that any programming error could leave the program in a completely undefined state, but this isn't like C++ where the stack could be corrupted by any section of the code. You can know which parts of the program can't be affected and are safe to continue. There may be situations where you are writing critical code and maybe you do want fail fast, but I hardly think that it's impossible to know which part of your program failed.

It's just simply ridiculous to assume that the correct action would always be to kill the process immediately. If microsoft word had a bug when it saved to a network path because of a coding error, would you want it to immediately kill the app? No, you'd want it to save the file to a temporary location, give an error message to the user, log the problem, and then offer to try and restore the file when it loads next time.

HaloFour commented 9 years ago

I think that it's absolutely silly for a contract violation on argument validation to cause the entire process to crash. Implementing it that way is a sure-fire way to guarantee that the feature is never used, at least by anyone writing anything non-esoteric. The point of that side of this feature is argument validation, and argument validation is inherently something from which the program can recover in some manner. And frankly, if it can't, the caller can make that decision by not catching the exception. This is how all implementations of code contracts that I've ever seen works on .NET now or in any other language.

On the other side of contracts, output validation, I can somewhat more understand this decision. In that case the state really is corrupt and it's hard to determine what is the appropriate course of action. However, even then I think failing fast is horribly abrupt.

In short, to fail fast is not only incredibly unintuitive and unexpected, but it will also wreak absolute havoc on all existing testing frameworks designed to help developers find and catch these very problems. Rather than make developer's lives easier it will only make it significantly harder. Or, frankly, it just won't be used, at least by anybody who understands what it does.

jaredpar commented 9 years ago

@mirhagk

Your POV seems to be that any programming error could leave the program in a completely undefined state, but this isn't like C++ where the stack could be corrupted by any section of the code.

True, managed code makes it less likely, though definitely not impossible, for memory / stack to be in corrupted state. However this is not the only type of bug that can lead to undefined behavior. There are many other cases:

These are represent real errors that can lead to data loss or data exposure.

Customers will be annoyed if your program crashes a lot, they'll be furious if you corrupt their data.

You can know which parts of the program can't be affected and are safe to continue.

Sorry this is just not true. It is nearly impossible in a sufficiently large program to isolate the effects on data from one piece of code to another. There are just so many ambient ways in which they can share data.

If microsoft word had a bug when it saved to a network path because of a coding error, would you want it to immediately kill the app?

This is an example of IO, an operation which is fundamentally unpredictable and not suitable for contracts. This is instead a place where exceptions shine.

jaredpar commented 9 years ago

@HaloFour

I think that it's absolutely silly for a contract violation on argument validation to cause the entire process to crash. Implementing it that way is a sure-fire way to guarantee that the feature is never used, at least by anyone writing anything non-esoteric.

I don't take the position that contract violations should cause a process failure lightly. I have this belief after years of repeated experiences across a wide variety of code bases where we moved from Debug.Assert or exception style validation to fail fast semantics. Every single time it had the same results.

Short term instability did increase. Years of bugs that were hidden by debug only semantics or exceptions that were eventually swallowed were suddenly un-ignorable (a process crash is a sure fire way of getting a bug filed). After about a month though two things began to happen.

  1. Product stability dramatically increased.
  2. Lots of bugs simply stopped reproing. Turns out they were just side effects of bugs the weak argument validation was hiding.

This pattern repeated across a variety of code bases I worked on over the years. These code bases ranged from small (about ten thousand lines) to very large (millions of lines).

Rather than make developer's lives easier it will only make it significantly harder. Or, frankly, it just won't be used, at least by anybody who understands what it does.

This is exactly how I felt the first time I was introduced to this concept. I thought it was a mistake and that we'd suffer greatly from it. After a couple weeks of living with it and seeing the bugs it was finding I became a convert. All my experiences since then have re-enforced this stance.

All my arguments being made I think this feature is more likely to end up with the two varieties:

The reason being that it's a great back compat story. It allows the unification of all of the existing forms of argument validation into one single feature with a more concise and documentable syntax.

Even if this is the eventual outcome I highly encourage you to embrace fail fast. Yes it will cause short term instability in the code base. In the long run though it will make your code better by removing a whole class of silent failures and the strange bugs that result from them.

aarondandy commented 9 years ago

@jaredpar

I would totally use fail fast, but I would like to be able to use that within my private surface. I really want exceptions for my public surface. I feel that if a user of my code decides to feed exceptions to the exception monster, that is their choice and they (implicitly, their users too) will deal with the consequences.

While you may see the light and I sometimes see it too I don't think people will be very into contracts if it is 100% fail fast, it's just not friendly enough to be practical in my opinion. I will defend and make noise in favor of fail fast just as I do for exceptions however. I want them both and think they both have their place in my code. I don't consider myself special so I am certain that there are many others out there who feel as I do. I think the world would be better with exceptions than with no contracts at all and I strongly feel that people will not take kindly to it, if it is 100% fail fast.

The bigger deal to me is the static analysis. I already have if and throw and Code Contracts so I don't need code contracts added to roslyn. I want something better though, who doesn't? I know that has nothing to do with roslyn from an implementation point of view but if contracts and exceptions are to be separated, I at least want one unified way of encoding contracts for analysis, for both "roslyn contracts" and exceptions, so that they can all be processed together by tools. Xml docs are not enough.

Przemyslaw-W commented 9 years ago

How about exception/fail fast switch in app manifest?

2015-02-13 5:20 GMT+01:00 Aaron Dandy notifications@github.com:

@jaredpar https://github.com/jaredpar

I would totally use fail fast, but I would like to be able to use that within my private surface. I really want exceptions for my public surface. I feel that if a user of my code decides to feed exceptions to the exception monster, that is their choice and they (implicitly, their users too) will deal with the consequences.

While you may see the light and I sometimes see it too I don't think people will be very into contracts if it is 100% fail fast, it's just not friendly enough to be practical in my opinion. I will defend and make noise in favor of fail fast just as I do for exceptions however. I want them both and think they both have their place in my code. I don't consider myself special so I am certain that there are many others out there who feel as I do. I think the world would be better with exceptions than with no contracts at all and I strongly feel that people will not take kindly to it, if it is 100% fail fast.

The bigger deal to me is the static analysis. I already have if and throw and Code Contracts so I don't need code contracts added to roslyn. I want something better though, who doesn't? I know that has nothing to do with roslyn from an implementation point of view but if contracts and exceptions are to be separated, I at least want one unified way of encoding contracts for analysis, for both "roslyn contracts" and exceptions, so that they can all be processed together by tools. Xml docs are not enough.

— Reply to this email directly or view it on GitHub https://github.com/dotnet/roslyn/issues/119#issuecomment-74203216.

fschmied commented 9 years ago

@jaredpar

This is the root of the disagreement. How can the application make an educated decision that it is a safe location? The application has identified that the code it is calling has a coding error in it. That is all it knows. [...] The application simply cannot make an informed decision on whether or not it is safe to continue executing. This is why termination is the best option.

This is not about continuing executing. This is mainly about allowing application code to decide how to fail. Failing could mean:

You can know which parts of the program can't be affected and are safe to continue.

Sorry this is just not true. It is nearly impossible in a sufficiently large program to isolate the effects on data from one piece of code to another. There are just so many ambient ways in which they can share data.

It is safe to execute code after a programming error occurs

I (and I think the others) are not advocating generally catching the exception and just continue running as if nothing happened. But a general purpose programming language simply cannot assume that it is okay to call Environment.FailFast when a programmer made a mistake, this really depends on the concrete application.

As @mirhagk wrote:

If microsoft word had a bug when it saved to a network path because of a coding error, would you want it to immediately kill the app?

This is an example of IO, an operation which is fundamentally unpredictable and not suitable for contracts. This is instead a place where exceptions shine.

No, it can also be because of a coding error. Assume the programmers of Microsoft Word thought that a path had certain properties and built a corresponding contract check. In production, a certain network path does not fulfill those properties, and the contract check fails. You would of course not want Word to simply catch that unexpected error and pretend nothing happened. But you would also not want Word to crash immediately. Instead, you want Word to log the error, try to save your contents as well as it can to a temporary location for recovery, show a user interface, then quit.

The application developers have to be able to decide what happens when an unexpected programming error occurs.

All my arguments being made I think this feature is more likely to end up with the two varieties: [...]

That's not enough. The else throw new ... syntax is great for customizing what exception is thrown. But if the ordinary contract syntax always failed fast, you

In fact, you could only use it in applications where it's okay to immediately crash when a programmer's bug is encountered.

About what you yourself wrote:

I don't take the position that contract violations should cause a process failure lightly. I have this belief after years of repeated experiences across a wide variety of code bases where we moved from Debug.Assert or exception style validation to fail fast semantics. Every single time it had the same results.

Short term instability did increase. Years of bugs that were hidden by debug only semantics or exceptions that were eventually swallowed were suddenly un-ignorable (a process crash is a sure fire way of getting a bug filed).

First, I'm not talking about using debug assertions for contract checking. I'm very much opposed to that. I'm talking about exceptions for signaling contract validation only.

Second, in "exceptions that were eventuelly swallowed", exceptions are not the problem. Swallowing exceptions that indicate programming errors is. IMO, failing fast always is like throwing out the baby with the bath water.

@Przemyslaw-W That would be one way of solving it, yes.

HaloFour commented 9 years ago

I'd rather see method contracts throw exceptions (at least for the requires clause) and add a new language keyword assert to fail-fast if whatever condition is not met.

If you polled regarding this feature I'm sure that the vast, vast majority of developers just want a simpler way to validate arguments and to throw some variation of ArgumentException in the case that the argument is invalid. The alternate syntax for method contracts makes this possible (assuming it would be adopted) but manages to actually be more verbose than coding it by hand. As such a developer has very little impetus to use method contracts.

I agree that Debug.Assert is basically useless and I wouldn't associate method contracts with it at all.

As for the concern about developers just swallowing validation exceptions, sure, that is a valid concern. But the developer has to opt-into being stupid and the language shouldn't be second-guessing whether or not the developer had a valid reason for wanting to handle the exception differently. Given that the default behavior of the run time if an exception manages to bubble up to the threadproc is to fail fast anyway I don't see why method contracts should be jumping the gun on that decision.

MgSam commented 9 years ago

I think "failing fast" is a horrible idea for all the reasons mentioned by other posts. It takes all control away from the programmer. If that was the default behavior of method contracts I can guarantee that I and many others would never use them.

The runtime and language has a robust exception mechanism that allows the user to decide how to handle errors. It's not the role of the language designers to make this decision for us.

Throwing exceptions should absolutely be the default behavior of any method contract.