Skip to content

Replace name with type and codepage in EncodingConversionOverflow exception#62910

Merged
marek-safar merged 4 commits intodotnet:mainfrom
marek-safar:encoding
Jan 5, 2022
Merged

Replace name with type and codepage in EncodingConversionOverflow exception#62910
marek-safar merged 4 commits intodotnet:mainfrom
marek-safar:encoding

Conversation

@marek-safar
Copy link
Contributor

Encoding::name is virtual property that can throw during exception reporting. It also has large dependencies (~4k) on rarely used data.

…eptions

Encoding name is virtual property that can throw during exception
reporting. It also has large dependencies (~4k) on rarely used data.
@ghost ghost assigned marek-safar Dec 16, 2021
@ghost
Copy link

ghost commented Dec 16, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@marek-safar marek-safar marked this pull request as ready for review December 17, 2021 09:18
@ghost
Copy link

ghost commented Dec 17, 2021

Tagging subscribers to this area: @dotnet/area-system-text-encoding, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

Encoding::name is virtual property that can throw during exception reporting. It also has large dependencies (~4k) on rarely used data.

Author: marek-safar
Assignees: marek-safar
Labels:

area-System.Text.Encoding

Milestone: -

@marek-safar marek-safar removed their assignment Dec 17, 2021
// This happens if user has implemented an encoder fallback with a broken GetMaxCharCount
throw new ArgumentException(
SR.Format(SR.Argument_EncodingConversionOverflowBytes, EncodingName, EncoderFallback.GetType()), "bytes");
SR.Format(SR.Argument_EncodingConversionOverflowBytes, GetType(), _codePage, EncoderFallback.GetType()), "bytes");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the concrete type returned from GetType() here going to be meaningful to consumers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the type is abstract I believe so

Copy link
Member

@tarekgh tarekgh Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type name will be very confusing and will not be helpful in most cases. Can we get the encoding name inside try-catch block to avoid throwing case? I am not expecting the encoding name to throw most of the time too. If we need to go with this change I would suggest at least get rid of the type name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an update with GetType out. This exception is rarely thrown and as mentioned in the description EncodingName has a surprisingly large dependency. I tried to simplify this corner case not to impact every .net app.

@marek-safar marek-safar merged commit 3058fa4 into dotnet:main Jan 5, 2022
@marek-safar marek-safar deleted the encoding branch January 5, 2022 09:32
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants