Runtime: BrotliDecoder variables are being used in tests without initialization because it has no fields in the ref assembly

[jnm2]

Pointed out by @AlgorithmsAreCool

https://github.com/dotnet/corefx/blob/9bb93a61365db848aaf914b5dfb8d6684df3335e/src/System.IO.Compression.Brotli/ref/System.IO.Compression.Brotli.cs#L10-L15

Occurrences of missing initialization include:

https://github.com/dotnet/corefx/blob/9bb93a61365db848aaf914b5dfb8d6684df3335e/src/System.IO.Compression.Brotli/tests/BrotliEncoderTests.cs#L70-L71

https://github.com/dotnet/corefx/blob/9bb93a61365db848aaf914b5dfb8d6684df3335e/src/System.IO.Compression.Brotli/tests/BrotliEncoderTests.cs#L369-L375

According to https://github.com/dotnet/corefx/issues/6185, ref assemblies may not contain fieldless structs unless the implementation struct is fieldless.

[jnm2]

/cc @weshaggard re https://github.com/dotnet/corefx/issues/6185#issuecomment-357376047:

All the refs in corefx that have structs have had their refs updated to have private fields that follow the rules defined in our tools
dotnet/buildtools#1859 https://github.com/dotnet/buildtools/blob/master/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs#L141.

[stephentoub]

This begs the question where else might we still have incorrect refs missing fields.

[bartonjs]

I thought we regenerated all the ref.cs files recently using the tool?

[stephentoub]

I think @ahsonkhan had started doing so, but it never got merged?

Though I do see
https://github.com/dotnet/corefx/pull/35557/files#diff-622b0077b1377ce9c1179ce1e2fc3ddc
but it's not updating these fields.

[ahsonkhan]

I had added this as part of https://github.com/dotnet/corefx/pull/35474, particularly https://github.com/dotnet/corefx/pull/35474/commits/1534ca918cb69fda62039663b2c5282785c3b03f and https://github.com/dotnet/corefx/pull/35474/commits/455029a468573492c22ff276bc0fe0d00f6ca52c, but that change was too disruptive to review/merge and I didn't have bandwidth to push it forward, piecemeal.

That PR had a bunch of changes based on generating the ref from source (using the tool), which included param name changes as well like GetMaxCompressedLength(...)

[stephentoub]

but that change was too disruptive to review/merge

@ahsonkhan, were there any other changes that involved adding dummy fields that weren't merged?

[ahsonkhan]

This was the commit that generated the ref from source assemblies (rather than just normalizing the refs): https://github.com/dotnet/corefx/pull/35474/commits/1534ca918cb69fda62039663b2c5282785c3b03f

were there any other changes that involved adding dummy fields that weren't merged?

Yes, a few.
I called out the Brotli one (https://github.com/dotnet/corefx/pull/35474#discussion_r258747557), and there were some other "dummy" field changes, but I don't think many hit the definite assignment bug (because they would be adding an additional dummy fields following our rules for them for correctness/consistency, where at least one already existed).

For instance:
SqlSingle
https://github.com/dotnet/corefx/pull/35474/commits/1534ca918cb69fda62039663b2c5282785c3b03f#diff-72f3403532347d17ca668f66ce8ccd00R3143

Added:
```C#
private object _dummy;

Already had:
```C#
private int _dummyPrimitive;

This one probably needs to be fixed though:
FileSystemEntry
https://github.com/dotnet/corefx/pull/35474/commits/1534ca918cb69fda62039663b2c5282785c3b03f#diff-354915f624b5b4a04772f32034c06ab7R254

[stephentoub]

I don't think many hit the definite assignment bug (because they would be adding an additional dummy fields following our rules for them for correctness/consistency, where at least one already existed)

There's a reason though why both the value type and reference type fields are needed. If there's no object field but the actual type does contain a reference type, the C# compiler will for example enable the type to be used where there's an unmanaged constraint, even though it shouldn't.

[ahsonkhan]

Ah, I see. I hadn't considered other scenarios where these fields are used by the compiler. Good point.
We would then need to fix it in a bunch of places (~10).

[stephentoub]

This one probably needs to be fixed though

Any where an int, object, or generic field were added need to be fixed. They each serve different purposes.

[stephentoub]

We would then need to fix it in a bunch of places (~10).

@ahsonkhan, can you please open a 3.0 issue with the list, or just submit a PR that fixes them?

@ericstj, just FYI.

[ahsonkhan]

Yep, will do.

[ahsonkhan]

can you please open a 3.0 issue with the list, or just submit a PR that fixes them?

See https://github.com/dotnet/corefx/pull/39687

[karelz]

Fixed in master (5.0) in PR dotnet/corefx#39653 and also in release/3.0 branch (3.0) in PR dotnet/corefx#39667.