Add JsonDocumentOptions and use that instead of JsonReaderOptions. #38866

ahsonkhan · 2019-06-25T07:38:17Z

Based on offline discussions, fixes https://github.com/dotnet/corefx/issues/37694#issuecomment-492898975

cc @bartonjs, @JamesNK, @stephentoub, @steveharter

src/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs

src/System.Text.Json/tests/JsonElementWriteTests.cs

stephentoub · 2019-06-25T14:10:10Z

src/System.Text.Json/ref/System.Text.Json.cs

+        private int _dummyPrimitive;
+        public bool AllowTrailingCommas { readonly get { throw null; } set { } }
+        public System.Text.Json.JsonCommentHandling CommentHandling { readonly get { throw null; } set { } }
+        public int MaxDepth { get { throw null; } set { } }


Why are AlllowTrailingCommas and CommentHandling readonly, but MaxDepth isn't? Is that leaking what should be an implementation detail?

The "readonly" was added as part of the auto-generation of the ref. ~~It looks like there is a bug where it isn't marking all the property getters as readonly (specifically for an integer property).~~

~~I don't want to manually change this and keep fighting the auto-gen. Would it be considered a breaking change to mark a property getter readonly in the future?~~

~~@tannergooding, @safern, @ericstj - do you know why the MaxDepth property is not getting marked as readonly here?~~

If I regenerate the ref from the source directory, the readonly keyword on the CommentHandling getter gets removed too (only AllowTrailingCommas retains it). Does the readonly keyword only apply to auto-properties?

Edit:
Nevermind. Looks like if I fix the source, then the auto-gen works as expected and normalizes the ref accordingly.

Looking back at dotnet/arcade#3010 (comment)

Auto properties on mutable structs have their getter marked readonly starting with C# 8.

The AllowTrailingCommas is the only auto-prop. The other two are no longer auto-props.

Right, readonly is added to auto property getters by default.

It would probably be good to mark most of our non-auto property getters as readonly as well; since they shouldn't be mutating state. Note: it was also already determined that using unsafe code to allow lazy initialization in a readonly member should be fine as well (if that comes up as a concern).

src/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs

src/System.Text.Json/src/System/Text/Json/Document/JsonDocumentOptions.cs

stephentoub · 2019-06-25T14:13:33Z

src/System.Text.Json/src/System/Text/Json/Document/JsonDocumentOptions.cs

