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.

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

Jump to bottom

Don't report rude edits for using declaration changes #51977

Merged
merged 9 commits into from
Mar 26, 2021
Merged

Don't report rude edits for using declaration changes #51977

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
12bf4be
Don't report rude edits for using declaration changes
davidwengier Mar 19, 2021
02b5c5a
Add tests to simulate real world scenarios, both supported and not
davidwengier Mar 19, 2021
f663207
Test that introducing an ambiguous reference still emits the valid code
davidwengier Mar 19, 2021
66fda70
Fix tests
davidwengier Mar 21, 2021
249534f
Merge remote-tracking branch 'upstream/main' into EnCUsings
davidwengier Mar 25, 2021
4d7f074
Use a specific TFM
davidwengier Mar 25, 2021
aaca0b0
Allow changes to imports statements in VB
davidwengier Mar 26, 2021
6da4a4b
No special logic needed for debugging situations
davidwengier Mar 26, 2021
b442f08
Whitespace
davidwengier Mar 26, 2021
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
76 changes: 76 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10500,6 +10500,82 @@ .locals init ([int] V_0,
IL_0023: ldloc.s V_8
IL_0025: ret
}
");
}

[Fact]
public void AddUsing_AmbiguousCode()
{
var source0 = MarkedSource(@"
using System.Threading;

class C
{
static void E()
{
var t = new Timer(s => System.Console.WriteLine(s));
}
}");
var source1 = MarkedSource(@"
using System.Threading;
using System.Timers;

class C
{
static void E()
{
var t = new Timer(s => System.Console.WriteLine(s));
}

static void G()
{
System.Console.WriteLine(new TimersDescriptionAttribute(""""));
}
}");

var compilation0 = CreateCompilation(source0.Tree, options: ComSafeDebugDll);
var compilation1 = compilation0.WithSource(source1.Tree);

var e0 = compilation0.GetMember<MethodSymbol>("C.E");
var e1 = compilation1.GetMember<MethodSymbol>("C.E");
var g1 = compilation1.GetMember<MethodSymbol>("C.G");

var v0 = CompileAndVerify(compilation0);
var md0 = ModuleMetadata.CreateFromImage(v0.EmittedAssemblyData);
var generation0 = EmitBaseline.CreateInitialBaseline(md0, v0.CreateSymReader().GetEncMethodDebugInfo);

// Pretend there was an update to C.E to ensure we haven't invalidated the test

var diffError = compilation1.EmitDifference(
generation0,
ImmutableArray.Create(
SemanticEdit.Create(SemanticEditKind.Update, e0, e1, GetSyntaxMapFromMarkers(source0, source1), preserveLocalVariables: true)));

diffError.EmitResult.Diagnostics.Verify(
// (9,21): error CS0104: 'Timer' is an ambiguous reference between 'System.Threading.Timer' and 'System.Timers.Timer'
// var t = new Timer(s => System.Console.WriteLine(s));
Diagnostic(ErrorCode.ERR_AmbigContext, "Timer").WithArguments("Timer", "System.Threading.Timer", "System.Timers.Timer").WithLocation(9, 21));

// Semantic errors are reported only for the bodies of members being emitted so we shouldn't see any

var diff = compilation1.EmitDifference(
generation0,
ImmutableArray.Create(
SemanticEdit.Create(SemanticEditKind.Insert, null, g1)));

diff.EmitResult.Diagnostics.Verify();

diff.VerifyIL(@"C.G", @"
{
// Code size 18 (0x12)
.maxstack 1
IL_0000: nop
IL_0001: ldstr """"
IL_0006: newobj ""System.Timers.TimersDescriptionAttribute..ctor(string)""
IL_000b: call ""void System.Console.WriteLine(object)""
IL_0010: nop
IL_0011: ret
}
");
}
}
Expand Down
182 changes: 170 additions & 12 deletions src/EditorFeatures/CSharpTest/EditAndContinue/TopLevelEditingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ public void UsingDelete2()
"Delete [using D = System.Diagnostics;]@2",
"Delete [using System.Collections;]@33");

edits.VerifyRudeDiagnostics(
Diagnostic(RudeEditKind.Delete, null, CSharpFeaturesResources.using_directive),
Diagnostic(RudeEditKind.Delete, null, CSharpFeaturesResources.using_directive));
edits.VerifyRudeDiagnostics();
}

[Fact]
Expand All @@ -75,9 +73,7 @@ public void UsingInsert()
"Insert [using D = System.Diagnostics;]@2",
"Insert [using System.Collections;]@33");

edits.VerifyRudeDiagnostics(
Diagnostic(RudeEditKind.Insert, "using D = System.Diagnostics;", CSharpFeaturesResources.using_directive),
Diagnostic(RudeEditKind.Insert, "using System.Collections;", CSharpFeaturesResources.using_directive));
edits.VerifyRudeDiagnostics();
}

