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

Exceptions in indexers are always quietly ignored for IDictionary<T1,T2> #10655

Closed
mklement0 opened this issue Sep 30, 2019 · 10 comments

Comments

@mklement0
Copy link
Contributor

commented Sep 30, 2019

Update: As @SeeminglyScience's states below, ignoring of exceptions seems to be limited to indexing with types that implement IDictionary<T1, T2>, in which potential exceptions are deliberately avoided with .TryGetValue() calls.

An argument could be made that this behavior makes sense for the sake of symmetry with PowerShell's regular, non-generic (ordered) hashtables, which themselves ignore non-existent keys.

It does amount to PowerShell modifying the behavior of .NET types in a non-obvious manner, however.


At least with Set-StrictMode -Version 3 or higher, using an array's indexer with an out-of-bounds index results in a statement-terminating error.

The docs state with respect to version 3:

Prohibit out of bounds or unresolvable array indexes.

By contrast, other exceptions appear to be quietly ignored, irrespective of the effective Set-StrictMode setting.

A follow-up question is: should exceptions other than System.IndexOutOfRangeException and System.Collections.Generic.KeyNotFoundException always be surfaced?

Other types of exceptions are probably rare, but they do occur; e.g.:

# Note: Called  via .Item() instead of indexer syntax ([...]), 
# to ensure that the exception surfaces.
# Throws a System.ArgumentException
[Newtonsoft.Json.Linq.JObject]::Parse('{"foo":1}').Item([datetime]::now)

Steps to reproduce

Set-StrictMode -Version 3

# OK - exception (statement-terminating error)
{ (0,1)[2] } | should -Throw -ErrorId System.IndexOutOfRangeException

# Does NOT throw an exception, even though the underlying type does.
# Note: The non-generic [hashtable] type (@{ ... }) does NOT throw an exception,
#           but System.Collections.Generic.Dictionary<T, T> does.
{ $dt = [Collections.Generic.Dictionary[string, string]]::new(); $dt['nosuch'] } |
  Should -Throw -ErrorId System.Collections.Generic.KeyNotFoundException

Expected behavior

Both tests should pass.

Actual behavior

The 2nd test fails:

Expected an exception, to be thrown, but no exception was thrown.

That is, the exception that occurs in the [System.Collections.Generic.Dictionary] indexer is quietly ignored.

You can surface it if you call the underlying .Item() method (parameterized property) directly:

$dt = [Collections.Generic.Dictionary[string, string]]::new(); $dt.Item('nosuch')

This reports the following error (ultimately a System.Collections.Generic.KeyNotFoundException exception):

Exception getting "Item": "The given key 'nosuch' was not present in the dictionary"

Environment data

PowerShell Core 7.0.0-preview.4
@vexx32

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2019

@mklement0 similar is seen when accessing hashtable / dictionary keys with an invalid key.

Not sure that their behaviour is concretely defined with StrictMode enabled, but I would tend to think that StrictMode should surface that kind of exception as well for parity with how array indexes work. 👍

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Thanks, @vexx32 - didn't know that about hashtables / dictionaries. Can you give a quick example?

@vexx32

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2019

Set-StrictMode -Version 3
$hashtable = @{}
$hashtable['nonexistentkey'] # no error

The same is true without StrictMode enabled, of course. Though I'm not sure that's technically the same, I guess? Since arrays have defined sizes, you can go outside their bounds, but dictionaries typically aren't a bounded set by design. It may be the same for the JObject indexer?

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

@vexx32:

It's not technically the same, because Hashtable itself quietly ignores non-existent keys - no exception happens (except if you pass $null).

By contrast, the (directly implemented) JObject indexer does throw an exception, which you can see if you call it via .Item():

# Throws exception, which surfaces due to calling via .Item()
[Newtonsoft.Json.Linq.JObject]::Parse('{"foo": "bar"}').Item([datetime]::now)
@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@mklement0 Dictionary<,> does though:

PS> $dt = [Collections.Generic.Dictionary[string, string]]::new()
PS> $dt['what']
# nothing
PS> $dt.get_Item('what')
Exception calling "get_Item" with "1" argument(s): "The given key 'what' was not present in the dictionary."
At line:1 char:1
+ $dt.get_Item('what')
+ ~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : KeyNotFoundException
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Thanks, @SeeminglyScience - I've updated the OP with your example, because a generic dictionary is probably more common in PowerShell than Newtonsoft.Json.Linq.JObject; I've retained the latter as an example of where an indexer throws something other than OutOfRangeException / KeyNotFoundException

@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@mklement0 here's where it happens:

private Expression SafeIndexResult(Expression expr)
{
var exception = Expression.Parameter(typeof(Exception));
return Expression.TryCatch(
expr.Cast(typeof(object)),
Expression.Catch(
exception,
Expression.Block(
Expression.IfThen(Compiler.IsStrictMode(3), Expression.Rethrow()),
GetNullResult())));
}

I think it's pretty safe to say that it's by design. That expression generates code similar to:

try
{
    return (object)IndexExpressionInExprVar();
}
catch (Exception)
{
    if (Compiler.IsStrictMode(3))
    {
        throw;
    }
}

return null;
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Thanks, @SeeminglyScience, but that doesn't seem to be what's happening in practice:

Set-StrictMode -Version 3
# Despite strict mode v3, the following do *not* surface the underlying exceptions.
[Newtonsoft.Json.Linq.JObject]::Parse('{"foo":1}')[[datetime]::now]
[Collections.Generic.Dictionary[string, string]]::new()['nosuch']
@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

Yeah IDictionary<,> follows a different pattern, it does TryGetValue instead:

return new DynamicMetaObject(
Expression.Block(
new ParameterExpression[] { outParam },
Expression.Condition(
Expression.Call(target.Expression.Cast(idictionary), tryGetValue, keyExpr, outParam),
outParam.Cast(typeof(object)),
GetNullResult())),
bindingRestrictions);

Apparently JObject is also IDictionary<,>.

@mklement0 mklement0 changed the title Exceptions in indexers are always quietly ignored Exceptions in indexers are always quietly ignored for IDictionary<T1,T2> Oct 1, 2019
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

Thanks, @SeeminglyScience.

An argument could be made that this behavior makes sense for the sake of symmetry with PowerShell's regular, non-generic (ordered) hashtables, which themselves ignore non-existent keys.

It does amount to PowerShell modifying the behavior of .NET types in a non-obvious manner, however - so I've created MicrosoftDocs/PowerShell-Docs#4868

That said, re-reading the docs, with respect to Set-StrictMode it really is only about numeric array indices, so I'm closing this.

@mklement0 mklement0 closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.