Treat decompiled sources separately from metadata sources #27940

sharwell · 2018-06-17T19:24:17Z

Fixes #25244
Fixes #25255
Fixes #25731
Fixes #26329
Fixes #27187

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

Fixes dotnet#25244 Fixes dotnet#25731 Fixes dotnet#27187

Fixes dotnet#25255 Fixes dotnet#26329

jasonmalinowski

I believe we're now losing Mutexes which might cause files to be deleted under the process which might end badly. Everything else looks reasonable than that bit though.

jasonmalinowski · 2018-06-20T18:33:03Z

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

            {
                var guidString = Guid.NewGuid().ToString("N");
-                _rootTemporaryPathWithGuid = Path.Combine(_rootTemporaryPath, guidString);
+                rootTemporaryPathWithGuid = Path.Combine(_rootTemporaryPath, guidString);
                _mutex = new Mutex(initiallyOwned: true, name: CreateMutexName(guidString));


This now may be overwriting a previous Mutex created by our process; I'm guessing if the Mutex GCs then another process might clean up things under us? I believe the right fix here is to preserve a single folder, and just maybe add another layer of folders inside of it, i.e. MetadataAsSource-{guid}\Decompiled vs. MetadataAsSource-{guid}\Metadata. Or, hold onto two mutexes, but I think going back to a single directory is easier, and also scales if/when we create a third type of something to go in here...

Two mutexes is easy with the current pattern. Yay ref locals!

jasonmalinowski · 2018-06-20T18:35:14Z

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

            this.Workspace = sourceProject.Solution.Workspace;
-            this.LanguageName = sourceProject.Language;
+            if (allowDecompilation && sourceProject.Language != LanguageNames.CSharp)


If you just move this after this.LanguageName =, then I think you can reduce this all back to _parseOptions = Workspace.Services.GetLanguageServices(this.LanguageName).GetRequriedService().ParseOptions;. Basically, you've already figured out once that if it's decompiled it's always C#; no reason to do it twice.

sharwell · 2018-06-20T19:04:18Z

@jasonmalinowski Updated based on review feedback. Planning to rebase this back to 2 commits once the review completes.

CyrusNajmabadi · 2018-06-20T19:06:11Z

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

        {
-            if (_rootTemporaryPathWithGuid == null)
+            ref string rootTemporaryPathWithGuid = ref (allowDecompilation ? ref _rootDecompiledTemporaryPathWithGuid : ref _rootMetadataTemporaryPathWithGuid);
+            ref Mutex mutex = ref (allowDecompilation ? ref _decompiledMutex : ref _metadataMutex);


i personally hate this :) but i guess i will have to grow to live with these sorts of patterns :)

Yeah, I admire your desire to play with new things, but if we just went back to a single mutex/folder and generated the paths differently, it's just two lines with no fancy tricks whatsoever.

"If you write your code as cleverly as possible..."

CyrusNajmabadi · 2018-06-20T19:08:03Z

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

+            else
+            {
+                _parseOptions = Workspace.Services.GetLanguageServices(LanguageName).GetRequiredService<ISyntaxTreeFactoryService>().GetDefaultParseOptionsWithLatestLanguageVersion();
+            }


nit: consider ?:

Opted against it here. The two branches seem different enough that I like to keep it obvious.

Why still having the branches at all? When would the source project's parse options be different than the default parse options for the language in an observable way?

I'm not saying there's not a reason for this....I just can't tell what it is. I can't read your mind (not sure if good thing or bad thing?) but do we need a comment here?

jasonmalinowski

Would really like us to stop doing the fancy ref tricks when simpler code would do.

jasonmalinowski · 2018-06-20T19:36:27Z

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

+            else
+            {
+                _parseOptions = Workspace.Services.GetLanguageServices(LanguageName).GetRequiredService<ISyntaxTreeFactoryService>().GetDefaultParseOptionsWithLatestLanguageVersion();
+            }


Why still having the branches at all? When would the source project's parse options be different than the default parse options for the language in an observable way?

I'm not saying there's not a reason for this....I just can't tell what it is. I can't read your mind (not sure if good thing or bad thing?) but do we need a comment here?

…ervice

sharwell · 2018-06-26T15:41:27Z

@jasonmalinowski main feedback should be addressed now.

