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

Add type constraint for Throws and any method requiring Exception #2281

Closed
NN--- opened this Issue Jun 26, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@NN---

NN--- commented Jun 26, 2017

Instead of code:

class Throws {
  public static ExactTypeConstraint TypeOf<TExpected>() { .. }
}

The code should be:

class Throws {
  public static ExactTypeConstraint TypeOf<TExpected>() where T : System.Exception { .. }
}
@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Jun 26, 2017

Contributor

This is interesting.

  • In the CLR it's possible for any object type to be thrown, not just exceptions. Exceptions are the only CLS-compliant things that may be thrown. C# can only catch exception objects unless you use the parameterless catch block. That catches non-exceptions too but doesn't provide the actual thrown object, because in C# you can't throw objects that aren't exceptions.
  • NUnit should accommodate any CLR-compatible language though, not just C#, but right now it does not have the facility to catch anything but exceptions. We'd need to weave IL to do that. see below
  • I don't foresee anyone actually asking for the ability to catch non-exception objects. C++/CLI is not going to be more popular than it has already been and I don't know of any other languages that let you throw just any CLR object. A hypothetical new language probably won't.
  • Adding the proposed constraint is not likely to catch any real-world errors. I'd have a hard time believing that. But what it could do is document the current limitation of NUnit. NUnit is not limited per se- see below

So I'm for this. 👍

Contributor

jnm2 commented Jun 26, 2017

This is interesting.

  • In the CLR it's possible for any object type to be thrown, not just exceptions. Exceptions are the only CLS-compliant things that may be thrown. C# can only catch exception objects unless you use the parameterless catch block. That catches non-exceptions too but doesn't provide the actual thrown object, because in C# you can't throw objects that aren't exceptions.
  • NUnit should accommodate any CLR-compatible language though, not just C#, but right now it does not have the facility to catch anything but exceptions. We'd need to weave IL to do that. see below
  • I don't foresee anyone actually asking for the ability to catch non-exception objects. C++/CLI is not going to be more popular than it has already been and I don't know of any other languages that let you throw just any CLR object. A hypothetical new language probably won't.
  • Adding the proposed constraint is not likely to catch any real-world errors. I'd have a hard time believing that. But what it could do is document the current limitation of NUnit. NUnit is not limited per se- see below

So I'm for this. 👍

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jun 26, 2017

Member

I think the absence of a where clause is due to the fact that the original implementation was non-generic, taking a Type as an argument. We just translated what was already there to generic form.

Member

CharliePoole commented Jun 26, 2017

I think the absence of a where clause is due to the fact that the original implementation was non-generic, taking a Type as an argument. We just translated what was already there to generic form.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Jun 26, 2017

Contributor

@NN--- are you interested in submitting a PR for this?

Contributor

jnm2 commented Jun 26, 2017

@NN--- are you interested in submitting a PR for this?

@jnm2 jnm2 added the help wanted label Jun 26, 2017

@NN---

This comment has been minimized.

Show comment
Hide comment
@NN---

NN--- Jun 26, 2017

@jnm2 Thanks. I didn't think about this. I don't think anyone throws object not subtype of Exception.
Even in native C++ the good practice is to make your class deriving from base exception.
Interesting how many people used custom type here.

I can submit PR when I have time for this.

NN--- commented Jun 26, 2017

@jnm2 Thanks. I didn't think about this. I don't think anyone throws object not subtype of Exception.
Even in native C++ the good practice is to make your class deriving from base exception.
Interesting how many people used custom type here.

I can submit PR when I have time for this.

@WdeBruin

This comment has been minimized.

Show comment
Hide comment
@WdeBruin

WdeBruin Aug 25, 2017

Contributor

Picked this up (First issue / pr, experienced developer but new in contributing)

Contributor

WdeBruin commented Aug 25, 2017

Picked this up (First issue / pr, experienced developer but new in contributing)

@mikkelbu

This comment has been minimized.

Show comment
Hide comment
@mikkelbu

mikkelbu Aug 25, 2017

Member

