Emit diagnostics & exceptions for sourcegen handling init-only properties & JsonInclude attributes #58993
Conversation
…it-only inaccessible JsonInclude properties.
eiriktsarpalis
requested review from
layomia and
steveharter
as code owners
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue Details
Contributes to #58770. Should be backported to release/6.0
|
|
|
||
| // ConverterForInt32 throws this exception. | ||
| await Assert.ThrowsAsync<NotImplementedException>(async () => await JsonSerializerWrapperForString.SerializeWrapper(obj, options)); | ||
| // JsonInclude on private getters not supported. |
There was a problem hiding this comment.
Do we test that there are no exceptions (silent ignore) when [JsonInclude] isn't on the property?
There was a problem hiding this comment.
Yeah I think we do. Here's an example:
|
Test failures related to #58927 |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1234449429 |
| @@ -324,7 +324,7 @@ private string GenerateForTypeWithUnknownConverter(TypeGenerationSpec typeMetada | |||
| }} | |||
| else | |||
| {{ | |||
| throw new {InvalidOperationExceptionTypeRef}($""The converter '{{converter.GetType()}}' is not compatible with the type '{{typeToConvert}}'.""); | |||
There was a problem hiding this comment.
With this, we are localizing run-time messages in the generated code, even though we don't localize other run-time exceptions thrown from the runtime AFAIK. Do other generators do this?
My previous thoughts on this we that we shouldn't localize run-time exceptions since we don't do this elsewhere; we should only localize messages shown within VS as errors\warnings. If \ when we decide to localize messages in the runtime, then I think that would be the best time to do this for generated code. Localizing runtime messages may not always be desired -- it does make it harder to search for the message, and increases download size.
Also we generate the message with the current VS locale. Is that what developers want? They may want the message of the current locale that is running and not the locale that it was developed with.
ghost
locked as resolved and limited conversation to collaborators
Contributes to #58770.
Fixes #58292.
Should be backported to release/6.0