Operation could destabilize the runtime when using the Grammar Explorer on any sample using VS 2019.

[dammejed]

Hey there!

I recently saw that you updated the package for .net standard, so I wanted to try it out.

Things seem to work just fine when I build and run the project in VS2017,
but when I build and run the grammar explorer in VS2019 RC1 Svc1 (latest at the time I'm logging this), I get an exception during any parse operation:

System.Security.VerificationException: Operation could destabilize the runtime.
   at Irony.Parsing.ParsingContext.ComputeStackRangeSpan(Int32 nodeCount)
   at Irony.Parsing.ReduceParserAction.GetResultNode(ParsingContext context)
   at Irony.Parsing.ReduceParserAction.Execute(ParsingContext context)
   at Irony.Parsing.Parser.ExecuteNextAction()
   at Irony.Parsing.Parser.ParseAll()
   at Irony.Parsing.Parser.Parse(String sourceText, String fileName)
   at Irony.Parsing.Parser.Parse(String sourceText)
   at Irony.WinForms.Highlighter.EditorAdapter.ParseSource(String newText) in C:\Enablon\Irony\Irony.WinForms\Highlighter\EditorAdapter.cs:line 79
   at Irony.WinForms.Highlighter.EditorAdapter.ParserLoop() in C:\Enablon\Irony\Irony.WinForms\Highlighter\EditorAdapter.cs:line 119

Given that this only happens with VS 2019, which is still in RC, I suppose this is a problem for them to fix, but I wanted to log it to make you aware.

For reference, I started the GrammarExplorer completely fresh, then loaded the Irony.Samples.dll as mentioned in the readme.

I chose the c# grammar, then typed "namespace f" in the 'test' tab, which immediately triggered the exception.

[dammejed]

I found the underlying issue.
The difference between VS 2017 and VS 2019 is the default LangVersion selection behavior.

In VS 2017, the default is to default to the latest "major" version of the C# language, which is 7.0.
In VS 2019, the default for SDK projects (which the Irony project now is), is latest "minor", which is 7.3.

If I manually specify LangVersion 7.3 or 7.2, tests fail in both VS 2017 and VS 2019.
If I manually specify LangVersion 7.1 or 7.0, tests again pass in either version of visual studio.

When I run PEVerify on a bad version of the dll, it gives me 4 errors, all related to modifying 'initonly' fields outside of a constructor. I guess it's related to some optimizations that were intended to be made for readonly structs, but nothing in this project is a readonly struct, so I don't understand why the IL would be different. Ultimately I guess it's a bug in the C# compiler.

Seems to be caused by dotnet/roslyn#27382.

I guess for now the solution is to either:

1. Specify 7.1 or below in the 010.Irony project.
   Or
2. Remove the SecurityTransparent assembly attribute from the same project.

[yallie]

Hi Jeff,

thanks a lot for your observations!

That's quite interesting and looks like C# bug to me, too.
Setting the language version to 7.1 seems to be lesser of two evils,
but I'm not sure if we should work around bugs of yet unreleased C# compiler...

I guess we should try to extract a minimal reproduction case
to confirm that's not a bug in Irony.

[dammejed]

Hi Alexey,

To clarify, the compiler that causes the problem is already released. I get exactly the same runtime exceptions if I explicitly specify LangVersion 7.2 or above, then compile in VS2017. I suppose the same will be true in any version of VS that supports the 7.2 Lang version.

I followed a few links from the issue I mentioned to arrive at this closed issue:
dotnet/roslyn#22707

They mention that it's in the roadmap to automatically apply the option <Features>peverify-compat</Features> if the project sniffs out anything that would produce optimizations that break Verifiability of the compiled IL. In this case, some optimizations introduced for readonly fields in structs produce IL that is correct, but no longer considered verifiable by PEVerify, and therefore the CLR. Because the SecurityTransparent attribute enforces Verifiability of all of the methods in the assembly, these optimizations produce the runtime VerificationException.

So it's an intentionally breaking change that only affects few cases (of which Irony is one).

The 3rd option I didn't already mention is to use the peverify-compat feature in the project above.

It's only a problem during build of the Irony project, so it's not urgent, and doesn't affect anyone consuming the already compiled package on nuget.

[dammejed]

Will you accept a PR for one of the above compatibility changes?
Locally, I simply added <Features>peverify-compat</Features>

It seems like the best balance between allowing newer C# features while still letting the package be marked as SecurityTransparent.

[yallie]

To clarify, the compiler that causes the problem is already released.

Ah, so that's worse than I thought.

Will you accept a PR for one of the above compatibility changes?
Locally, I simply added peverify-compat

Sure, thank you! 👍