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

Harden ParseEmbeddedStatement #13924

Merged
merged 1 commit into from Sep 20, 2016
Merged

Harden ParseEmbeddedStatement #13924

merged 1 commit into from Sep 20, 2016

Conversation

jaredpar
Copy link
Member

The consumers of ParseEmbeddedStatement depend on it having a non-null return. Yet it directly returns the result of ParseStatementCore which can validly return null values in error conditions. In the case that does happen return an empty statement with a diagnostic.

@jaredpar
Copy link
Member Author

https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=266237&triage=true

@jaredpar
Copy link
Member Author

CC @dotnet/roslyn-compiler

cston
cston reviewed Sep 20, 2016
View reviewed changes
src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs
@@ -2623,6 +2623,40 @@ static void Test(int arg1, (byte, byte) arg2)
Assert.Equal(false, tree.GetRoot().ContainsDiagnostics);
}

[Fact]
[WorkItem(684860, "https://devdiv.visualstudio.com/DevDiv/_workitems/edit/266237")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WorkItem(266237, ...)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh 😦

@cston
Copy link
Member

cston commented Sep 20, 2016

LGTM

@cston cston closed this Sep 20, 2016
@jaredpar jaredpar reopened this Sep 20, 2016
@jaredpar
Harden ParseEmbeddedStatement
d689f37 
The consumers of ParseEmbeddedStatement depend on it having a non-null return.  Yet it directly returns the result of ParseStatementCore which can validly return null values in error conditions.  In the case that does happen return an empty statement with a diagnostic.
@jaredpar jaredpar closed this Sep 20, 2016
@jaredpar jaredpar reopened this Sep 20, 2016
@AlekseyTs
Copy link
Contributor

LGTM

1 similar comment
@VSadov
Copy link
Member

VSadov commented Sep 20, 2016

LGTM

@jaredpar jaredpar merged commit da74d1b into dotnet:master Sep 20, 2016
@jaredpar jaredpar deleted the watson branch September 20, 2016 20:29
@gafter gafter mentioned this pull request Sep 26, 2016
Closed
@gafter
Copy link
Member

gafter commented Sep 26, 2016

https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=266237&triage=true
summarizes a number of "related" issues. One call stack is recorded in it, but that call stack doesn't appear likely to be fixed by this PR

Here is that callstack:

1. Microsoft_CodeAnalysis!Microsoft.CodeAnalysis.GreenNode.AdjustFlagsAndWidth
2.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.ArgumentListSyntax..ctor
3.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.ContextAwareSyntax.ArgumentList
4.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseParenthesizedArgumentList
5.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParsePostFixExpression
6.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseSubExpressionCore
7.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseSubExpression
8.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseElementInitializer
9.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseVariableDeclarator
10.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseVariableDeclarators
11.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseDeclaration
12.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseLocalDeclarationStatement
13.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParsePossibleBadAwaitStatement
14.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParsePossibleBadAwaitStatement
15.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseStatementCore
16.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseStatements
17.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseBlock
18.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseStatementCore
19.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseEmbeddedStatement
20.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseIfStatement
21.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseStatementCore
22.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseStatements
23.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseBlock
24.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseBlockAndExpressionBodiesWithSemicolon
25.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseMethodDeclaration
26.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseMemberDeclarationOrStatement
27.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseClassOrStructOrInterfaceDeclaration
28.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseTypeDeclaration
29.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseMemberDeclarationOrStatement
30.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseNamespaceBody
31.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseNamespaceDeclaration
32.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseNamespaceBody
33.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseCompilationUnitCore
34.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser.ParseWithStackGuard[[System.__Canon,_mscorlib]]
35.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.CSharpSyntaxTree.ParseText
36.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.CSharpCompiler.ParseFile
37.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.CSharpCompiler.ParseFile
38.Microsoft_CodeAnalysis_CSharp!Microsoft.CodeAnalysis.CSharp.CSharpCompiler+__c__DisplayClass7_0._CreateCompilation_b__0
39.Microsoft_CodeAnalysis!Roslyn.Utilities.UICultureUtilities+__c__DisplayClass6_0_1[[System.Int32,_mscorlib]]._WithCurrentUICulture_b__0
40.mscorlib_ni!System.Threading.Tasks.Parallel+__c__DisplayClass17_0_1[[System.__Canon,_mscorlib]]._ForWorker_b__1
41.mscorlib_ni!System.Threading.Tasks.Task.InnerInvokeWithArg
42.mscorlib_ni!System.Threading.Tasks.Task+__c__DisplayClass176_0._ExecuteSelfReplicating_b__0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston left review comments

Assignees
No one assigned
Labels
cla-already-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jaredpar @cston @AlekseyTs @VSadov @gafter @dnfclas