Mono: Replace exception strings with those stored in the resx file (#34056) by databunks · Pull Request #78341 · dotnet/runtime

databunks · 2022-11-14T21:04:02Z

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks · 2022-11-14T21:04:02Z

Fixes#34056

ghost · 2022-11-14T21:04:08Z

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.

dnfadmin · 2022-11-14T21:04:16Z

All CLA requirements met.

AaronRobinsonMSFT · 2022-11-14T21:27:02Z

/cc @lambdageek @vargaz

vargaz · 2022-11-14T21:33:20Z

Looks ok to me.

Fixed 2nd parameter for argumentExceptions

jkoritzinsky · 2022-11-14T23:32:00Z

Can we switch all of the string.Format calls to SR.Format?

databunks · 2022-11-15T00:12:56Z

Can we switch all of the string.Format calls to SR.Format?

@jkoritzinsky

Is this just in the path:
/src/mono/System.Private.CoreLib ??

jkoritzinsky · 2022-11-15T00:23:06Z

Yeah only within your change.

string.format -> SR.Format

jkotas

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.

The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

databunks · 2022-11-21T13:49:29Z

There seems to be a lot of opportunities in de-duplicating the new error strings with the existing error strings. I have commented on some, but there is certainly more.
The easy way to do the deduplication is to build a small program that hits the error path, run it on CoreCLR and see what error it produces in that case.

@jkotas

Would a way to do it be like:

have a script which iterates through all of the exception strings in the .resx file
e.g: Arg_Enc (inside resx) -> find SR.Arg_Enc in the path ./src/mono/System.Private.CoreLib

If true dont remove, else remove??

and ill make a backup just in case

then run the build see if theres any missing references

jkotas · 2022-11-21T19:03:03Z

have a script which iterates through all of the exception strings in the .resx file

I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

databunks · 2022-11-23T19:03:52Z

have a script which iterates through all of the exception strings in the .resx file
I do not think you can script it. The error messages are not exactly the same. You need to be looking for same meaning that can be only done by humans (or AI).

ok thanks ill look into it
(also didnt mean to re-request review sorry)

danmoseley · 2023-03-20T17:03:52Z

Not related to this change, but I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?
(Don't change this PR for that, just curious)

akoeplinger · 2023-03-20T20:00:28Z

I see some ApplicationException and base Exception getting thrown, which we generally don't throw, does CLR throw the same in these cases?

No, we should clean those up at some point. You'd expect the same exception being thrown, regardless of runtime implementation.

akoeplinger · 2023-03-20T20:02:00Z

Regarding the CLA, it looks like some issue with the new CLA system since it was signed in the old system according to #78341 (comment) and the status on one of the earlier commits:

I'll try to find someone who can look into that.

Co-authored-by: Dan Moseley <danmose@microsoft.com>

outdated review

Co-authored-by: Dan Moseley <danmose@microsoft.com>

akoeplinger · 2023-03-21T14:41:06Z

@databunks~~this is ready now, would you mind signing the CLA again? You signed right when we were transitioning to a new CLA system and looks like yours got missed.~~

~~You can do so by commenting @dotnet-policy-service agree here.~~

Edit: we were able to update the new system instead so this is handled now.

# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

jkotas · 2023-03-22T21:33:14Z

 FieldInfo fi = internal_from_handle_type(handle.Value, declaringType.Value);
 if (fi == null)
- throw new ArgumentException("The field handle and the type handle are incompatible.");
+ throw new ArgumentException(SR.Argument_FieldPropertyEventAndTypeHandleIncompatibility);


Can this be Argument_ResolveFieldHandle like in CoreCLR?

Theoretically yes but I don't have the type(s) to fill in the format string and RuntimeFieldHandle.GetApproxDeclaringType is only implemented in CoreCLR.
I'd rather keep this as is for now so we can finally get this PR in.

databunks requested a review from marek-safar as a code owner November 14, 2022 21:04

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 14, 2022

AaronRobinsonMSFT added this to the 8.0.0 milestone Nov 14, 2022

AaronRobinsonMSFT added the area-CoreLib-mono label Nov 14, 2022

swapping exception strings from resx file
0d46864

databunks force-pushed the main branch from ac2ec56 to 0d46864Compare November 14, 2022 21:22

AaronRobinsonMSFT reviewed Nov 14, 2022
View reviewed changes

Comment threadsrc/mono/System.Private.CoreLib/src/System/TypeSpec.cs Outdated
Comment threadsrc/mono/System.Private.CoreLib/src/System/Reflection/Emit/DerivedTypes.Mono.cs Outdated

Fixed byref naming/text value
e276c22
Fixed 2nd parameter for argumentExceptions

AaronRobinsonMSFT reviewed Nov 14, 2022
View reviewed changes

Comment threadsrc/mono/System.Private.CoreLib/src/System/Reflection/Emit/DerivedTypes.Mono.cs Outdated

Fixed ByRef Arguments &
bf792d9
string.format -> SR.Format

jkoritzinsky reviewed Nov 15, 2022
View reviewed changes

jkotas previously requested changes Nov 20, 2022
View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 20, 2022

databunks mentioned this pull request Nov 21, 2022
Replace strings with resources in Mono corelib #34056
Closed

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 21, 2022

databunks added 2 commits November 23, 2022 18:47

fixes SRformat only used in exceptions & resx refs
9838ae5

fixing small typo
3eeec78

databunks requested review from jkotas and removed request for marek-safar November 23, 2022 19:02

jkoritzinsky reviewed Nov 23, 2022
View reviewed changes

Comment threadsrc/mono/System.Private.CoreLib/src/System/TypeLoadException.Mono.cs Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

akoeplingerand others added 2 commits March 20, 2023 21:08

Apply suggestions from code review
1ef9d27
Co-authored-by: Dan Moseley <danmose@microsoft.com>

Deduplicate some strings
637e1e1

akoeplinger added 2 commits March 20, 2023 21:49

Fixup string
01f8e4e

Fix errors in resource strings
c1b501c

danmoseley reviewed Mar 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
f38e86e
Co-authored-by: Dan Moseley <danmose@microsoft.com>

jkotas reviewed Mar 21, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

akoeplinger added 3 commits March 21, 2023 19:06

Merge branch main into databunks/main
27fa647
# Conflicts: # src/mono/System.Private.CoreLib/src/Mono/RuntimeMarshal.cs

Remove unused resource
5ad8622

A few more fixes
6974e7c

jkotas reviewed Mar 22, 2023
View reviewed changes

jkotas approved these changes Mar 22, 2023
View reviewed changes

build-analysisBot mentioned this pull request Mar 23, 2023
Tracking issue for CI build timeouts #76454
Closed

akoeplinger changed the title ~~Replacing exception strings with those stored in the resx file (#34056)~~ Mono: Replace exception strings with those stored in the resx file (#34056) Mar 23, 2023

akoeplinger merged commit 830d3d0 into dotnet:mainMar 23, 2023

runfoappBot mentioned this pull request Mar 23, 2023
Infra improvements for Helix #68176
Closed

kotlarmilos mentioned this pull request Mar 28, 2023
[Perf] Linux/x64: 5 Regressions on 3/22/2023 10:50:22 PM dotnet/perf-autofiling-issues#14523
Closed

ghost locked as resolved and limited conversation to collaborators Apr 22, 2023