No validation for pre-processor symbols in C# Parse Options, and report multiple errors #16920

OmarTawfik · 2017-02-03T07:48:42Z

Fixes #15900 in both C# and VB, by removing validation from ParseOptions, and adding tests to make sure that it doesn't throw exceptions.

Fixes #16913 only in C#, as VB parses input as tokens (not by splitting input), and has special back-compat reasons for keeping this behavior.

Note to self: inspect breaking API change.

…rt multiple errors

CyrusNajmabadi · 2017-02-03T20:35:43Z

src/Compilers/CSharp/Portable/PublicAPI.Shipped.txt

@@ -3263,7 +3263,6 @@ override Microsoft.CodeAnalysis.CSharp.Syntax.XmlTextSyntax.Accept<TResult>(Micr
 override Microsoft.CodeAnalysis.CSharp.Syntax.YieldStatementSyntax.Accept(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor visitor) -> void
 override Microsoft.CodeAnalysis.CSharp.Syntax.YieldStatementSyntax.Accept<TResult>(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor<TResult> visitor) -> TResult
 static Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser.Default.get -> Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser
-static Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser.ParseConditionalCompilationSymbols(string value, out System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostic> diagnostics) -> System.Collections.Generic.IEnumerable<string>


Was this actually shipped? If so, this is a binary breaking change. #Resolved

OmarTawfik · 2017-02-03T20:44:42Z

@CyrusNajmabadi @jaredpar @dotnet/roslyn-compat-council
For the API change here, I'd prefer to undo it if it is going to break existing scenarios.
But looking at CSharpCommandLineParser, it looks like the only other public method there is the Parse(), and all others are:

Private (to be used from the main Parse() method.
Take a diagnostics list that they add errors to.

Is there a reason that this specific ParseConditionalCompilationSymbols was made public and made to have an out parameter? was it by error? is there some external user?
If not, and we're OK taking this change, I'd suggest to do it to keep the public surface aligned. #Resolved

CyrusNajmabadi · 2017-02-03T20:51:05Z

First, let's find out when that API was added. If it was between 2015 and 2017, we can remove it. However, if it's been shipped from before 2017, then we basically can't remove it. Talk to @jaredpar if you want more info on our exceptionally high breaking-change bar. #Resolved

jcouv · 2017-02-06T23:45:25Z

src/Compilers/CSharp/Test/Syntax/Parsing/CSharpParseOptionsTests.cs

-            Assert.Throws<ArgumentException>(() => new CSharpParseOptions(preprocessorSymbols: ImmutableArray.Create<string>("Good", null, @"Bad\Symbol")));
+            var options = new CSharpParseOptions(preprocessorSymbols: new string[] { "valid1", "2invalid" });
+
+            Assert.Equal(2, options.PreprocessorSymbolNames.Count());


I think there is an AssertEx.Equal overload that could be used for this comparison, and the one in the following test. #Resolved

jcouv

LGTM, but the shipped API needs to be restored for back compat.

FYI @jaredpar @gafter regarding addition of new public API. My take is that we should not add the new one (only keep the old one, even though it is clunkier).

OmarTawfik · 2017-02-07T21:38:55Z

API changes reverted, per general guidance from team.
@dotnet/roslyn-compiler can I get a second review? #Resolved

jcouv · 2017-02-07T21:45:22Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

@@ -1470,7 +1470,7 @@ private ImmutableArray<string> BuildSearchPaths(string sdkDirectoryOpt, List<str

        public static IEnumerable<string> ParseConditionalCompilationSymbols(string value, out IEnumerable<Diagnostic> diagnostics)
        {
-            Diagnostic myDiagnostic = null;
+            List<Diagnostic> myDiagnostics = new List<Diagnostic>();


Nit: outputDiagnostics or resultDiagnostics maybe? #Resolved

jcouv

LGTM

AlekseyTs · 2017-02-08T21:03:08Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

@@ -1470,7 +1470,7 @@ private ImmutableArray<string> BuildSearchPaths(string sdkDirectoryOpt, List<str

        public static IEnumerable<string> ParseConditionalCompilationSymbols(string value, out IEnumerable<Diagnostic> diagnostics)
        {
-            Diagnostic myDiagnostic = null;
+            List<Diagnostic> myDiagnostics = new List<Diagnostic>();


List myDiagnostics = new List(); [](start = 12, length = 56)

Can we use a DiagnosticBag instead (probably from the pool)? #Resolved

Or an ArrayBuilder? We usually don't use List of T.

In reply to: 100169189 [](ancestors = 100169189)

AlekseyTs

It doesn't look like we are validating pre-processor symbols in parse options. I thought the plan was to move validation elsewhere.

AlekseyTs · 2017-02-08T21:25:00Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

@@ -8101,6 +8101,31 @@ End Module
            Assert.Equal($"error BC2012: can't open '{sourceLinkPath}' for writing: Fake IOException{Environment.NewLine}", outWriter.ToString())
        End Sub

+        <WorkItem(15900, "https://github.com/dotnet/roslyn/issues/15900")>
+        <Fact>
+        Public Sub CompilingCodeWithInvalidPreProcessorSymbolsShouldErrorOut()


CompilingCodeWithInvalidPreProcessorSymbolsShouldErrorOut [](start = 19, length = 57)

Could you include how the errors look like as text, as comments perhaps? #Resolved

OmarTawfik · 2017-02-08T22:31:19Z

It doesn't look like we are validating pre-processor symbols in parse options. I thought the plan was to move validation elsewhere.

~~@jaredpar can you please check @AlekseyTs's comment here per #15900 description?~~
Edit: Validation is done during compilation (not when creating API objects). We need to add it through compilation API as well.

OmarTawfik · 2017-02-09T00:37:34Z

Based on discussion with AlekseyTs, there are surfaces that are not covered by this change.
For example SyntaxFactory.ParseTokens(). Closing for now.

OmarTawfik added Area-Compilers Bug labels Feb 3, 2017

OmarTawfik added this to the 2.1 milestone Feb 3, 2017

dnfclas added the cla-already-signed label Feb 3, 2017

Omar Tawfik added 2 commits February 3, 2017 01:09

No validation for pre-processor symbols in C# Parse Options, and repo…

51a5d34

…rt multiple errors

Fixed tests

304da67

CyrusNajmabadi reviewed Feb 3, 2017

View reviewed changes

jcouv reviewed Feb 6, 2017

View reviewed changes

jcouv approved these changes Feb 6, 2017

View reviewed changes

Addressed CR comments

e280cfe

jcouv reviewed Feb 7, 2017

View reviewed changes

jcouv approved these changes Feb 7, 2017

View reviewed changes

AlekseyTs reviewed Feb 8, 2017

View reviewed changes

AlekseyTs requested changes Feb 8, 2017

View reviewed changes

AlekseyTs reviewed Feb 8, 2017

View reviewed changes

OmarTawfik closed this Feb 9, 2017

OmarTawfik mentioned this pull request Feb 10, 2017

Report all invalid preprocessor symbols in diagnostics #17050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No validation for pre-processor symbols in C# Parse Options, and report multiple errors #16920

No validation for pre-processor symbols in C# Parse Options, and report multiple errors #16920

OmarTawfik commented Feb 3, 2017 •

edited

Loading

CyrusNajmabadi Feb 3, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Feb 3, 2017 •

edited

Loading

CyrusNajmabadi commented Feb 3, 2017 •

edited by OmarTawfik

Loading

jcouv Feb 6, 2017 •

edited by OmarTawfik

Loading

jcouv left a comment

OmarTawfik commented Feb 7, 2017 •

edited

Loading

jcouv Feb 7, 2017 •

edited by OmarTawfik

Loading

jcouv left a comment

AlekseyTs Feb 8, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Feb 8, 2017

AlekseyTs left a comment

AlekseyTs Feb 8, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Feb 8, 2017 •

edited

Loading

OmarTawfik commented Feb 9, 2017

No validation for pre-processor symbols in C# Parse Options, and report multiple errors #16920

No validation for pre-processor symbols in C# Parse Options, and report multiple errors #16920

Conversation

OmarTawfik commented Feb 3, 2017 • edited Loading

CyrusNajmabadi Feb 3, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik commented Feb 3, 2017 • edited Loading

CyrusNajmabadi commented Feb 3, 2017 • edited by OmarTawfik Loading

jcouv Feb 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

OmarTawfik commented Feb 7, 2017 • edited Loading

jcouv Feb 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Feb 8, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs Feb 8, 2017

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs Feb 8, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik commented Feb 8, 2017 • edited Loading

OmarTawfik commented Feb 9, 2017

OmarTawfik commented Feb 3, 2017 •

edited

Loading

CyrusNajmabadi Feb 3, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Feb 3, 2017 •

edited

Loading

CyrusNajmabadi commented Feb 3, 2017 •

edited by OmarTawfik

Loading

jcouv Feb 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Feb 7, 2017 •

edited

Loading

jcouv Feb 7, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Feb 8, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Feb 8, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Feb 8, 2017 •

edited

Loading