Fix for #7128 Methods with return type [object] should return null for an empty result #7138
Conversation
…rn null for an empty result
| static [object] Bar2() { return & {} } | ||
| } | ||
| # Test instance method | ||
| [Foo]::new().Bar1() | Should -BeNullOrEmpty |
There was a problem hiding this comment.
We could explicitly check result type. I believe it make sense to use testcase for two return types (object and int/string) to ensure that default(T) works as expected.
There was a problem hiding this comment.
The only time this code path is hit is if the method returns [object]. All of the other cases are already handled elsewhere. And the object returned is null. How do you propose checking it's type?
There was a problem hiding this comment.
Sorry, it was my misunderstanding of default(T).
Should it be Should -BeNull?
|
If a method is "find a file" what it may indicate that "file not found" if it can not return null? |
| @@ -519,7 +519,10 @@ internal T InvokeAsMemberFunctionT<T>(object instance, object[] args) | |||
| invocationInfo: null, | |||
| propagateAllExceptionsToTop: true, | |||
| args: args); | |||
| Diagnostics.Assert(result.Count == 1, "Code generation ensures we return the correct type"); | |||
| if (result.Count == 0) | |||
There was a problem hiding this comment.
I understand the change, but it feels really dirty because in an indirect way, it allows a method to not return a value, similar to the parser allowing the method to omit a return statement.
Unfortunately I can't think of a cleaner fix. I suppose it's worth adding a comment explaining that this is not to allow methods to omit a return statement.
There was a problem hiding this comment.
Yeah - it feels dirty to me but pipeline semantics make it necessary. I like the idea of adding a comment to explain it.
There was a problem hiding this comment.
Note that a fix in the compiler is slightly nicer from a performance point of view - you would omit the check for non-object returning methods, but that fix is slightly more involved.
I would have asked for that if we could rely on the syntax, but the bug is still an issue with expressions:
class C {
static [object] f() {
$o = & {}
return $o
}
}
[C]::f()
PR Summary
Fix: #7128
If a method was typed to return [object] but no actual value was returned then you would get an index-out-of-range exception because the code expected a value in the result collection but there was none. The fix is to check to see if the array is empty. If it is, then return default(T).
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests