Improve error messages for why a solution failed to load (#5176)

* WIP Improve sln reader/writer error messages

* Added more tests

* Fix a few tests

src/Microsoft.DotNet.Cli.Sln.Internal/LocalizableStrings.cs

@@ -2,6 +2,14 @@
 {
     internal class LocalizableStrings
     {
+        // {0} is the line number
+        // {1} is the error message details
+        public const string ErrorMessageFormatString = "Invalid format in line {0}: {1}";
+
+        public const string ProjectParsingErrorFormatString = "Project section is missing '{0}' when parsing the line starting at position {1}";
+
+        public const string InvalidPropertySetFormatString = "Property set is missing '{0}'";
+
         public const string GlobalSectionMoreThanOnceError = "Global section specified more than once";
 
         public const string GlobalSectionNotClosedError = "Global section not closed";

src/Microsoft.DotNet.Cli.Sln.Internal/SlnFile.cs

@@ -120,6 +120,8 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
 
         private void Read(TextReader reader)
         {
+            const string HeaderPrefix = "Microsoft Visual Studio Solution File, Format Version ";
+
             string line;
             int curLineNum = 0;
             bool globalFound = false;
@@ -129,14 +131,16 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
             {
                 curLineNum++;
                 line = line.Trim();
-                if (line.StartsWith("Microsoft Visual Studio Solution File", StringComparison.Ordinal))
+                if (line.StartsWith(HeaderPrefix, StringComparison.Ordinal))
                 {
-                    int i = line.LastIndexOf(' ');
+                    if (line.Length <= HeaderPrefix.Length)
-                    if (i == -1)
                     {
-                        throw new InvalidSolutionFormatException(curLineNum);
+                        throw new InvalidSolutionFormatException(
+                            curLineNum,
+                            LocalizableStrings.FileHeaderMissingError);
                     }
-                    FormatVersion = line.Substring(i + 1);
+
+                    FormatVersion = line.Substring(HeaderPrefix.Length);
                     _prefixBlankLines = curLineNum - 1;
                 }
                 if (line.StartsWith("# ", StringComparison.Ordinal))
@@ -157,7 +161,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
                 {
                     if (globalFound)
                     {
-                        throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.GlobalSectionMoreThanOnceError);
+                        throw new InvalidSolutionFormatException(
+                            curLineNum,
+                            LocalizableStrings.GlobalSectionMoreThanOnceError);
                     }
                     globalFound = true;
                     while ((line = reader.ReadLine()) != null)
@@ -181,7 +187,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
                     }
                     if (line == null)
                     {
-                        throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.GlobalSectionNotClosedError);
+                        throw new InvalidSolutionFormatException(
+                            curLineNum,
+                            LocalizableStrings.GlobalSectionNotClosedError);
                     }
                 }
                 else if (line.IndexOf('=') != -1)
@@ -191,7 +199,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
             }
             if (FormatVersion == null)
             {
-                throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.FileHeaderMissingError);
+                throw new InvalidSolutionFormatException(
+                    curLineNum,
+                    LocalizableStrings.FileHeaderMissingError);
             }
         }
 
@@ -331,15 +341,20 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
                 }
             }
 
-            throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.ProjectSectionNotClosedError);
+            throw new InvalidSolutionFormatException(
+                curLineNum,
+                LocalizableStrings.ProjectSectionNotClosedError);
         }
 
         private void FindNext(int ln, string line, ref int i, char c)
         {
+            var inputIndex = i;
             i = line.IndexOf(c, i);
             if (i == -1)
             {
-                throw new InvalidSolutionFormatException(ln);
+                throw new InvalidSolutionFormatException(
+                    ln,
+                    string.Format(LocalizableStrings.ProjectParsingErrorFormatString, c, inputIndex));
             }
         }
 
@@ -481,7 +496,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
             {
                 return SlnSectionType.PostProcess;
             }
-            throw new InvalidSolutionFormatException(curLineNum, String.Format(LocalizableStrings.InvalidSectionTypeError, s));
+            throw new InvalidSolutionFormatException(
+                curLineNum, 
+                String.Format(LocalizableStrings.InvalidSectionTypeError, s));
         }
 
         private string FromSectionType(bool isProjectSection, SlnSectionType type)
