Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[runtime/corlib] Improve MissingMethodExceptions by including message and signature. Fixes #60505 #5941

Merged
merged 1 commit into from Nov 15, 2017

Conversation

@kumpera
Copy link
Member

kumpera commented Nov 3, 2017

This PR is missing the following and I'd love if someone could help me here.

Figure out serialization for MethodMissingException - and whether it's ok to break it?
Alternatively, I could use the existing byte[] signature field in MissingMemberException and store a utf8 version of the string currently stored in the new field.

Sort out linker support, this PR adds a dependency to a private constructor so we need the linker to special case it.

return Environment.GetResourceString("MissingMethod_Name",
ClassName + "." + MemberName +
(Signature != null ? " " + FormatSignature(Signature) : ""));
string res = ClassName + "." + MemberName;

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 4, 2017

Member

this should go under #if MONO

}
}
}

// Called from the EE
private MissingMethodException(String className, String methodName, byte[] signature)
private MissingMethodException(String className, String methodName, String signature, String message) : base (message)

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 4, 2017

Member

You should add a new #if MONO only ctor

(Signature != null ? " " + FormatSignature(Signature) : ""));
string res = ClassName + "." + MemberName;
if (!string.IsNullOrEmpty(signature))
res = string.Format (signature, res);

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 4, 2017

Member

This is culture-specific call which should be avoided

This comment has been minimized.

Copy link
@kumpera

kumpera Nov 6, 2017

Author Member

What's the culture oblivious alternative for string interpolation? The string comes from the runtime.

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 7, 2017

Member

either don't use it or at least use string.Format with CultureInfo.InvariantCulture

if (!string.IsNullOrEmpty(signature))
res = string.Format (signature, res);
else
res += "(<UNK>)";

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 4, 2017

Member

I think adding nothing would be better in this case

@@ -73,5 +77,6 @@ public MissingMethodException(String className, String methodName)
// If ClassName != null, Message will construct on the fly using it
// and the other variables. This allows customization of the
// format depending on the language environment.
string signature;

This comment has been minimized.

Copy link
@marek-safar

marek-safar Nov 4, 2017

Member

I think probably the best solution is [NotSerialized] here

break;
}
}
exception = mono_exception_from_name_four_strings_checked (mono_defaults.corlib, "System", "MissingMethodException", type_name, method_name, signature, message, error_out);

This comment has been minimized.

…… and signature. Fixes #60505

This fix is a bit weird but it's due to some restrictions of the managed API.

MissingMemberException & friends require both ClassName and MemberName to be supplied independently, so the signature
can't simply be applied to MemberName. So we pass it on a separate field.

The next thing is that we want C#'esque signatures, where the return type comes before ClassName.MemberName, so we pass
a format string and apply it in managed.
@kumpera kumpera force-pushed the kumpera:fix_method_missing branch from 4e8da4a to 8e5e356 Nov 14, 2017
@kumpera

This comment has been minimized.

Copy link
Member Author

kumpera commented Nov 14, 2017

The new commit fixes all issues highlighted my Marek.

  • Wrapped changes in referencesources around #if mono
  • Added [NonSerializable] to the new field
  • Used Invariant Culture with string.Format
  • Updated the linker descriptor for corlib
@kumpera

This comment has been minimized.

Copy link
Member Author

kumpera commented Nov 14, 2017

We now get a sweet System.MissingMethodException: void Y.Foo<!0>(!!0)

@marek-safar marek-safar changed the title [WIP][runtime/corlib] Improve MissingMethodExceptions by including message and signature. Fixes #60505 [runtime/corlib] Improve MissingMethodExceptions by including message and signature. Fixes #60505 Nov 15, 2017
@marek-safar marek-safar merged commit 115b290 into mono:master Nov 15, 2017
13 of 16 checks passed
13 of 16 checks passed
OS X x64 Build finished. 63067 tests run, 1306 skipped, 1 failed.
Details
Linux AArch64 Interpreter Build started sha1 is merged.
Details
Linux ARMv7 hard float Build started sha1 is merged.
Details
API Diff No public API changes found.
Details
Linux AArch64 Build finished. 63902 tests run, 1428 skipped, 0 failed.
Details
Linux ARMv5 soft float Build finished. 63866 tests run, 1423 skipped, 0 failed.
Details
Linux ARMv7 hard float Interpreter Build finished. 11117 tests run, 94 skipped, 0 failed.
Details
Linux i386 Build finished. 63905 tests run, 1420 skipped, 0 failed.
Details
Linux x64 Build finished. 63905 tests run, 1422 skipped, 0 failed.
Details
Linux x64 FullAOT Build finished. 21701 tests run, 548 skipped, 0 failed.
Details
Linux x64 Interpreter Build finished. 11126 tests run, 94 skipped, 0 failed.
Details
Linux x64 mcs Build finished.
Details
OS X i386 Build finished. 63067 tests run, 1304 skipped, 0 failed.
Details
Test Result Viewer Click to view aggregated test results (Xamarin internal).
Details
Windows i386 Build finished. 59062 tests run, 1131 skipped, 0 failed.
Details
Windows x64 Build finished. 59084 tests run, 1133 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.