Bug 58291 - Mono.CSharp Evaluator does not compile enums and throws an exception
Summary: Mono.CSharp Evaluator does not compile enums and throws an exception
Status: RESOLVED FIXED
Alias: None
Product: Class Libraries
Classification: Mono
Component: mscorlib (show other bugs)
Version: 5.2 (2017-04)
Hardware: PC Windows
: --- normal
Target Milestone: 15.4
Assignee: Marek Safar
URL:
Depends on:
Blocks:
 
Reported: 2017-07-21 11:55 UTC by Michael Stoyke
Modified: 2017-08-28 12:59 UTC (History)
4 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Diff for a potential bug fix (1.57 KB, application/mbox)
2017-07-21 11:55 UTC, Michael Stoyke
Details
Better fix for enums in Mono.CSharp (1.89 KB, patch)
2017-07-24 19:50 UTC, Michael Stoyke
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Michael Stoyke 2017-07-21 11:55:49 UTC
Created attachment 23713 [details]
Diff for a potential bug fix

When using the evaluator in Mono.CSharp we noticed a problem with enums. Enums were not compiled correctly and the compiler threw and exception in Mono.CSharp.Const.Emit() when calling FieldBuilder.SetConstant (c.GetValue());

Attached to this issue is a diff for a potential bugfix. But I don't understand enough of the mcs codebase to judge the implication of this change. I don't know if it's a good fix or just a workaround that treats the symptoms.

Please review the diff and let me know if this is a proper bugfix so that I can issue a pull request for it. Until then I'll be using this in our project as it seems it WorksForMe(tm)

To reproduce the issue, create an instance of the Mono.CSharp Evaluator and call Run() with something like:

"using System; namespace Test { public class TestClass { private TestEnum _te; public string Get() { return _te.ToString(); } } public enum TestEnum { First, Second } }"
Comment 1 Michael Stoyke 2017-07-24 19:50:05 UTC
Created attachment 23761 [details]
Better fix for enums in Mono.CSharp

I don't know if this new patch is a proper fix for the problem, but the first diff was certainly wrong and this one works for all my use cases. I'm looking forward to get some feedback if this is actually fixing the problems or just hiding the symptoms.
Comment 2 Marek Safar 2017-07-25 14:41:54 UTC
This is actually not mcs issue but SRE.

PR with the fix https://github.com/mono/mono/pull/5262
Comment 3 Michael Stoyke 2017-07-25 15:08:26 UTC
I don't think that this is the right solution to it. The issue is not just related to Mono, it also fails when using .NET (I used 4.6.2 in my tests).
Comment 4 Marek Safar 2017-07-25 15:13:22 UTC
Could you attach the exception you get on .NET ?
Comment 5 Michael Stoyke 2017-07-25 15:17:36 UTC
System.ArgumentException: Constant does not match the defined type.
   at System.Reflection.Emit.TypeBuilder.SetConstantValue(ModuleBuilder module, Int32 tk, Type destType, Object value)
   at System.Reflection.Emit.FieldBuilder.SetConstant(Object defaultValue)
   at Mono.CSharp.Const.Emit() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\const.cs:line 96
   at Mono.CSharp.TypeDefinition.Emit() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\class.cs:line 2228
   at Mono.CSharp.TypeDefinition.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\class.cs:line 2261
   at Mono.CSharp.TypeContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\class.cs:line 339
   at Mono.CSharp.NamespaceContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\namespace.cs:line 876
   at Mono.CSharp.TypeContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\class.cs:line 339
   at Mono.CSharp.NamespaceContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\namespace.cs:line 876
   at Mono.CSharp.TypeContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\class.cs:line 339
   at Mono.CSharp.ModuleContainer.EmitContainer() in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\module.cs:line 602
   at Mono.CSharp.Evaluator.CompileBlock(Class host, Undo undo, Report Report) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\eval.cs:line 783
   at Mono.CSharp.Evaluator.Compile(String input, CompiledMethod& compiled) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\eval.cs:line 268
   at Mono.CSharp.Evaluator.Evaluate(String input, Object& result, Boolean& result_set) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\eval.cs:line 359
   at Mono.CSharp.Evaluator.Run(String statement) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\mcs\eval.cs:line 464
   at Interface.Systems.ScriptingHost.Run(String code) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\Scriptinghost.cs:line 39
   at TestMonoCompiler.Program.Main(String[] args) in D:\Projects\Tests\TestMonoCompiler\TestMonoCompiler\Program.cs:line 44
Comment 6 Marek Safar 2017-07-26 11:35:17 UTC
The proposed patch is not really correct as it defines enum member with underlying type and not the enum type itself.

I made different fix for it instead
Comment 7 Michael Stoyke 2017-07-26 12:09:53 UTC
Awesome. Thanks a lot. I just saw the commit and it looks indeed much better than my hack.

I've also investigated a big more with a debugger. Apparently the main problem is that the Fieldbuilder is incompletely initialized for an enum. It looks like the IsEnum property returns true, but the underlying type property throws an exception.

I also looked at the .NET SRE code and it has proper handling for enums in the SetConstant code. The fact that the Fieldbuilder is throwing the exception when the underlying type property is accessed seems to be the main issue that amde this fail.
Comment 8 Prasad Raghorte 2017-08-02 07:22:39 UTC
@Marek Safar @Michael Stoyke
Can you please provide the fix version for verification.
Comment 9 Marek Safar 2017-08-02 15:40:27 UTC
It has been fixed in mono master only
Comment 10 Michael Stoyke 2017-08-28 12:59:51 UTC
It seems that the emitted type information is still not correct with this fix, should I open a new ticket or reuse this one?

Details:
If we try to iterate the fields (reflection) of a class that is using an enum compiled with this fix, we get an exception:

System.TypeLoadException: Field 'MyEnumParameter' is an enum type with a bad underlying type
at (wrapper managed-to-native) System.RuntimeType:GetFields_native (System.RuntimeType,intptr,System.Reflection.BindingFlags)
at System.RuntimeType.GetFields_internal (System.String name, System.Reflection.BindingFlags bindingAttr, System.RuntimeType reflectedType) [0x0001a] in <8bab4e9291ac48d3a5b37cf4064ebe75>:0 
at System.RuntimeType.GetFieldCandidates (System.String name, System.Reflection.BindingFlags bindingAttr, System.Boolean allowPrefixLookup) [0x0000f] in <8bab4e9291ac48d3a5b37cf4064ebe75>:0 
at System.RuntimeType.GetFields (System.Reflection.BindingFlags bindingAttr) [0x00000] in <8bab4e9291ac48d3a5b37cf4064ebe75>:0