Obsolete Encoding.UTF7 property and UTF7Encoding ctors

[GrabYourPitchforks]

Resolves #32284.

Breaking changes

• The property Encoding.UTF7 is now obsolete as warning (diagnostic id BCL0001).
• The UTF7Encoding constructors are now obsolete as warning (diagnostic id BCL0001). The type itself is not obsolete in order to allow code like if (x is UTF7Encoding) to continue to work without warning.
• The APIs Encoding.GetEncoding(65000 /* UTF-7 code page */) and Encoding.GetEncoding("utf-7") will now throw an "unknown code page" exception.

AppContext can be used to re-enable Encoding.GetEncoding recognizing the string "utf-7" or the code page 65000.

This resulted in some changes to unit tests. /cc @carlossanlop @jozkee @dotnet/ncl to review their areas of ownership and to make sure I didn't torpedo the tests. For the System.Net.Http.Json UTF-7 tests, I removed the test body entirely rather than suppressing, as the point of this change was to disallow HttpClient and friends from honoring Content-Type: ...; charset=utf-7 without explicit opt-in by the app developer.

/cc @terrajobst as well since this our first usage of the "better obsoletion" APIs that were added, so we should settle on URL and diagnostic id prefix.

[ghost]

Tagging subscribers to this area: @tarekgh, @krwq
Notify danmosemsft if you want to be subscribed.

[GrabYourPitchforks]

/cc @jkotalik and @Tratcher since there's some aspnet code which touches the Encoding.UTF7 property

[stephentoub]

Do obsoletion IDs need to be numerical? If not, have we considered using more descriptive names that would avoid the need for a comment? May not be a great idea to break from convention even if it's possible.

[stephentoub]

What custom provider?

[GrabYourPitchforks]

If you call Encoding.RegisterProvider, you can override the return value of Encoding.GetEncoding("utf-7"), Encoding.GetEncoding("utf-8"), and any other encoding instance. If you register a custom provider for UTF-7, we never query the AppContext switch because this switch statement never get hit.

Registering a custom provider doesn't change the return value of the built-in accelerators (like Encoding.UTF7 or Encoding.UTF8) since they're hardcoded to always use the built-in implementation.

[stephentoub]

Right. It's the "re-enabled" lingo that confuses me. There's no way for someone to get at our built-in Encoding to "re-enable" it via Encoding.RegisterProvider, is there? They've have to implement one from scratch, no?

[Tratcher]

/cc @jkotalik and @Tratcher since there's some aspnet code which touches the Encoding.UTF7 property

All AspNetCore usages I see are either checks to prevent it from being used, or tests for explicit usage. We may have to tweak the code a bit, but there shouldn't be any functional changes.

[jkotas]

@eerhardt Anything we want to adjust here to make it easy for the linker to strip the UTF7Encoding?

[eerhardt]

Anything we want to adjust here to make it easy for the linker to strip the UTF7Encoding?

Yes, as it exists today the linker won't be able to strip UTF7Encoding since it is being rooted by System.Text.Encoding::GetEncoding(System.Int32).

To enable stripping, we can make the new switch a full blown feature switch. You can see an example of this in #38129.

Basically the thing we will want to add is an entry into this file

runtime/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

Lines 2 to 6 in 38aa0b7

	<assembly fullname="System.Private.CoreLib">
	<type fullname="System.Globalization.GlobalizationMode">
	<method signature="System.Boolean get_Invariant()" body="stub" value="true" feature="System.Globalization.Invariant" featurevalue="true" />
	</type>
	</assembly>

that tells the linker when LocalAppContextSwitches.EnableUnsafeUTF7Encoding property can be stubbed out as return false;. This will allow any code that branches based on the property to get the branch removed during linking time.

Something like:

    <type fullname="System.LocalAppContextSwitches">
      <method signature="System.Boolean get_EnableUnsafeUTF7Encoding()" body="stub" value="false" feature="Switch.System.Text.Encoding.EnableUnsafeUTF7Encoding" featurevalue="false" />
    </type>

[GrabYourPitchforks]

Thanks @eerhardt for the tip! I'll plumb it through in the next iteration.

[jkotas]

This looks reversed to me. I believe that this would allow UTF7Encoding to be unconditionally enabled; but not trimmed.

[GrabYourPitchforks]

I think you're right. Even though the default value is false, we still want trimming to take place if somebody has set false.

[GrabYourPitchforks]

@jkotas any idea why the wasm tests might be failing?
https://helix.dot.net/api/2019-06-17/jobs/b400b19c-d5d5-41c2-a0a5-10064c66ac86/workitems/System.Text.Encoding.Tests/console

[23:20:06] dbug: test[0]
      [FAIL] System.Text.Encodings.Tests.EncodingMiscTests.NormalizationTest(codepage: 65000, normalized: False, normalizedC: False, normalizedD: False, normalizedKC: False, normalizedKD: False)
      
[23:20:06] dbug: test[0]
      System.NotSupportedException : Support for UTF-7 is disabled. See https://aka.ms/dotnet-warnings/MSLIB0001 for more information.

There is a runtimeconfig.template.json file present in that test directory which re-enables the UTF-7 code paths, so the test project succeeds on all other platforms. See https://github.com/dotnet/runtime/blob/9d6811c5753d0d627d608a4d5d932aee014f43e5/src/libraries/System.Text.Encoding/tests/runtimeconfig.template.json.

[jkotas]

@jkotas any idea why the wasm tests might be failing?

I do not think wasm supports .runtimeconfig.json files. @safern ?

[akoeplinger]

@jkotas any idea why the wasm tests might be failing?

I do not think wasm supports .runtimeconfig.json files. @safern ?

It's not supported right now, but I think we should make it work.

I've opened #38433 to track that, you can add an [ActiveIssue(..., TestPlatforms.Browser)] on the affected tests.

[GrabYourPitchforks]

Awesome - thanks both for the pointer! :)

[GrabYourPitchforks]

Test failure is #38605. I'm just waiting for a review signoff before merging.

@tarekgh I think I addressed all your feedback?

[tarekgh]

Thanks @GrabYourPitchforks. LGTM.

[tarekgh]

@GrabYourPitchforks just checking, did you log and fill a breaking change template for this one? sorry if I missed it.

[GrabYourPitchforks]

@tarekgh Not yet, thanks for the reminder. :)
I'll add it to the docs repo and make sure the aka.ms link forwards to it.

[GrabYourPitchforks]

Breaking change notification: dotnet/docs#19274