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

Treat decompiled sources separately from metadata sources #27940

Merged
merged 5 commits into from
Jun 29, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,17 @@ internal class MetadataAsSourceFileService : IMetadataAsSourceFileService

/// <summary>
/// We create a mutex so other processes can see if our directory is still alive. We destroy the mutex when
/// we purge our generated files.
/// we purge our generated files. This mutex protects files placed under
/// <see cref="_rootMetadataTemporaryPathWithGuid"/>.
/// </summary>
private Mutex _mutex;
private string _rootTemporaryPathWithGuid;
private Mutex _metadataMutex;
/// <summary>
/// This mutex behaves similarly to <see cref="_metadataMutex"/>, but protects decompiled sources placed under
/// <see cref="_rootDecompiledTemporaryPathWithGuid"/>.
/// </summary>
private Mutex _decompiledMutex;
private string _rootMetadataTemporaryPathWithGuid;
private string _rootDecompiledTemporaryPathWithGuid;
private readonly string _rootTemporaryPath;

public MetadataAsSourceFileService()
Expand All @@ -63,16 +70,18 @@ private static string CreateMutexName(string directoryName)
return "MetadataAsSource-" + directoryName;
}

private string GetRootPathWithGuid_NoLock()
private string GetRootPathWithGuid_NoLock(bool allowDecompilation)
{
if (_rootTemporaryPathWithGuid == null)
ref string rootTemporaryPathWithGuid = ref (allowDecompilation ? ref _rootDecompiledTemporaryPathWithGuid : ref _rootMetadataTemporaryPathWithGuid);
ref Mutex mutex = ref (allowDecompilation ? ref _decompiledMutex : ref _metadataMutex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..."

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

return _rootTemporaryPathWithGuid;
return rootTemporaryPathWithGuid;
}

public async Task<MetadataAsSourceFile> GetGeneratedFileAsync(Project project, ISymbol symbol, bool allowDecompilation, CancellationToken cancellationToken = default)
Expand Down Expand Up @@ -103,8 +112,8 @@ public async Task<MetadataAsSourceFile> GetGeneratedFileAsync(Project project, I
{
InitializeWorkspace(project);

var infoKey = await GetUniqueDocumentKey(project, topLevelNamedType, cancellationToken).ConfigureAwait(false);
fileInfo = _keyToInformation.GetOrAdd(infoKey, _ => new MetadataAsSourceGeneratedFileInfo(GetRootPathWithGuid_NoLock(), project, topLevelNamedType));
var infoKey = await GetUniqueDocumentKey(project, topLevelNamedType, allowDecompilation, cancellationToken).ConfigureAwait(false);
fileInfo = _keyToInformation.GetOrAdd(infoKey, _ => new MetadataAsSourceGeneratedFileInfo(GetRootPathWithGuid_NoLock(allowDecompilation), project, topLevelNamedType, allowDecompilation));

_generatedFilenameToInformation[fileInfo.TemporaryFilePath] = fileInfo;

Expand Down Expand Up @@ -381,18 +390,18 @@ private void RemoveDocumentFromWorkspace_NoLock(MetadataAsSourceGeneratedFileInf
_openedDocumentIds = _openedDocumentIds.RemoveKey(fileInfo);
}

private async Task<UniqueDocumentKey> GetUniqueDocumentKey(Project project, INamedTypeSymbol topLevelNamedType, CancellationToken cancellationToken)
private async Task<UniqueDocumentKey> GetUniqueDocumentKey(Project project, INamedTypeSymbol topLevelNamedType, bool allowDecompilation, CancellationToken cancellationToken)
{
var compilation = await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false);
var peMetadataReference = compilation.GetMetadataReference(topLevelNamedType.ContainingAssembly) as PortableExecutableReference;

if (peMetadataReference.FilePath != null)
{
return new UniqueDocumentKey(peMetadataReference.FilePath, project.Language, SymbolKey.Create(topLevelNamedType, cancellationToken));
return new UniqueDocumentKey(peMetadataReference.FilePath, project.Language, SymbolKey.Create(topLevelNamedType, cancellationToken), allowDecompilation);
}
else
{
return new UniqueDocumentKey(topLevelNamedType.ContainingAssembly.Identity, project.Language, SymbolKey.Create(topLevelNamedType, cancellationToken));
return new UniqueDocumentKey(topLevelNamedType.ContainingAssembly.Identity, project.Language, SymbolKey.Create(topLevelNamedType, cancellationToken), allowDecompilation);
}
}

Expand Down Expand Up @@ -438,12 +447,19 @@ public void CleanupGeneratedFiles()
{
using (_gate.DisposableWait())
{
// Release our mutex to indicate we're no longer using our directory and reset state
if (_mutex != null)
// Release our mutexes to indicate we're no longer using our directory and reset state
if (_metadataMutex != null)
{
_metadataMutex.Dispose();
_metadataMutex = null;
_rootMetadataTemporaryPathWithGuid = null;
}

if (_decompiledMutex != null)
{
_mutex.Dispose();
_mutex = null;
_rootTemporaryPathWithGuid = null;
_decompiledMutex.Dispose();
_decompiledMutex = null;
_rootDecompiledTemporaryPathWithGuid = null;
}

// Clone the list so we don't break our own enumeration
Expand Down Expand Up @@ -541,23 +557,26 @@ private class UniqueDocumentKey : IEquatable<UniqueDocumentKey>
private readonly AssemblyIdentity _assemblyIdentity;
private readonly string _language;
private readonly SymbolKey _symbolId;
private readonly bool _allowDecompilation;

public UniqueDocumentKey(string filePath, string language, SymbolKey symbolId)
public UniqueDocumentKey(string filePath, string language, SymbolKey symbolId, bool allowDecompilation)
{
Contract.ThrowIfNull(filePath);

_filePath = filePath;
_language = language;
_symbolId = symbolId;
_allowDecompilation = allowDecompilation;
}

public UniqueDocumentKey(AssemblyIdentity assemblyIdentity, string language, SymbolKey symbolId)
public UniqueDocumentKey(AssemblyIdentity assemblyIdentity, string language, SymbolKey symbolId, bool allowDecompilation)
{
Contract.ThrowIfNull(assemblyIdentity);

_assemblyIdentity = assemblyIdentity;
_language = language;
_symbolId = symbolId;
_allowDecompilation = allowDecompilation;
}

public bool Equals(UniqueDocumentKey other)
Expand All @@ -570,7 +589,8 @@ public bool Equals(UniqueDocumentKey other)
return StringComparer.OrdinalIgnoreCase.Equals(_filePath, other._filePath) &&
object.Equals(_assemblyIdentity, other._assemblyIdentity) &&
_language == other._language &&
s_symbolIdComparer.Equals(_symbolId, other._symbolId);
s_symbolIdComparer.Equals(_symbolId, other._symbolId) &&
_allowDecompilation == other._allowDecompilation;
}

public override bool Equals(object obj)
Expand All @@ -584,7 +604,8 @@ public override int GetHashCode()
Hash.Combine(StringComparer.OrdinalIgnoreCase.GetHashCode(_filePath ?? string.Empty),
Hash.Combine(_assemblyIdentity != null ? _assemblyIdentity.GetHashCode() : 0,
Hash.Combine(_language.GetHashCode(),
s_symbolIdComparer.GetHashCode(_symbolId))));
Hash.Combine(s_symbolIdComparer.GetHashCode(_symbolId),
_allowDecompilation.GetHashCode()))));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Immutable;
using System.IO;
using System.Text;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.Editor.Implementation.MetadataAsSource
Expand All @@ -21,16 +22,24 @@ internal sealed class MetadataAsSourceGeneratedFileInfo

private readonly ParseOptions _parseOptions;

public MetadataAsSourceGeneratedFileInfo(string rootPath, Project sourceProject, INamedTypeSymbol topLevelNamedType)
public MetadataAsSourceGeneratedFileInfo(string rootPath, Project sourceProject, INamedTypeSymbol topLevelNamedType, bool allowDecompilation)
{
this.SourceProjectId = sourceProject.Id;
_parseOptions = sourceProject.ParseOptions;
this.Workspace = sourceProject.Solution.Workspace;
this.LanguageName = sourceProject.Language;
this.LanguageName = allowDecompilation ? LanguageNames.CSharp : sourceProject.Language;
if (sourceProject.Language == LanguageName)
{
_parseOptions = sourceProject.ParseOptions;
}
else
{
_parseOptions = Workspace.Services.GetLanguageServices(LanguageName).GetRequiredService<ISyntaxTreeFactoryService>().GetDefaultParseOptionsWithLatestLanguageVersion();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider ?:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down