Replace IndexOutOfRangeException with ArgumentOutOfRangeException in vector indexers

[Robmaister]

IndexOutOfRangeException is supposed to only be thrown internally for arrays. Everything else should be throwing ArgumentOutOfRangeExceptions. Even the BCL collections do this.

I copied the Vector3 class out to another project, VS2012's code analysis pointed this out as CA1065

[andykorth]

You might want to submit this to: https://github.com/opentk/opentk also!

[Robmaister]

Oh cool, didn't realize it officially moved to GitHub! I'll definitely submit it there too.

[andykorth]

Yup, it's official and is getting regular updates! Most of the issues I've fixed have already been moved into that branch.

On Nov 22, 2013, at 2:57 PM, Robert Rouhani notifications@github.com wrote:

Oh cool, didn't realize it officially moved to GitHub! I'll definitely submit it there too.

— Reply to this email directly or view it on GitHub.

[BrainSlugs83]

I disagree with the legitimacy of CA1065 for indexers that take only a single integer as a parameter -- such a parameter IS an index; In such cases you should just suppress the warning. -- In all other cases, using ArgumentOutOfRangeException is the correct approach.

[Robmaister]

This is the approach taken by all the BCL collection classes that have indexers, including List<T>. IndexOutOfRangeException inherits from SystemException, which is provided as a means to differentiate between exceptions defined by the system versus exceptions defined by applications and is only thrown by 3 CIL instructions - ldelem, ldelema, and stelem.

If anything, making this change is just conforming to what users expect of C#'s standard library and other applications.

[BrainSlugs83]

Yet, this is clearly not the recommended, or standard (even for Microsoft) practice.

Here's something that perhaps _we can_ agree on: In the XML comments for those indexers which now throw ArgumentOutOfRangeException, can you please add some /// <exception cref="" /> markups for the exception types they throw.

---

Footnotes:

• See code sample provided by Microsoft on Indexers -- Note that if the index is out of range it throws a "new IndexOutOfRangeException" -- this has been the recommended practice for indexer implementation since .NET 1.1 in 2003.
• Note the purpose of an Indexer: "An indexer is a member that enables an object to be indexed in the same way as an array." (emphasis mine.)
• Note also that there is no text on any of the pages that you linked saying that this exception should not be thrown -- I believe you misunderstand (or at least misuse) ".. is provided as a means to differentiate between exceptions defined by the system versus exceptions defined by applications." -- the keyword is "defined". As in, this exception is _defined_ by the system, and not by the application -- it doesn't mean it can't be used by the application, it just can't be inherited and used as a base for application specific exceptions (note that it is a sealed class).
• Also, it is false to say that it is only thrown by 3 CIL instructions. The link you provided says that it is thrown by 3 CIL instructions but nowhere does it say "only".
• There there are several classes in the .NET framework that throw IndexOutOfRangeExceptions (a quick search reveals 15 classes in mscorlib alone).
  • There are interfaces in the BCL that have methods which, when implemented, according to the documentation, must throw IndexOutOfRangeException (System.Data::IDataRecord.GetFieldType for example).
  • String throws IndexOutOfRangeException (string is not an array, but under the hood it might be calling those IL Instructions -- I can't say).
  • System.Windows.Forms has classes which EXPLICITLY throw new IndexOutOfRangeException from indexers;
  • ImageList throws it from a public method (SetKeyName).
  • XNA definitely throws IndexOutOfRangeExceptions (and other SystemException exceptions, such as IOException, etc).
• I have found no such example provided by Microsoft on using ArgumentOfOutRangeException in conjunction with Indexers. The MSDN page on ArgumentOutOfRangeException says that it is for use with methods, it does not say to use it with indexers
• As for what users of C# expect: personally, anytime I use array notation (i.e. to access an indexer) I expect that an IndexOutOfRangeException can be thrown -- it's intuitive. When you're using index-notation, you're passing indices, not arguments.

Feel free to disagree with my assessment, but from where I'm standing, it seems like a matter of personal preference.

[Frassle]

Just to add a voice here, I think BrainSlugs84 is accurate with the assessment that it's a matter of personal preference. I can see where Robmaister is coming from, and I wouldn't put up a fight if the maintainers deemed ArgumentOutOfRange preferable over IndexOutOfRange, but as a personal preference I to expect indexer methods to throw IndexOutOfRange.

ArgumentException (and it's descendants such as ArgumentOutOfRangeException), NotImplementedException, and NotSupportedException all inherit from SystemException. Clearly the distinction between System and Application exception is in definition, not use.

[Robmaister]

Yes, more/better documentation is always better.

As for the sample code provided in specification, the actual BitArray class's indexer throws an ArgumentOutOfRangeException instead.

If you look at all the collections in the System.Collections.Generic namespace, in the case of an indexer with a single int as the index, they all throw ArgumentOutOfRangeException. I always try to keep my own code very similar to the BCL.

I do see the point in string's indexer throwing an IndexOutOfRangeException, as that's a data type and not a collection. Vector/Matrix classes can be either, depending on how you look at it.

As for what else I've read online, people seem split between using the two. I've already submitted the pull request to the opentk/opentk fork. If @thefiddler prefers IndexOutOfRangeException, I can either add another commit that changes it back, or delete the pull request and rewrite my branch's history to not change which exception is thrown but keep the other changes I made, then submit another pull request. I'm fine with it either way.

[Frassle]

"I can either add another commit that changes it back, or delete the pull request and rewrite my branch's history to not change which exception is thrown but keep the other changes I made, then submit another pull request. I'm fine with it either way."

I wouldn't spend time on this unless @thefiddler ask. You've already submitted a pull request with it in just leave it. If it's conclusively deemed problematic/confusing/wrong at some point in the future it can be changed. There seems to be consensus on it being personal preference, let's not argue the color of the garden shed over this.

[Robmaister]

Yeah, I'm not going to change it unless @thefiddler prefers IndexOutOfRangeException and asks for me to change it.