-
Notifications
You must be signed in to change notification settings - Fork 231
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
SE - Nullable: Infer constraints for HasValue
property
#6850
Conversation
7e064bc
to
636e192
Compare
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
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.
It looks like we are on the right track.
I think a second test case would help in understanding what's going on here.
Tag("HasValueAfterNull", hasValue); | ||
Tag("SymbolAfterNull", arg); | ||
"""; | ||
var validator = SETestContext.CreateCS(code, ", bool? arg").Validator; |
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 test would be easier to read if we use int?
instead. It's confusing to keep track of HasValue
, arg
, and BoolConstraint
at the same time. Maybe add a second test case with int?
?
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.
int
doesn't have any constraint on it. I wouldn't mind if you rewrite it in your PRs. As there's ToDo
for you here anyway :)
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.
LGTM
Part of #6812
Set
BoolConstraint
on.HasValue
from knownObjectConstraint
, if present.Do not set
NotNull
for the symbol itself, assomething.HasValue
doesn't mean it cannot be null in this case.