Add failure text to the end of the metadata document if decompilation fails by sharwell · Pull Request #24188 · dotnet/roslyn

sharwell · 2018-01-11T20:07:24Z

This was originally requested by @siegfriedpammer to help with debugging scenarios. If ILSpy fails with an exception, the text from the exception is added to the end of the final metadata source file as a comment, to aid with diagnostics.

No comment is added if decompilation is disabled (which it is by default).

… fails

jasonmalinowski

Can we inject a failure in unit tests somehow to verify the error handling doesn't crash?

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

jasonmalinowski

The IFormattable trick seems way too fragile to me that we're deferring the string creation until a bit later. I'd far prefer it if we just passed around a Lazy<string>, if for no other reason than it means that stepping through the test won't change the test behavior, since ToString() has a side effect!

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

src/Test/Utilities/Portable/LazyToString.cs

src/EditorFeatures/Test/MetadataAsSource/AbstractMetadataAsSourceTests.cs

jasonmalinowski · 2020-01-21T22:20:39Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

Don't these need to be \r\n if we're generating into a CRLF file? Is this going to result in inconsistent line endings?

(can we add a test either way? Since the test for this more or less duplicates this line, it seems we don't really have a test for this standalone)

Updated to use system line endings

The code is run through the formatter here, so the specific line endings don't matter:

roslyn/src/Features/Core/Portable/MetadataAsSource/AbstractMetadataAsSourceService.cs

Lines 56 to 57 in 052a7c8

var formattedDoc = await Formatter.FormatAsync(

docWithAssemblyInfo, SpecializedCollections.SingletonEnumerable(node.FullSpan), options: null, rules: GetFormattingRules(docWithAssemblyInfo), cancellationToken: cancellationToken).ConfigureAwait(false);

jasonmalinowski · 2020-01-21T22:22:08Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

The file is empty up to this point, right? So the new TextSpan(0,0) doesn't really matter what it is before? Add a comment?

It might or might not be empty at this point. The comment already says it's placed at the top (which can be seen) and ends up at the bottom (covered by tests).

When is it not already empty?

"Insert this at the top" implies there's something after our insertion point, but the test doesn't show anything after. Either we can add an assert here that the existing file is empty, or we're missing a test case.

I'm not aware of a case where it's not empty here. However, the code added by this pull request adheres to the stated behavior whether or not the document is empty at this point.

jasonmalinowski

Back to you @sharwell: removing the block as the Lazy<string> seems nicer.

jasonmalinowski · 2020-01-22T00:23:08Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

+                            // help with diagnosing failures. When the generated source is added to the document, this
+                            // comment will end up at the bottom.
+                            var failureText = e.ToString();
+                            var failureTextAsComment = "// " + failureText.Replace("\n", "\n// ") + Environment.NewLine;


Sorry one more thing that would be nice: add some text along the lines of "the decompilation failed, and here was the failure". Otherwise people might wonder why there's a random exception at the end of the file.

jasonmalinowski · 2020-01-22T00:24:21Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

"Insert this at the top" implies there's something after our insertion point, but the test doesn't show anything after. Either we can add an assert here that the existing file is empty, or we're missing a test case.

jasonmalinowski · 2020-01-22T00:25:44Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

-}}");
-            await GenerateAndVerifySourceAsync(metadataSource, symbolName, LanguageNames.VisualBasic, $@"#Region ""{FeaturesResources.Assembly} ReferencedAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null""
+}}"));
+            await GenerateAndVerifySourceAsync(metadataSource, symbolName, LanguageNames.VisualBasic, new Lazy<string>(() => $@"#Region ""{FeaturesResources.Assembly} ReferencedAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null""


I guess I was just imagining adding an overload for the cases where the Lazy doesn't matter...

jasonmalinowski · 2020-01-22T00:27:38Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.cs

+                    TestExportProvider.EntireAssemblyCatalogWithCSharpAndVisualBasic.WithPart(typeof(ThrowingDecompiledSourceService)))
+                .CreateExportProvider();
+            var throwingService = exportProvider.GetExportedValue<ThrowingDecompiledSourceService>();
+            var expectedException = throwingService.Exception;


Inline expectedException? That way you're not going to risk use of the unthrown exception?

jasonmalinowski · 2020-01-22T00:39:08Z

I'm somewhat surprised we wouldn't use the gold bar? That's what we use for things like refactorings throwing exceptions. Why not be consistent there? I assume that also gets us other telemetry and such...?

Add failure text to the end of the metadata document if decompilation…

1628bdf

… fails

sharwell requested a review from a team as a code owner January 11, 2018 20:07

jasonmalinowski requested changes Jan 11, 2018

View reviewed changes

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs Outdated Show resolved Hide resolved

jaredpar added the Area-IDE label Apr 24, 2018

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 2, 2018

sharwell mentioned this pull request Jan 16, 2020

Improving F12 Decompilation Quality By Passing Fuzzy-Matched References #41007

Merged

Merge remote-tracking branch 'dotnet/master' into ilspy-show-failures

ebdf311

sharwell requested review from a team as code owners January 21, 2020 18:28

sharwell changed the base branch from dev15.6.x to master January 21, 2020 18:28

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 21, 2020

sharwell requested a review from jasonmalinowski January 21, 2020 18:29

jasonmalinowski requested changes Jan 21, 2020

View reviewed changes

sharwell force-pushed the ilspy-show-failures branch from 44a7e66 to 1ec31d7 Compare January 22, 2020 00:06

Update code for clarity and add error handling test

d1b9666

sharwell force-pushed the ilspy-show-failures branch from 1ec31d7 to d1b9666 Compare January 22, 2020 00:08

jasonmalinowski reviewed Jan 22, 2020

View reviewed changes

sharwell mentioned this pull request Jan 22, 2020

Fix issues in MetadataAsSource XML documentation comment conversion #41119

Merged

Base automatically changed from master to main March 3, 2021 23:51

sharwell closed this May 26, 2022

sharwell deleted the ilspy-show-failures branch May 26, 2022 05:13

	var formattedDoc = await Formatter.FormatAsync(
	docWithAssemblyInfo, SpecializedCollections.SingletonEnumerable(node.FullSpan), options: null, rules: GetFormattingRules(docWithAssemblyInfo), cancellationToken: cancellationToken).ConfigureAwait(false);

Conversation

sharwell commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Jan 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharwell commented Jan 11, 2018 •

edited

Loading