jasonmalinowski · 2018-06-26T18:30:15Z

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

            this.References = sourceProject.MetadataReferences.ToImmutableArray();
            this.AssemblyIdentity = topLevelNamedType.ContainingAssembly.Identity;

-            var extension = sourceProject.Language == LanguageNames.CSharp ? ".cs" : ".vb";
+            var extension = LanguageName == LanguageNames.CSharp ? ".cs" : ".vb";

            var directoryName = Guid.NewGuid().ToString("N");
            this.TemporaryFilePath = Path.Combine(rootPath, directoryName, topLevelNamedType.Name + extension);


Doesn't this still need to have some differentiation for the decompiled vs. not?

It does. Two levels of directory names are used when the keys don't match; this one will still get one unique GUID in the path where before it was getting two.

jasonmalinowski · 2018-06-27T04:35:32Z

@jinujoseph I see this is assigned to me now, who is doing the work to take this for QB approval?

jinujoseph · 2018-06-28T17:56:18Z

approved to merge for 15.8.Preview5 (once green)

sharwell added 2 commits June 17, 2018 14:22

Cache decompiled sources separately from metadata sources

022581c

Fixes dotnet#25244 Fixes dotnet#25731 Fixes dotnet#27187

Always generate C# source files when decompilation is enabled

5a4b1ac

Fixes dotnet#25255 Fixes dotnet#26329

sharwell requested a review from a team as a code owner June 17, 2018 19:24

sharwell added this to the 15.8 milestone Jun 17, 2018

jinujoseph added the Area-IDE label Jun 18, 2018

jasonmalinowski requested changes Jun 20, 2018

View reviewed changes

sharwell added 2 commits June 20, 2018 14:02

Fix failure to create mutex for decompiled sources folder

4a04fae

Simplify parse options selection

f1ed8a7

sharwell force-pushed the separate-decompiled-sources branch from dcaa384 to f1ed8a7 Compare June 20, 2018 19:02

CyrusNajmabadi reviewed Jun 20, 2018

View reviewed changes

jasonmalinowski requested changes Jun 20, 2018

View reviewed changes

Remove unnecessary separation of directories in MetadataAsSourceFileS…

547bac8

…ervice

jinujoseph added the Urgency-Soon label Jun 26, 2018

jinujoseph assigned jasonmalinowski Jun 26, 2018

jasonmalinowski reviewed Jun 26, 2018

View reviewed changes

jasonmalinowski approved these changes Jun 26, 2018

View reviewed changes

sharwell changed the base branch from master to dev15.8-preview4 June 26, 2018 22:39

jinujoseph removed the Urgency-Soon label Jun 26, 2018

sharwell changed the base branch from dev15.8-preview4 to master June 28, 2018 17:44

jinujoseph added the Approved to merge label Jun 28, 2018

jasonmalinowski merged commit 4bf5381 into dotnet:master Jun 29, 2018

sharwell deleted the separate-decompiled-sources branch June 29, 2018 09:55

sharwell mentioned this pull request Aug 8, 2018

Improved User Experience with "Navigate to decompiled sources" #29149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat decompiled sources separately from metadata sources #27940

Treat decompiled sources separately from metadata sources #27940

sharwell commented Jun 17, 2018

jasonmalinowski left a comment

jasonmalinowski Jun 20, 2018

sharwell Jun 20, 2018

jasonmalinowski Jun 20, 2018

sharwell commented Jun 20, 2018

CyrusNajmabadi Jun 20, 2018

jasonmalinowski Jun 20, 2018

CyrusNajmabadi Jun 20, 2018

sharwell Jun 20, 2018

jasonmalinowski Jun 20, 2018

jasonmalinowski left a comment

jasonmalinowski Jun 20, 2018

sharwell commented Jun 26, 2018

jasonmalinowski Jun 26, 2018

sharwell Jun 26, 2018

jasonmalinowski commented Jun 27, 2018

jinujoseph commented Jun 28, 2018 •

edited

Loading

Treat decompiled sources separately from metadata sources #27940

Treat decompiled sources separately from metadata sources #27940

Conversation

sharwell commented Jun 17, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Jun 27, 2018

jinujoseph commented Jun 28, 2018 • edited Loading

jinujoseph commented Jun 28, 2018 •

edited

Loading