diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs index 57df04e568..aa59d96a64 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs @@ -128,6 +128,9 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring // Don't allow moving the declaration into the resource aquisition of a using statement if (parent is UsingStatement) return true; + // Don't allow moving things into arbitrary positions of for statements + if (parent is ForStatement && anchorNode.Role != Roles.EmbeddedStatement) + return true; return false; } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableHidesMemberIssue/VariableHidesMemberIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableHidesMemberIssue/VariableHidesMemberIssue.cs index 6b1dffdf6c..5fb0de1215 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableHidesMemberIssue/VariableHidesMemberIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableHidesMemberIssue/VariableHidesMemberIssue.cs @@ -23,36 +23,83 @@ // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. - using System.Collections.Generic; using System.Linq; using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; +using System; namespace ICSharpCode.NRefactory.CSharp.Refactoring { public abstract class VariableHidesMemberIssue : ICodeIssueProvider { - public IEnumerable GetIssues (BaseRefactoringContext context) + public IEnumerable GetIssues(BaseRefactoringContext context) { - return GetGatherVisitor (context).GetIssues (); + return GetGatherVisitor(context).GetIssues(); } protected static bool HidesMember(BaseRefactoringContext ctx, AstNode node, string variableName) { - var typeDecl = node.GetParent (); + var typeDecl = node.GetParent(); if (typeDecl == null) return false; - var typeResolveResult = ctx.Resolve (typeDecl) as TypeResolveResult; + var entityDecl = node.GetParent(); + var memberResolveResult = ctx.Resolve(entityDecl) as MemberResolveResult; + if (memberResolveResult == null) + return false; + var typeResolveResult = ctx.Resolve(typeDecl) as TypeResolveResult; if (typeResolveResult == null) return false; - var entityDecl = node.GetParent (); - var isStatic = (entityDecl.Modifiers & Modifiers.Static) == Modifiers.Static; + var sourceMember = memberResolveResult.Member; + + return typeResolveResult.Type.GetMembers(m => m.Name == variableName).Any(m2 => IsAccessible(sourceMember, m2)); + } + + static bool IsAccessible(IMember sourceMember, IMember targetMember) + { + if (sourceMember.IsStatic != targetMember.IsStatic) + return false; - return typeResolveResult.Type.GetMembers (m => m.Name == variableName && m.IsStatic == isStatic).Any (); + var sourceType = sourceMember.DeclaringType; + var targetType = targetMember.DeclaringType; + switch (targetMember.Accessibility) { + case Accessibility.None: + return false; + case Accessibility.Private: + // check for members of outer classes (private members of outer classes can be accessed) + var targetTypeDefinition = targetType.GetDefinition(); + for (var t = sourceType.GetDefinition(); t != null; t = t.DeclaringTypeDefinition) { + if (t.Equals(targetTypeDefinition)) + return true; + } + return false; + case Accessibility.Public: + return true; + case Accessibility.Protected: + return IsProtectedAccessible(sourceType, targetType); + case Accessibility.Internal: + return IsInternalAccessible(sourceMember.ParentAssembly, targetMember.ParentAssembly); + case Accessibility.ProtectedOrInternal: + return IsInternalAccessible(sourceMember.ParentAssembly, targetMember.ParentAssembly) || IsProtectedAccessible(sourceType, targetType); + case Accessibility.ProtectedAndInternal: + return IsInternalAccessible(sourceMember.ParentAssembly, targetMember.ParentAssembly) && IsProtectedAccessible(sourceType, targetType); + default: + throw new Exception("Invalid value for Accessibility"); + } + } + + static bool IsProtectedAccessible(IType sourceType, IType targetType) + { + return sourceType.GetAllBaseTypes().Any(type => targetType.Equals(type)); + } + + static bool IsInternalAccessible(IAssembly sourceAssembly, IAssembly targetAssembly) + { + return sourceAssembly.InternalsVisibleTo(targetAssembly); } - internal abstract GatherVisitorBase GetGatherVisitor (BaseRefactoringContext context); + internal abstract GatherVisitorBase GetGatherVisitor(BaseRefactoringContext context); } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableHidesMemberIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableHidesMemberIssueTests.cs index 41b118eedf..cbdf040a6f 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableHidesMemberIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableHidesMemberIssueTests.cs @@ -108,6 +108,87 @@ class TestClass { int j; } +}"; + Test (input, 0); + } + + [Test] + public void TestAccessiblePrivate () + { + var input = @" +class TestClass +{ + int i; + + void Method () + { + int i = 0; + } +}"; + Test (input, 1); + } + + [Test] + public void TestAccessiblePrivateDueToTypeNesting () + { + var input = @" +class RootClass +{ + int i; + + class NestedClass : RootClass + { + // Issue 1 + void Method () + { + int i = 0; + } + + class NestedNestedClass : NestedClass + { + // Issue 2 + void OtherMethod () + { + int i = 0; + } + } + } +}"; + Test (input, 2); + } + + [Test] + public void TestInternalAccessibility () + { + var input = @" +class BaseClass +{ + internal int i; +} +class TestClass : BaseClass +{ + void Method () + { + int i = 0; + } +}"; + Test (input, 1); + } + + [Test] + public void TestInaccessiblePrivate () + { + var input = @" +class BaseClass +{ + int i; +} +class TestClass : BaseClass +{ + void Method () + { + int i = 0; + } }"; Test (input, 0); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterHidesMemberIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterHidesMemberIssueTests.cs index 40b62739e5..982182d02f 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterHidesMemberIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterHidesMemberIssueTests.cs @@ -88,7 +88,7 @@ class TestClass }"; Test (input, 1); } - + [Test] public void TestStaticNoIssue () { @@ -103,6 +103,78 @@ class TestClass static void TestMethod2 (int j) { } +}"; + Test (input, 0); + } + + [Test] + public void TestAccessiblePrivate () + { + var input = @" +class TestClass +{ + int i; + + void Method (int i) + { + } +}"; + Test (input, 1); + } + + [Test] + public void TestAccessiblePrivateDueToTypeNesting () + { + var input = @" +class RootClass +{ + int i; + + class NestedClass : RootClass + { + // Issue 1 + void Method (int i) {} + + class NestedNestedClass : NestedClass + { + // Issue 2 + void OtherMethod (int i) {} + } + } +}"; + Test (input, 2); + } + + [Test] + public void TestInternalAccessibility () + { + var input = @" +class BaseClass +{ + internal int i; +} +class TestClass : BaseClass +{ + void Method (int i) + { + } +}"; + Test (input, 1); + } + + [Test] + public void TestInaccessiblePrivate () + { + var input = @" +class BaseClass +{ + private int i; +} +class TestClass : BaseClass +{ + void Method (int i) + { + } }"; Test (input, 0); } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs index cfa1c737e2..be30980f8c 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs @@ -391,6 +391,24 @@ class A ", 0); } + [Test] + public void DoesNotExpandForStatement () + { + TestStatements(@" + var val = GetValue (); + if (false) { + for (var i = GetValue (); ; i++) + System.Console.WriteLine (val); + } +", 1, @" + if (false) { + var val = GetValue (); + for (var i = GetValue (); ; i++) + System.Console.WriteLine (val); + } +"); + } + [Test] public void DoesNotInsertBlockStatementInResourceAquisition () { diff --git a/ICSharpCode.NRefactory.Tests/Utils/CompositeFormatStringParser/CompositeFormatStringParserTests.cs b/ICSharpCode.NRefactory.Tests/Utils/CompositeFormatStringParser/CompositeFormatStringParserTests.cs index 972b2c99df..9606cc8f80 100644 --- a/ICSharpCode.NRefactory.Tests/Utils/CompositeFormatStringParser/CompositeFormatStringParserTests.cs +++ b/ICSharpCode.NRefactory.Tests/Utils/CompositeFormatStringParser/CompositeFormatStringParserTests.cs @@ -37,15 +37,16 @@ namespace ICSharpCode.NRefactory.Utils IList ParseTest(string format, params IFormatStringSegment[] expectedFormatSegments) { var parser = new CompositeFormatStringParser(); - var actualFormatSegments = parser.Parse(format).Segments; + var formatStringParseResult = parser.Parse(format); + var actualFormatSegments = formatStringParseResult.Segments; Console.WriteLine("Expected format segments:"); foreach (var item in expectedFormatSegments) { - Console.WriteLine(item.ToString()); + Console.WriteLine(item); } Console.WriteLine("Actual format segments:"); foreach (var item in actualFormatSegments) { - Console.WriteLine(item.ToString()); + Console.WriteLine(item); foreach (var error in item.Errors) { Console.WriteLine("\t{0}", error); } @@ -117,7 +118,8 @@ namespace ICSharpCode.NRefactory.Utils [Test] public void BraceEscape() { - ParseTest("{{}}", new TextSegment("{}")); + var segments = ParseTest("{{}}", new TextSegment("{}")); + Assert.IsFalse(segments.First().HasErrors); } [Test] diff --git a/ICSharpCode.NRefactory/Utils/CompositeFormatStringParser/CompositeFormatStringParser.cs b/ICSharpCode.NRefactory/Utils/CompositeFormatStringParser/CompositeFormatStringParser.cs index 3289187994..ad3f6abdcd 100644 --- a/ICSharpCode.NRefactory/Utils/CompositeFormatStringParser/CompositeFormatStringParser.cs +++ b/ICSharpCode.NRefactory/Utils/CompositeFormatStringParser/CompositeFormatStringParser.cs @@ -67,8 +67,8 @@ namespace ICSharpCode.NRefactory.Utils if (i < format.Length && format [i] == '{') { int formatItemStart = i; int index; - int? alignment = null; - string argumentFormat = null; + int? alignment; + string argumentFormat; var textSegmentErrors = new List(GetErrors()); // Try to parse the parts of the format item @@ -83,26 +83,30 @@ namespace ICSharpCode.NRefactory.Utils CheckForMissingEndBrace (format, i, length); // Check what we parsed - if (i == formatItemStart + 1 && (i == length || (i < length && format [i] != '}'))) { + if (i == formatItemStart + 1 && (i == length || (i < length && format[i] != '}'))) { // There were no format item after all, this was just an - // unescaped left brace + // unescaped left brace, or the initial brace of an escape sequence SetErrors(textSegmentErrors); - AddError (new DefaultFormatStringError { - Message = "Unescaped '{'", - StartLocation = formatItemStart, - EndLocation = formatItemStart + 1, - OriginalText = "{", - SuggestedReplacementText = "{{" - }); + if (i >= length || format[i] != '{') { + AddError (new DefaultFormatStringError { + Message = "Unescaped '{'", + StartLocation = formatItemStart, + EndLocation = formatItemStart + 1, + OriginalText = "{", + SuggestedReplacementText = "{{" + }); + } continue; - } else if (formatItemStart - textStart > 0) { + } + + if (formatItemStart - textStart > 0) { // We have parsed a format item, end the text segment var textSegment = new TextSegment (UnEscape (format.Substring (textStart, formatItemStart - textStart))); textSegment.Errors = textSegmentErrors; result.Segments.Add (textSegment); } - // Unclosed format items in fixed text gets advances i one step too far + // Unclosed format items in fixed text advances i one step too far if (i < length && format [i] != '}') --i; @@ -242,20 +246,22 @@ namespace ICSharpCode.NRefactory.Utils parsedCharacters = numberText.Length; int numberLength = 0; int? number = GetNumber (numberText, ref numberLength); - var endingChar = index + numberLength; if (numberLength != parsedCharacters && fieldEnd < format.Length && delimiters.Contains (format [fieldEnd])) { // Not the entire number field could be parsed // The field actually ended as intended, so set the index to the end of the field index = fieldEnd; var suggestedNumber = (number ?? 0).ToString (); AddInvalidNumberFormatError (numberFieldStart, format.Substring (numberFieldStart, index - numberFieldStart), suggestedNumber); - } else if (numberLength != parsedCharacters) { - // Not the entire number field could be parsed - // The field didn't end, it was cut off so we are missing an ending brace - index = endingChar; - AddMissingEndBraceError (index, index, "Missing ending '}'", ""); } else { - index = endingChar; + var endingChar = index + numberLength; + if (numberLength != parsedCharacters) { + // Not the entire number field could be parsed + // The field didn't end, it was cut off so we are missing an ending brace + index = endingChar; + AddMissingEndBraceError (index, index, "Missing ending '}'", ""); + } else { + index = endingChar; + } } return number; }