Skip to content

Block type update in Add-Type cmdlet #9609

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.

Sign up for GitHub

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

Merged
iSazonov merged 4 commits into from
Aug 22, 2019
Merged

Block type update in Add-Type cmdlet #9609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7f7825
Block type update in Add-Type cmdlet
iSazonov May 15, 2019
a813d5b
Use -PassThru in test
iSazonov Jul 9, 2019
5bb086b
Revert "Use -PassThru in test"
iSazonov Jul 9, 2019
1eee5c5
Add comment
iSazonov Jul 26, 2019
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
20 changes: 5 additions & 15 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ protected override void EndProcessing()
// These dictionaries prevent reloading already loaded and unchanged code.
// We don't worry about unbounded growing of the cache because in .Net Core 2.0 we can not unload assemblies.
// TODO: review if we will be able to unload assemblies after migrating to .Net Core 2.1.
private static readonly Dictionary<string, int> s_sourceTypesCache = new Dictionary<string, int>();
private static readonly HashSet<string> s_sourceTypesCache = new HashSet<string>();
private static readonly Dictionary<int, Assembly> s_sourceAssemblyCache = new Dictionary<int, Assembly>();

private static readonly string s_defaultSdkDirectory = Utils.DefaultPowerShellAppBase;
Expand Down Expand Up @@ -1052,7 +1052,7 @@ private void CompileToAssembly(List<SyntaxTree> syntaxTrees, CompilationOptions

private void CheckDuplicateTypes(Compilation compilation, out ConcurrentBag<string> newTypes)
{
AllNamedTypeSymbolsVisitor visitor = new AllNamedTypeSymbolsVisitor(_syntaxTreesHash);
AllNamedTypeSymbolsVisitor visitor = new AllNamedTypeSymbolsVisitor();
visitor.Visit(compilation.Assembly.GlobalNamespace);

foreach (var symbolName in visitor.DuplicateSymbols)
Expand Down Expand Up @@ -1084,16 +1084,9 @@ private void CheckDuplicateTypes(Compilation compilation, out ConcurrentBag<stri
// Visit symbols in all namespaces and collect duplicates.
private class AllNamedTypeSymbolsVisitor : SymbolVisitor
{
private readonly int _hash;

public readonly ConcurrentBag<string> DuplicateSymbols = new ConcurrentBag<string>();
public readonly ConcurrentBag<string> UniqueSymbols = new ConcurrentBag<string>();

public AllNamedTypeSymbolsVisitor(int hash)
{
_hash = hash;
}

public override void VisitNamespace(INamespaceSymbol symbol)
{
// Main cycle.
Expand All @@ -1109,12 +1102,9 @@ public override void VisitNamedType(INamedTypeSymbol symbol)
// It is namespace-fully-qualified name
var symbolFullName = symbol.ToString();

if (s_sourceTypesCache.TryGetValue(symbolFullName, out int hash))
if (s_sourceTypesCache.TryGetValue(symbolFullName, out _))
{
if (hash == _hash)
{
DuplicateSymbols.Add(symbolFullName);
}
DuplicateSymbols.Add(symbolFullName);
}
else
{
Expand All @@ -1127,7 +1117,7 @@ private void CacheNewTypes(ConcurrentBag<string> newTypes)
{
foreach (var typeName in newTypes)
{
s_sourceTypesCache.Add(typeName, _syntaxTreesHash);
s_sourceTypesCache.Add(typeName);
}
}

Expand Down
12 changes: 11 additions & 1 deletion test/powershell/Modules/Microsoft.PowerShell.Utility/Add-Type.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

Describe "Add-Type" -Tags "CI" {
BeforeAll {
$guid = [Guid]::NewGuid().ToString().Replace("-","")
Expand Down Expand Up @@ -65,7 +66,7 @@ Describe "Add-Type" -Tags "CI" {
$code = @"
using System.Management.Automation;
[System.Management.Automation.Cmdlet("Get", "Thing$guid", ConfirmImpact = System.Management.Automation.ConfirmImpact.High, SupportsPaging = true)]
public class AttributeTest$guid : PSCmdlet
public class SMAAttributeTest$guid : PSCmdlet
{
protected override void EndProcessing()

Expand Down Expand Up @@ -207,6 +208,15 @@ public class AttributeTest$guid : PSCmdlet
{ Add-Type -CompilerOptions "/platform:anycpuERROR" -Language CSharp -MemberDefinition "public static string TestString() { return ""}" -Name "TestType1" -Namespace "TestNS" -ErrorAction Stop } | Should -Throw -ErrorId "SOURCE_CODE_ERROR,Microsoft.PowerShell.Commands.AddTypeCommand"
}

It "Throw if the type already exists" {
Add-Type -TypeDefinition "public class Foo$guid {}"

# The cmdlet writes TYPE_ALREADY_EXISTS for every duplicated type and then terminates with COMPILER_ERRORS.
# So here we check 2 errors.
{ Add-Type -TypeDefinition "public class Foo$guid { public int Bar {get {return 42;} }" -ErrorAction SilentlyContinue } | Should -Throw -ErrorId "COMPILER_ERRORS,Microsoft.PowerShell.Commands.AddTypeCommand"
$error[1].FullyQualifiedErrorId | Should -BeExactly "TYPE_ALREADY_EXISTS,Microsoft.PowerShell.Commands.AddTypeCommand"
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jul 8, 2019

Choose a reason for hiding this comment

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

You should use -PassThru to check the ErrorRecord rather than relying on $error[1].

Copy link
Collaborator Author

@iSazonov iSazonov Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The cmdlet writes TYPE_ALREADY_EXISTS for every duplicated type and then terminates with COMPILER_ERRORS. So the test is correct. (Note that we have to use SilentlyContinue)

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jul 25, 2019

Choose a reason for hiding this comment

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

I see. Perhaps add a comment.

Copy link
Collaborator Author

@iSazonov iSazonov Jul 26, 2019

Choose a reason for hiding this comment

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

Comment added.

}

It "OutputType parameter requires that the OutputAssembly parameter be specified." {
$code = "public static string TestString() {}"
{ Add-Type -TypeDefinition $code -OutputType Library } | Should -Throw -ErrorId "OUTPUTTYPE_REQUIRES_ASSEMBLY,Microsoft.PowerShell.Commands.AddTypeCommand"
Expand Down