Honor annotations for user defined operators and conversions: #34556
Conversation
- Add a VisitOperandsHonoringAnnotations that will split the state if needed based on annotations - Call it from user defined conversions and binary operators - Add tests
chsienki
added
Area-Compilers
Feature - Nullable Reference Types
| { | ||
| c2.ToString(); | ||
| } | ||
| } |
There was a problem hiding this comment.
} [](start = 4, length = 1)
For completeness, consider calling c.ToString(); c2.ToString(); in each branch. (At that point, it would be better to have two methods, one testing == and the other testing !=, so there's no side-effects from earlier calls to ToString().)
| { | ||
| bool saveDisableDiagnostics = _disableDiagnostics; | ||
| _disableDiagnostics = true; | ||
| VisitArgumentsEvaluateHonoringAnnotations(operands, method.ParameterRefKinds, annotations); |
There was a problem hiding this comment.
VisitArgumentsEvaluateHonoringAnnotations [](start = 16, length = 41)
looking at the other place where we use this method, we do some gymnastics to save/restore the state to avoid side-effects from double-visit. It feels likely that a similar scenario could affect user-defined operators.
A scenario that may hit that (should have no warnings on s.ToString():
string? s= "";
_ = s.ToString() + (s= null); // with a custom `+`
Update: On second thought, that's probably not a good scenario (where saving/restoring state matters). VisitArgumentsEvaluateHonoringAnnotations doesn't produce more diagnostics, it only updates the state.
There was a problem hiding this comment.
I believe the state saving in the other case is because we do some other visits which can affect it, not due to VisitArgumentsEvaluateHonoringAnnotations
In reply to: 270161114 [](ancestors = 270161114)
| node.ConversionKind == ConversionKind.ImplicitUserDefined) && | ||
| !(conversion.Method is null)) | ||
| { | ||
| VisitOperandsHonoringAnnotations(conversion.Method, ImmutableArray.Create(operand)); |
There was a problem hiding this comment.
VisitOperandsHonoringAnnotations [](start = 16, length = 32)
Was there a reason this could not be pushed into ApplyConversion?
Also, there are tuple scenarios worth testing: A converts to B with user-defined conversion, then (B, B) t = (a, a); and variants.
I'm wondering whether we should ignore annotations on such user-defined conversions though. It's not obvious what is the benefit...
There was a problem hiding this comment.
There may also be a problem with not saving/restoring the state here, given that we visit the operand twice. Maybe a scenario like (x.String() + (x = null))-- with a user-defined -.
In reply to: 270163123 [](ancestors = 270163123)
There was a problem hiding this comment.
I originally prototyped it in ApplyConversion, but was trying to make the change smaller: we'd need to handle a potentially split state in other places if we have it in apply conversion.
I'm not sure how the tuple conversions would be used? you could obviously annotate them, but are there scenarios where they would actually have any effect?
In reply to: 270163123 [](ancestors = 270163123)
jcouv
added
the
PR For Personal Review Only
|
@chsienki The lack of annotations on operators (cast and others) is inhibiting the adoption of nullable in my project (we use operators a lot). Will this be merged any time soon? |
|
Bi-yearly check up :) Will this be merged or closed? |
Fixes #32671