@rprouse - can you invite @WdeBruin to the NUnit contributors team so that one of us can assign this issue to him.

Member

mikkelbu commented Aug 25, 2017

@rprouse - can you invite @WdeBruin to the NUnit contributors team so that one of us can assign this issue to him.

@jnm2 jnm2 removed the help wanted label Aug 25, 2017

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

@WdeBruin We're glad to have you! I removed the up-for-grabs label to signal that the work is yours and as soon as we can we'll assign you too.

Contributor

jnm2 commented Aug 25, 2017

@WdeBruin We're glad to have you! I removed the up-for-grabs label to signal that the work is yours and as soon as we can we'll assign you too.

@WdeBruin

This comment has been minimized.

Show comment
Hide comment
@WdeBruin

WdeBruin Aug 25, 2017

Contributor

#2383

I did delve into this a bit deeper. Apparently the CLI wraps throws of other types than exception in a runtimewrappedexception, which actually does derive from Exception.
That means that we already always get a type Exception, if I understand correctly.
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimewrappedexception?view=netframework-4.5

Contributor

WdeBruin commented Aug 25, 2017

#2383

I did delve into this a bit deeper. Apparently the CLI wraps throws of other types than exception in a runtimewrappedexception, which actually does derive from Exception.
That means that we already always get a type Exception, if I understand correctly.
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimewrappedexception?view=netframework-4.5

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

I did not know this (https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimewrappedexception?view=netframework-4.5#Remarks, emphasis mine):

Some languages, such as C++, allow you to throw exceptions of any managed type. Other languages, such as Microsoft C# and Visual Basic, require that every thrown exception be derived from the Exception class. To maintain compatibility between languages, the common language runtime (CLR) wraps objects that do not derive from Exception in a RuntimeWrappedException object.

You can use the RuntimeCompatibilityAttribute class to specify whether exceptions should appear wrapped inside catch blocks and exception filters for an assembly. Many language compilers, including the Microsoft C# and Visual Basic compilers, apply this attribute by default to specify the wrapping behavior.

Note that the runtime still wraps exceptions even if you use the RuntimeCompatibilityAttribute class to specify that you do not want them wrapped. In this case, exceptions are unwrapped only inside catch blocks or exception filters.

Contributor

jnm2 commented Aug 25, 2017

I did not know this (https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimewrappedexception?view=netframework-4.5#Remarks, emphasis mine):

Some languages, such as C++, allow you to throw exceptions of any managed type. Other languages, such as Microsoft C# and Visual Basic, require that every thrown exception be derived from the Exception class. To maintain compatibility between languages, the common language runtime (CLR) wraps objects that do not derive from Exception in a RuntimeWrappedException object.

You can use the RuntimeCompatibilityAttribute class to specify whether exceptions should appear wrapped inside catch blocks and exception filters for an assembly. Many language compilers, including the Microsoft C# and Visual Basic compilers, apply this attribute by default to specify the wrapping behavior.

Note that the runtime still wraps exceptions even if you use the RuntimeCompatibilityAttribute class to specify that you do not want them wrapped. In this case, exceptions are unwrapped only inside catch blocks or exception filters.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

By virtue of being compiled by the C# compiler, NUnit does already catch non-exceptions as RuntimeWrappedException.

We should still go ahead with the original change to document the fact that NUnit represents caught non-exception objects as RuntimeWrappedException.

NUnit could also unwrap the original object, which would be a breaking change and require us to reverse this PR. But I don't think we'd ever want to do that.

Contributor

jnm2 commented Aug 25, 2017

By virtue of being compiled by the C# compiler, NUnit does already catch non-exceptions as RuntimeWrappedException.

We should still go ahead with the original change to document the fact that NUnit represents caught non-exception objects as RuntimeWrappedException.

NUnit could also unwrap the original object, which would be a breaking change and require us to reverse this PR. But I don't think we'd ever want to do that.

@rprouse rprouse added this to the 3.8 milestone Aug 26, 2017

@rprouse rprouse closed this Aug 26, 2017

@rprouse rprouse added the closed:done label Aug 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment