-
Notifications
You must be signed in to change notification settings - Fork 20
Fix handling of parameters with default null #85
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
base: master
Are you sure you want to change the base?
Conversation
.Parameters.Select(x => | ||
{ | ||
var dispString = x.ToDisplayString(FullyQualifiedDisplayFormat); | ||
if (x.HasExplicitDefaultValue && x.ExplicitDefaultValue is null && x.NullableAnnotation != NullableAnnotation.Annotated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToDisplayString
can take a NullableFlowState
parameter - could you perhaps use that and avoid the regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NullableFlowState
is only available for ITypeSymbol
, but here we have a IParameterSymbol
, so that overload can't be used.
It's a bit weird, because our FullyQualifiedDisplayFormat
already includes SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier
, but for some reason the questionmark is missing in the output.
But if there's a better solution for the regex, I'm all for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's disappointing. It's particularly odd when IParameterSymbol
has a Type
property that exposes the parameter's type via an ITypeSymbol
😵💫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had to do a bit of regex on parameter symbols in my most recent PR in this repo, and part of the change involves regenerating the type part of a parameter (so we can qualify references to generated interfaces), so in that case the ITypeSymbol
will be available 🤷♀️.
It's difficult when the PRs start to stack up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what you mean now. Sorry, I'm not very experienced with the CodeAnalysis stuff. Sadly the NullableFlowState
parameter doesn't seem to do anything.
I found another approach by using the DisplayParts instead of parsing with regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the DisplayParts
approach. I think I might do the same in my PR and get rid of my regex too. It wasn't feeling very future-proof.
This in an attempt of improving handling of nullable parameters.
In our solution at work, all projects have
<Nullable>disable</Nullable>
. In some of the generated interfaces, we receive the warningwarning CS8625: Cannot convert null literal to non-nullable reference type.
, because the interfaces are generated with#nullable enable
and have parameters likestring input = null
.I've changed the generator so that these parameters activate the nullable mode and add a
?
if it's missing.There were some changes in the tests I didn't expect, e.g. Misc.CustomNameSpace, but I think they're all correct like this.
Note that for 100% correctness, I think we would need to check the semantic model for every method and get the nullable context to decide if a parameter is actually nullable or not.