Analysis of attributes in conversions of lambdas#56792
Conversation
This reverts commit 3bb400f.
81798a7 to
8171cdf
Compare
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM with nits and rambling
| // var f2 = ([DisallowNull]string? x) => | ||
| Diagnostic(ErrorCode.WRN_NullabilityMismatchInParameterTypeOfTargetDelegate, "([DisallowNull]string? x) =>").WithArguments("x", "lambda expression", "System.Action<string?>").WithLocation(10, 10), | ||
| // (16,10): warning CS8622: Nullability of reference types in type of parameter 'x' of 'lambda expression' doesn't match the target delegate '<anonymous delegate>' (possibly because of nullability attributes). | ||
| // var f3 = ([MaybeNull]out string x) => |
There was a problem hiding this comment.
It's possible this is the correct behavior as designed but it feels wrong that the lambda is incompatible with its own inferred type.
There was a problem hiding this comment.
As I continue to review this I find myself feeling like: if it's effectively required for both the delegate and the lambda to specify the same nullability attributes, why should it be permitted for the lambda to specify nullability attributes, instead of only inheriting the same ones from the delegate.
I definitely think that as it stands, it is better to have the warnings from this PR and the previous related PR than to not have them at this point, particularly since we don't have any scheme or design for "inheriting" delegate attributes onto a lambda. The fact that a warning is given in the case of a mismatch is a good thing.
If there is motivation in the future then perhaps we could design it and make it so lambdas can either implicitly "inherit" the nullability attributes from the delegate--possibly not in terms of emit but just behavior of nullability symbol APIs--or they can optionally repeat the nullable attributes of the target delegate, as they are currently required to do, to provide clarity for the reader. I would not consider it a high priority but the thought does enter my mind.
There was a problem hiding this comment.
This is going into 17.1, so we'll have time to chat/brainstorm. I get the intuition, but I'm wondering what would be the proposed design.
Would we say that nullability attributes get target-typed in conversions from lambdas? Would the lambda really have those attributes?
|
@cston @dotnet/roslyn-compiler for second review. Thanks |
|
@cston @dotnet/roslyn-compiler for second review. Thanks |
Fixes #56668
Fixes #47167
Closes #55649
Relates to test plan #52192