@@ -502,13 +519,17 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
             int k = line.IndexOf('(');
             if (k == -1)
             {
-                throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.SectionIdMissingError);
+                throw new InvalidSolutionFormatException(
+                    curLineNum,
+                    LocalizableStrings.SectionIdMissingError);
             }
             var tag = line.Substring(0, k).Trim();
             var k2 = line.IndexOf(')', k);
             if (k2 == -1)
             {
-                throw new InvalidSolutionFormatException(curLineNum);
+                throw new InvalidSolutionFormatException(
+                    curLineNum,
+                    LocalizableStrings.SectionIdMissingError);
             }
             Id = line.Substring(k + 1, k2 - k - 1);
 
@@ -531,7 +552,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
             }
             if (line == null)
             {
-                throw new InvalidSolutionFormatException(curLineNum, LocalizableStrings.ClosingSectionTagNotFoundError);
+                throw new InvalidSolutionFormatException(
+                    curLineNum,
+                    LocalizableStrings.ClosingSectionTagNotFoundError);
             }
         }
 
@@ -550,7 +573,9 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
                     var i = line.IndexOf('.');
                     if (i == -1)
                     {
-                        throw new InvalidSolutionFormatException(_baseIndex + n);
+                        throw new InvalidSolutionFormatException(
+                            _baseIndex + n,
+                            string.Format(LocalizableStrings.InvalidPropertySetFormatString, '.'));
                     }
                     var id = line.Substring(0, i);
                     if (curSet == null || id != curSet.Id)
@@ -1141,12 +1166,8 @@ namespace Microsoft.DotNet.Cli.Sln.Internal
 
     public class InvalidSolutionFormatException : Exception
     {
-        public InvalidSolutionFormatException(int line) : base("Invalid format in line " + line)
+        public InvalidSolutionFormatException(int line, string details)
-        {
+            : base(string.Format(LocalizableStrings.ErrorMessageFormatString, line, details))
-        }
-
-        public InvalidSolutionFormatException(int line, string msg) 
-            : base("Invalid format in line " + line + ": " + msg)
         {
         }
     }

src/dotnet/CommonLocalizableStrings.cs

@@ -105,7 +105,7 @@ namespace Microsoft.DotNet.Tools
         public const string CouldNotFindSolutionIn = "Specified solution file {0} does not exist, or there is no solution file in the directory.";
         public const string CouldNotFindSolutionOrDirectory = "Could not find solution or directory `{0}`.";
         public const string MoreThanOneSolutionInDirectory = "Found more than one solution file in {0}. Please specify which one to use.";
-        public const string InvalidSolution = "Invalid solution `{0}`.";
+        public const string InvalidSolutionFormatString = "Invalid solution `{0}`. {1}"; // {0} is the solution path, {1} is already localized details on the failure
         public const string SolutionDoesNotExist = "Specified solution file {0} does not exist, or there is no solution file in the directory.";
         
         /// add p2p

src/dotnet/SlnFileFactory.cs

@@ -30,9 +30,12 @@ namespace Microsoft.DotNet.Tools.Common
             {
                 slnFile = SlnFile.Read(solutionPath);
             }
-            catch
+            catch (InvalidSolutionFormatException e)
             {
-                throw new GracefulException(CommonLocalizableStrings.InvalidSolution, solutionPath);
+                throw new GracefulException(
+                    CommonLocalizableStrings.InvalidSolutionFormatString,
+                    solutionPath,
+                    e.Message);
             }
             return slnFile;
         }

test/Microsoft.DotNet.Cli.Sln.Internal.Tests/Microsoft.DotNet.Cli.Sln.Internal.Tests.cs

@@ -276,11 +276,13 @@ EndGlobal
                 .Should().Be(SolutionModified);
         }
 