+            set
+            {
+                if (value < 0)
+                    throw ThrowHelper.GetArgumentException_MaxDepthMustBePositive();


I've asked this on other PRs but I don't think I've seen an answer. Why are we using this throw ThrowHelper.Get* approach rather than either just throw new ArgumentException(...) or ThrowHelper.Throw*? I'm not understanding the purpose of the middle-ground.

I had responded previously, but it probably got missed because it was within a comment thread that was "resolved" (i.e. collapsed).

Essentially, where the throwing is happening right at the top of a public method as part of argument validation, there is (arguably) some value in keeping the throw keyword in the caller.

#38469 (comment)

I don't think we are consistent across the board here. I think, depending on who had written it/what they valued more at the time, they picked one or the other and the rationale for one or the other isn't super concrete (at least as far as I remember). I had used both myself (i.e. wasn't always consistent).

There are two patterns:

throw HelperToGetException(...)

Used to build up an exception with an exception message where building up the message is non-trivially expensive (or if there are multiple arguments and we need to avoid extra comparisons).

Not absolutely necessary, pattern 2 can be used just as well for most of these cases.

The benefit here is that the throw keyword is clearly visible in the caller (and so it's obvious that we aren't returning from this point).

ThrowCallingHelperWhichThrows()

Used when there isn't a single exception type that can be thrown, so a Helper that returns an exception wouldn't be feasible (however, you can also just return the base type Exception in pattern 1).

Moving the throw into the helper makes the caller code smaller.

Consistent with the existing pattern:

corefx/src/Common/src/CoreLib/System/ThrowHelper.cs

Lines 86 to 90 in fb81332

[DoesNotReturn]

internal static void ThrowArgumentException_CannotExtractScalar(ExceptionArgument argument)

{

throw GetArgumentException(ExceptionResource.Argument_CannotExtractScalar, argument);

}

corefx/src/System.Memory/src/System/ThrowHelper.cs

Lines 27 to 29 in fb81332

internal static void ThrowArgumentNullException(ExceptionArgument argument) { throw CreateArgumentNullException(argument); }

[MethodImpl(MethodImplOptions.NoInlining)]

private static Exception CreateArgumentNullException(ExceptionArgument argument) { return new ArgumentNullException(argument.ToString()); }

Ideally, we would just use pattern 2 anywhere it made sense to do so. In some cases, where the throwing is happening right at the top of a public method as part of argument validation, there is (arguably) some value in keeping the throw keyword in the caller. That's probably why we have pattern 1 being used in this instance.

sharplab.io for example

In some cases, where the throwing is happening right at the top of a public method as part of argument validation, there is (arguably) some value in keeping the throw keyword in the caller

What value? Just that the call site has "throw" instead of "Throw"? I'm not sure why that's more beneficial when doing argument validation; if anything I'd think it'd be more beneficial when nestled inside the method, where throws are less common as compared to argument validation.

Regardless, my preference is for consistency here, and if we're going to use ThrowHelper, which is all about perf, being consistent on the form that eeks out the best perf, aka (2).

What value? Just that the call site has "throw" instead of "Throw"?

That (keyword throw rather than method call "Throw") and the control flow is clear (especially where the compiler would complain about things like out parameters not set).

Regardless, my preference is for consistency here, and if we're going to use ThrowHelper, which is all about perf, being consistent on the form that eeks out the best perf, aka (2).

Agreed.

src/System.Text.Json/src/System/Text/Json/Document/JsonDocumentOptions.cs

src/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs

terrajobst · 2019-06-26T00:56:19Z

I've marked the PR as api-approved to express that we approved the API as part of a last minute fire drill for preview 7.

…nDocOptions

ahsonkhan · 2019-06-26T10:27:18Z

src/System.Text.Json/ref/System.Text.Json.cs

        public JsonException(string message) { }
        public JsonException(string message, System.Exception innerException) { }
        public JsonException(string message, string path, long? lineNumber, long? bytePositionInLine) { }
        public JsonException(string message, string path, long? lineNumber, long? bytePositionInLine, System.Exception innerException) { }
        public long? BytePositionInLine { get { throw null; } }
        public long? LineNumber { get { throw null; } }
+        public override string Message { get { throw null; } }


FYI, @steveharter, this was missed.

ahsonkhan · 2019-06-26T10:27:43Z

src/System.Text.Json/ref/System.Text.Json.cs

@@ -447,13 +455,13 @@ public abstract partial class JsonAttribute : System.Attribute
    public abstract partial class JsonConverter
    {
        internal JsonConverter() { }
-        public abstract bool CanConvert(System.Type type);
+        public abstract bool CanConvert(System.Type typeToConvert);


Same here, @steveharter - the source and the ref didn't agree. I have updated the parameter name to match what was API approved.

ahsonkhan · 2019-06-26T10:28:34Z

src/System.Text.Json/ref/System.Text.Json.cs

+    {
+        public JsonStringEnumConverter() { }
+        public JsonStringEnumConverter(System.Text.Json.JsonNamingPolicy namingPolicy = null, bool allowIntegerValues = true) { }
+        public override bool CanConvert(System.Type type) { throw null; }


@JeremyKuhne, I believe the parameter name needs to be typeToConvert to match the base class, and not type.

ghost · 2019-06-26T10:33:26Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan · 2019-06-26T13:24:10Z

Unrelated test failure: https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F38866~2Fmerge/test~2Ffunctional~2Fcli~2Finnerloop~2F/20190626.6/workItem/System.Text.RegularExpressions.Tests/analysis/xunit/System.Text.RegularExpressions.Tests.RegexGroupTests~2FGroupsDanish

RedHat.7.Amd64.Open-x64-Debug
System.Text.RegularExpressions.Tests.RegexGroupTests/GroupsDanish

Unhandled Exception of Type Xunit.Sdk.XunitException
Message :
Timed out after 60000ms waiting for remote process.
\tProcess ID: 3177
\tHandle: 888
Stack Trace :
   at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose(Boolean disposing) in /_/src/Microsoft.DotNet.RemoteExecutor/src/Microsoft.DotNet.RemoteExecutor/RemoteInvokeHandle.cs:line 131
   at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose() in /_/src/Microsoft.DotNet.RemoteExecutor/src/Microsoft.DotNet.RemoteExecutor/RemoteInvokeHandle.cs:line 55
   at System.Text.RegularExpressions.Tests.RegexGroupTests.GroupsDanish() in /_/src/System.Text.RegularExpressions/tests/Regex.Groups.Tests.cs:line 708

cc @ViktorHofer

stephentoub · 2019-06-26T13:45:44Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

        /// </summary>
        /// <exception cref="InvalidOperationException">
        /// Thrown if this property is set after serialization or deserialization has occurred.
        /// </exception>
+        /// <remarks>
+        /// By default, it's set to false, and <exception cref="JsonException"/> is thrown if a trailing comma is encountered.
+        /// </remarks>
        public bool AllowTrailingCommas
        {
            get


This isn't readonly? I'm not clear on when we're choosing to use it in these types and when we're not.

I am not familiar enough with all the JsonSerializerOptions (and the new readonly feature) to know what getters should have it. Presumably most value type property getters should be marked as readonly.

AllowTrailingCommas
DefaultBufferSize
IgnoreNullValues
IgnoreReadOnlyProperties
MaxDepth
PropertyNameCaseInsensitive
ReadCommentHandling
WriteIndented

I don't know about these two:

public JsonNamingPolicy PropertyNamingPolicy public JsonNamingPolicy DictionaryKeyPolicy

corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Line 145 in d1afe6b

public bool IgnoreReadOnlyProperties

corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Line 101 in d1afe6b

public JsonNamingPolicy DictionaryKeyPolicy

cc @steveharter

Would property getters on JsonWriterOptions also be eligible?

corefx/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterOptions.cs

Line 26 in d1afe6b

return (_optionsMask & IndentBit) != 0;

I am going to address this feedback outside this PR.

Nullability of reference types in type of parameter 'nonPublic' doesn't match overridden member.

Ok

Basically, any method which doesn't mutate instance state can be readonly.

Members which will never mutate state should be readonly. Property getters generally fall into this realm.

Members which may mutate state but only as part of deterministic lazy initialization (so one time mutation, two threads initiating concurrently will get the same result/etc) can be readonly.

Members which mutate state cannot be readonly.

Right, I understand when readonly can be used; I was saying I didn't understand the lack of consistency with which we were applying it to these types.

Right, I understand when readonly can be used

I was giving an explanation for @ahsonkhan, since he indicated he didn't understand where they can/should be used :)

stephentoub · 2019-06-26T13:45:53Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

        /// </summary>
        /// <exception cref="InvalidOperationException">
        /// Thrown if this property is set after serialization or deserialization has occurred.
        /// </exception>
        /// <exception cref="ArgumentOutOfRangeException">
        /// Thrown when the max depth is set to a negative value.
        /// </exception>
+        /// <remarks>
+        /// Going past this depth will throw a <exception cref="JsonException"/>.
+        /// </remarks>
        public int MaxDepth
        {
            get => _maxDepth;


stephentoub · 2019-06-26T13:46:03Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

        /// </summary>
        /// <exception cref="InvalidOperationException">
        /// Thrown if this property is set after serialization or deserialization has occurred.
        /// </exception>
        /// <exception cref="ArgumentOutOfRangeException">
        /// Thrown when the comment handling enum is set to a value that is not supported (or not within the <see cref="JsonCommentHandling"/> enum range).
        /// </exception>
+        /// <remarks>
+        /// By default <exception cref="JsonException"/> is thrown if a comment is encountered.
+        /// </remarks>
        public JsonCommentHandling ReadCommentHandling
        {
            get


Ditto. Will stop asking.

stephentoub · 2019-06-26T14:10:11Z

src/System.Text.Json/src/System/Text/Json/Document/JsonDocument.Parse.cs

        /// <returns>
        ///   A JsonDocument representation of the JSON value.
        /// </returns>
        /// <exception cref="JsonException">
        ///   <paramref name="utf8Json"/> does not represent a valid single JSON value.
        /// </exception>
        /// <exception cref="ArgumentException">
-        ///   <paramref name="readerOptions"/> contains unsupported options.
+        ///   <paramref name="options"/> contains unsupported options.


Is this still the case?

I see, we only allow a subset of the enum values.

stephentoub · 2019-06-26T14:12:36Z

src/System.Text.Json/src/System/Text/Json/Reader/JsonReaderOptions.cs

        /// </summary>
+        /// <remarks>
+        /// By default, it's set to false, and <exception cref="JsonException"/> is thrown if a trailing comma is encountered.


Nit: "By default, it's set to false" => "The defaults to false"... same for other occurrences

stephentoub · 2019-06-26T14:13:37Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

        /// </summary>
        /// <exception cref="InvalidOperationException">
        /// Thrown if this property is set after serialization or deserialization has occurred.
        /// </exception>
        /// <exception cref="ArgumentOutOfRangeException">
        /// Thrown when the comment handling enum is set to a value that is not supported (or not within the <see cref="JsonCommentHandling"/> enum range).
        /// </exception>
+        /// <remarks>
+        /// By default <exception cref="JsonException"/> is thrown if a comment is encountered.


"is thrown"... from where? To me at least it sounds like this is saying that this property throws an exception.

Will address the comment feedback separately in the interest of getting the breaking changes in.

ahsonkhan · 2019-06-26T21:20:40Z

cc @BrennanConroy, @wli3, @billwert as an FYI

…otnet/corefx#38866) * Add JsonDocumentOptions and use that instead of JsonReaderOptions. * Address PR feedback. * Auto-generate the reference assembly. * Auto-gen the ref from the source. * Explicitly mark property getters as readonly. * Cleanup and fix converter argument name to typeToConvert. Commit migrated from dotnet/corefx@e4d3297

Add JsonDocumentOptions and use that instead of JsonReaderOptions.

7be05f8

ahsonkhan added the area-System.Text.Json label Jun 25, 2019

ahsonkhan added this to the 3.0 milestone Jun 25, 2019

ahsonkhan self-assigned this Jun 25, 2019

bartonjs reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/tests/JsonElementWriteTests.cs Outdated Show resolved Hide resolved