[Fact]
Expand All @@ -98,8 +94,7 @@ public void UsingUpdate1()
edits.VerifyEdits(
"Update [using System.Collections;]@29 -> [using X = System.Collections;]@29");

edits.VerifyRudeDiagnostics(
Diagnostic(RudeEditKind.Update, "using X = System.Collections;", CSharpFeaturesResources.using_directive));
edits.VerifyRudeDiagnostics();
}

[Fact]
Expand All @@ -120,8 +115,7 @@ public void UsingUpdate2()
edits.VerifyEdits(
"Update [using X1 = System.Collections;]@29 -> [using X2 = System.Collections;]@29");

edits.VerifyRudeDiagnostics(
Diagnostic(RudeEditKind.Update, "using X2 = System.Collections;", CSharpFeaturesResources.using_directive));
edits.VerifyRudeDiagnostics();
}

[Fact]
Expand All @@ -142,8 +136,7 @@ public void UsingUpdate3()
edits.VerifyEdits(
"Update [using System.Diagnostics;]@2 -> [using System;]@2");

edits.VerifyRudeDiagnostics(
Diagnostic(RudeEditKind.Update, "using System;", CSharpFeaturesResources.using_directive));
edits.VerifyRudeDiagnostics();
}

[Fact]
Expand Down Expand Up @@ -218,6 +211,171 @@ namespace N
"Delete [using System.Collections;]@22");
}

[Fact]
public void Using_Delete_ChangesCodeMeaning()
davidwengier marked this conversation as resolved.
Show resolved Hide resolved
{
// This test specifically validates the scenario we _don't_ support, namely when inserting or deleting
// a using directive, if existing code changes in meaning as a result, we don't issue edits for that code.
// If this ever regresses then please buy a lottery ticket because the feature has magically fixed itself.
var src1 = @"
using System.IO;
using DirectoryInfo = N.C;

namespace N
{
public class C
{
public C(string a) { }
public FileAttributes Attributes { get; set; }
}

public class D
{
public void M()
{
var d = new DirectoryInfo(""a"");
var x = directoryInfo.Attributes;
}
}
}";
var src2 = @"
using System.IO;

namespace N
{
public class C
{
public FileAttributes Attributes { get; set; }
}

public class D
{
public void M()
{
var d = new DirectoryInfo(""aa"");
var x = directoryInfo.Attributes;
}
}
}";

var edits = GetTopEdits(src1, src2);

edits.VerifyEdits("Delete [using DirectoryInfo = N.C;]@20");

edits.VerifySemantics();
}

[Fact]
public void Using_Insert_ForNewCode()
{
// As distinct from the above, this test validates a real world scenario of inserting a using directive
// and changing code that utilizes the new directive to some effect.
var src1 = @"
namespace N
{
class Program
{
static void Main(string[] args)
{
}
}
}";
var src2 = @"
using System;

namespace N
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine(""Hello World!"");
}
}
}";

var edits = GetTopEdits(src1, src2);

edits.VerifySemantics(SemanticEdit(SemanticEditKind.Update, c => c.GetMember("N.Program.Main")));
}