-        [Fact]
+        [Theory]
-        public void WhenGivenAnSolutionWithMissingHeaderItThrows()
+        [InlineData("Invalid Solution")]
+        [InlineData("Microsoft Visual Studio Solution File, Format Version ")]
+        public void WhenGivenASolutionWithMissingHeaderItThrows(string fileContents)
         {
             var tmpFile = Temp.CreateFile();
-            tmpFile.WriteAllText("Invalid Solution");
+            tmpFile.WriteAllText(fileContents);
 
             Action action = () =>
             {
@@ -290,5 +292,180 @@ EndGlobal
             action.ShouldThrow<InvalidSolutionFormatException>()
                 .WithMessage("Invalid format in line 1: File header is missing");
         }
+
+        [Fact]
+        public void WhenGivenASolutionWithMultipleGlobalSectionsItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Global
+EndGlobal
+Global
+EndGlobal
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 5: Global section specified more than once");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithGlobalSectionNotClosedItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Global
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 3: Global section not closed");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithProjectSectionNotClosedItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Project(""{9A19103F-16F7-4668-BE54-9A1E7A4F7556}"") = ""App"", ""App\App.csproj"", ""{7072A694-548F-4CAE-A58F-12D257D5F486}""
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 3: Project section not closed");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithInvalidProjectSectionItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Project""{9A19103F-16F7-4668-BE54-9A1E7A4F7556}"") = ""App"", ""App\App.csproj"", ""{7072A694-548F-4CAE-A58F-12D257D5F486}""
+EndProject
+";
+
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 3: Project section is missing '(' when parsing the line starting at position 0");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithInvalidSectionTypeItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Global
+	GlobalSection(SolutionConfigurationPlatforms) = thisIsUnknown
+	EndGlobalSection
+EndGlobal
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 4: Invalid section type: thisIsUnknown");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithMissingSectionIdTypeItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Global
+	GlobalSection = preSolution
+	EndGlobalSection
+EndGlobal
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 4: Section id missing");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithSectionNotClosedItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Global
+	GlobalSection(SolutionConfigurationPlatforms) = preSolution
+EndGlobal
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                SlnFile.Read(tmpFile.Path);
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 6: Closing section tag not found");
+        }
+
+        [Fact]
+        public void WhenGivenASolutionWithInvalidPropertySetItThrows()
+        {
+            const string SolutionFile = @"
+Microsoft Visual Studio Solution File, Format Version 12.00
+Project(""{7072A694-548F-4CAE-A58F-12D257D5F486}"") = ""AppModified"", ""AppModified\AppModified.csproj"", ""{9A19103F-16F7-4668-BE54-9A1E7A4F7556}""
+EndProject
+Global
+	GlobalSection(ProjectConfigurationPlatforms) = postSolution
+		{7072A694-548F-4CAE-A58F-12D257D5F486} Debug|Any CPU ActiveCfg = Debug|Any CPU
+	EndGlobalSection
+EndGlobal
+";
+            var tmpFile = Temp.CreateFile();
+            tmpFile.WriteAllText(SolutionFile);
+
+            Action action = () =>
+            {
+                var slnFile = SlnFile.Read(tmpFile.Path);
+                if (slnFile.ProjectConfigurationsSection.Count == 0)
+                {
+                    // Need to force loading of nested property sets
+                }
+            };
+
+            action.ShouldThrow<InvalidSolutionFormatException>()
+                .WithMessage("Invalid format in line 7: Property set is missing '.'");
+        }
     }
 }

test/dotnet-add-proj.Tests/GivenDotnetAddProj.cs

@@ -90,7 +90,7 @@ Additional Arguments:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput($"add InvalidSolution.sln project {projectToAdd}");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`.");
+            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }
 
@@ -110,7 +110,7 @@ Additional Arguments:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput($"add project {projectToAdd}");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be($"Invalid solution `{solutionPath}`.");
+            cmd.StdErr.Should().Be($"Invalid solution `{solutionPath}`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }
 

test/dotnet-list-proj.Tests/GivenDotnetListProj.cs

@@ -83,7 +83,7 @@ Options:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput("list InvalidSolution.sln projects");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`.");
+            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }
 
@@ -102,7 +102,7 @@ Options:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput("list projects");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be($"Invalid solution `{solutionFullPath}`.");
+            cmd.StdErr.Should().Be($"Invalid solution `{solutionFullPath}`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }
 

test/dotnet-remove-proj.Tests/GivenDotnetRemoveProj.cs

@@ -88,7 +88,7 @@ Additional Arguments:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput($"remove InvalidSolution.sln project {projectToRemove}");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`.");
+            cmd.StdErr.Should().Be("Invalid solution `InvalidSolution.sln`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }
 
@@ -108,7 +108,7 @@ Additional Arguments:
                 .WithWorkingDirectory(projectDirectory)
                 .ExecuteWithCapturedOutput($"remove project {projectToRemove}");
             cmd.Should().Fail();
-            cmd.StdErr.Should().Be($"Invalid solution `{solutionPath}`.");
+            cmd.StdErr.Should().Be($"Invalid solution `{solutionPath}`. Invalid format in line 1: File header is missing");
             cmd.StdOut.Should().BeVisuallyEquivalentTo(HelpText);
         }