Merge pull request #124 from riviti/master

Fixes for a couple of issues

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;
 			}
 

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<CodeIssue> GetIssues (BaseRefactoringContext context)
+		public IEnumerable<CodeIssue> 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<TypeDeclaration> ();
+			var typeDecl = node.GetParent<TypeDeclaration>();
 			if (typeDecl == null)
 				return false;
-			var typeResolveResult = ctx.Resolve (typeDecl) as TypeResolveResult;
+			var entityDecl = node.GetParent<EntityDeclaration>();
+			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<EntityDeclaration> ();
+			var sourceMember = memberResolveResult.Member;
-			var isStatic = (entityDecl.Modifiers & Modifiers.Static) == Modifiers.Static;
+
+			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);
 
 	}
 }

ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/LocalVariableHidesMemberIssueTests.cs

@@ -108,6 +108,87 @@ class TestClass
 	{
 		int j;
 	}
+}";
+			Test<LocalVariableHidesMemberIssue> (input, 0);
+		}
+		
+		[Test]
+		public void TestAccessiblePrivate ()
+		{
+			var input = @"
+class TestClass
+{
+	int i;
+
+	void Method ()
+	{
+		int i = 0;
+	}
+}";
+			Test<LocalVariableHidesMemberIssue> (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<LocalVariableHidesMemberIssue> (input, 2);
+		}
+		
+		[Test]
+		public void TestInternalAccessibility ()
+		{
+			var input = @"
+class BaseClass
+{
+	internal int i;
+}
+class TestClass : BaseClass
+{
+	void Method ()
+	{
+		int i = 0;
+	}
+}";
+			Test<LocalVariableHidesMemberIssue> (input, 1);
+		}
+		
+		[Test]
+		public void TestInaccessiblePrivate ()
+		{
+			var input = @"
+class BaseClass
+{
+	int i;
+}
+class TestClass : BaseClass
+{
+	void Method ()
+	{
+		int i = 0;
+	}
 }";
 			Test<LocalVariableHidesMemberIssue> (input, 0);
 		}

ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterHidesMemberIssueTests.cs

@@ -88,7 +88,7 @@ class TestClass
 }";
 			Test<ParameterHidesMemberIssue> (input, 1);
 		}
-
+		
 		[Test]
 		public void TestStaticNoIssue ()
 		{
@@ -103,6 +103,78 @@ class TestClass
 	static void TestMethod2 (int j)
 	{
 	}
+}";
+			Test<ParameterHidesMemberIssue> (input, 0);
+		}
+		
+		[Test]
+		public void TestAccessiblePrivate ()
+		{
+			var input = @"
+class TestClass
+{
+	int i;
+
+	void Method (int i)
+	{
+	}
+}";
+			Test<ParameterHidesMemberIssue> (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<ParameterHidesMemberIssue> (input, 2);
+		}
+		
+		[Test]
+		public void TestInternalAccessibility ()
+		{
+			var input = @"
+class BaseClass
+{
+	internal int i;
+}
+class TestClass : BaseClass
+{
+	void Method (int i)
+	{
+	}
+}";
+			Test<ParameterHidesMemberIssue> (input, 1);
+		}
+		
+		[Test]
+		public void TestInaccessiblePrivate ()
+		{
+			var input = @"
+class BaseClass
+{
+	private int i;
+}
+class TestClass : BaseClass
+{
+	void Method (int i)
+	{
+	}
 }";
 			Test<ParameterHidesMemberIssue> (input, 0);
 		}

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 ()
 		{	

ICSharpCode.NRefactory.Tests/Utils/CompositeFormatStringParser/CompositeFormatStringParserTests.cs

@@ -37,15 +37,16 @@ namespace ICSharpCode.NRefactory.Utils
 		IList<IFormatStringSegment> 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]

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;
+					int? alignment;
-					string argumentFormat = null;
+					string argumentFormat;
 					var textSegmentErrors = new List<IFormatStringError>(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 {
+						if (i >= length || format[i] != '{') {
-							Message = "Unescaped '{'",
+							AddError (new DefaultFormatStringError {
-							StartLocation = formatItemStart,
+								Message = "Unescaped '{'",
-							EndLocation = formatItemStart + 1,
+								StartLocation = formatItemStart,
-							OriginalText = "{",
+								EndLocation = formatItemStart + 1,
-							SuggestedReplacementText = "{{"
+								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;
 		}