[Fact]
public void Using_Delete_ForOldCode()
{
var src1 = @"
using System;

namespace N
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine(""Hello World!"");
}
}
}";
var src2 = @"
namespace N
{
class Program
{
static void Main(string[] args)
{
}
}
}";

var edits = GetTopEdits(src1, src2);

edits.VerifySemantics(SemanticEdit(SemanticEditKind.Update, c => c.GetMember("N.Program.Main")));
}

[Fact]
public void Using_Insert_CreatesAmbiguousCode()
{
// This test validates that we still issue edits for changed valid code, even when unchanged
// code has ambiguities after adding a using.
var src1 = @"
using System.Threading;

namespace N
{
class C
{
void M()
{
// Timer exists in System.Threading and System.Timers
var t = new Timer(s => System.Console.WriteLine(s));
}
}
}";
var src2 = @"
using System.Threading;
using System.Timers;

namespace N
{
class C
{
void M()
{
// Timer exists in System.Threading and System.Timers
var t = new Timer(s => System.Console.WriteLine(s));
}

void M2()
{
// TimersDescriptionAttribute only exists in System.Timers
System.Console.WriteLine(new TimersDescriptionAttribute(""""));
}
}
}";
var edits = GetTopEdits(src1, src2);

edits.VerifySemantics(SemanticEdit(SemanticEditKind.Insert, c => c.GetMember("N.C.M2")));
}

#endregion

#region Extern Alias
Expand Down
24 changes: 18 additions & 6 deletions src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2080,11 +2080,17 @@ private void ClassifyInsert(SyntaxNode node)
return;

case SyntaxKind.ExternAliasDirective:
case SyntaxKind.UsingDirective:
case SyntaxKind.NamespaceDeclaration:
ReportError(RudeEditKind.Insert);
return;

case SyntaxKind.UsingDirective:
// We don't report rude edits for using directives though we also don't do any
// special processing, so inserting a using directive that changes semantics in
// unedited code will not issue edits for that code. Inserting a using directive
// and then consunming it will result in edits for the changes code as per usual.
return;

case SyntaxKind.ClassDeclaration:
case SyntaxKind.StructDeclaration:
case SyntaxKind.InterfaceDeclaration:
Expand Down Expand Up @@ -2149,13 +2155,20 @@ private void ClassifyDelete(SyntaxNode oldNode)
return;

case SyntaxKind.ExternAliasDirective:
case SyntaxKind.UsingDirective:
case SyntaxKind.NamespaceDeclaration:
// To allow removal of declarations we would need to update method bodies that
// were previously binding to them but now are binding to another symbol that was previously hidden.
ReportError(RudeEditKind.Delete);
return;

case SyntaxKind.UsingDirective:
// We don't report rude edits for using directives though we also don't do any
// special processing, so deleting a using directive that changes semantics in
// unedited code will not issue edits for that code. Deleting code and then doing
// something like Remove Unused Usings will report edits for the changes code as
// normal.
return;

case SyntaxKind.DestructorDeclaration:
case SyntaxKind.ConversionOperatorDeclaration:
case SyntaxKind.OperatorDeclaration:
Expand Down Expand Up @@ -2231,15 +2244,14 @@ private void ClassifyUpdate(SyntaxNode oldNode, SyntaxNode newNode)
return;

case SyntaxKind.UsingDirective:
// Dev12 distinguishes using alias from using namespace and reports different errors for removing alias.
// None of these changes are allowed anyways, so let's keep it simple.
ReportError(RudeEditKind.Update);
// We don't report rude edits for using directives though we also don't do any
// special processing, so updating a using directive that changes semantics in
// unedited code will not issue edits for that code.
return;

case SyntaxKind.NamespaceDeclaration:
ClassifyUpdate((NamespaceDeclarationSyntax)oldNode, (NamespaceDeclarationSyntax)newNode);
return;

case SyntaxKind.ClassDeclaration:
case SyntaxKind.StructDeclaration:
case SyntaxKind.InterfaceDeclaration:
Expand Down