Don't generate nullable type when applying value type with unconstrained nullable generic #49154
Conversation
| throw new System.NotImplementedException(); | ||
| } | ||
| }"); | ||
| } |
There was a problem hiding this comment.
can we a few more tests.
- where T is unconstrained
- where T is constrained to
class. - where T is unconstrained, and passed as T? to the base type
- where T is constrained to
class, and passed as T? to the base type - where T is constrained to
struct, and passed as T? to the base type
There was a problem hiding this comment.
Sure, make sure that other cases doesn't regress.
| { | ||
| // value type with nullable annotation may be composed from unconstrained nullable generic | ||
| // doesn't mean nullable value type in this case | ||
| typeSyntax = AddInformationTo(SyntaxFactory.NullableType(typeSyntax), symbol); | ||
| } |
There was a problem hiding this comment.
what about other named types that can be value types. like tuples. can you update as well (or add tests showing they're k).
There was a problem hiding this comment.
Are they special for this case? Here is adding top-level nullability, I couldn't imagine any type to have special semantics. The inner type has been handled above.
There was a problem hiding this comment.
they're a named type that is a value type :) i just want to make sure it works properly and nothing unexpected happens.
There was a problem hiding this comment.
Adding tests should be OK I think
jinujoseph
added
the
Community
|
Well, this is actually duplicated by #49242 . The corresponding issue can be closed now. |
|
@CyrusNajmabadi this has more test coverage than #49242 . Idea? |
You can reopen and just include the tests you want to add. Thanks! |
|
I think it's better to keep this PR, because it adds more comment and addresses another position. |
|
@CyrusNajmabadi Idea on this? Do you think it's worth to keep it? |
|
Azure Pipelines successfully started running 3 pipeline(s). |
ghost
added this to the 
Fixes #48742. Should prevent similar issue in other cases.