[features/param-nullchecking] The BangBang Update by kevinsun-dev · Pull Request #46520 · dotnet/roslyn

kevinsun-dev · 2020-08-03T19:41:55Z

Updated features/param-nullchecking for new !! syntax.

Updated Lexer
Updated Parser
Updated Syntax.xml
Updated Errors
Updated Tests

…ecking-fixes

src/Compilers/CSharp/Portable/Parser/Lexer.cs

src/Compilers/CSharp/Portable/Parser/SlidingTextWindow.cs

333fred · 2020-08-07T21:12:45Z

Done review pass (commit 23). The implementation is looking mostly good, but we need more tests. #Closed

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs

333fred · 2020-08-07T22:11:24Z

Done review pass (commit 24).

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

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

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

333fred

LGTM (commit 29). @dotnet/roslyn-compiler for a second review.

jaredpar · 2020-08-06T16:11:38Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

                case '!':
                    TextWindow.AdvanceChar();
-                    if (TextWindow.PeekChar() == '=')
+                    if (TextWindow.PeekChar() == '=' && TextWindow.PeekChar(-2) != '!')


Was a bit worried that this would cause a compat break but then remembered we do issue an error for this case:

var x = s!! = null;

Haven't gotten to the tests yet but if you didn't have to change a test like this we should write one to ensure it keeps failing after this change.

Oh, that's no longer in there. I think that was from commit 17, and is gone now. It does break things and I had to change a bit of PeerChar that broke an invariant back then, but those have since been removed.

cston · 2020-08-10T19:01:41Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        if (this.CurrentToken.Kind == SyntaxKind.ExclamationEqualsToken)
+                        {
+                            var exclamationEquals = this.EatToken();
+                            var exclamation2 = SyntaxFactory.Token(exclamationEquals.GetLeadingTrivia(), SyntaxKind.ExclamationToken, null);


exclamation2 [](start = 32, length = 12)

Consider renaming to "exclamation" since there's no "exclamation[1]'.

Might do that, or rename it to secondExclamation to emphasize that it's meant to be a part of the other Exclamation.

cston · 2020-08-10T19:30:25Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+
+        private SyntaxToken MergeTokens(SyntaxToken s1, SyntaxToken s2, SyntaxKind kind)
+        {
+            if (s1.GetTrailingTriviaWidth() == 0 && s2.GetLeadingTriviaWidth() == 0)


s1.GetTrailingTriviaWidth() == 0 && s2.GetLeadingTriviaWidth() == 0 [](start = 16, length = 67)

Are we testing both cases where this fails? (When s1.GetTrailingTriviaWidth() > 0 and when s1.GetTrailingTriviaWidth() == 0 && s2.GetLeadingTriviaWidth() > 0?)

For instance:

static void F(object arg! != null);

Here are some of the cases covered:

void M0(string name !!=null) { } void M1(string name! !=null) { } void M2(string name!!= null) { } void M3(string name ! !=null) { } void M4(string name ! ! =null) { } void M5(string name! ! =null) { }

cston · 2020-08-10T20:34:21Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+            }
+            else
+            {
+                s2 = this.AddError(s2, ErrorCode.ERR_InvalidExprTerm, this.CurrentToken.Text);


this.CurrentToken.Text [](start = 70, length = 22)

Referencing this.CurrentToken seems surprising. Should this be s2 instead? #Closed

You're absolutely right, changed.

cston · 2020-08-10T20:45:35Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -10896,9 +10904,10 @@ private bool ScanParenthesizedImplicitlyTypedLambda(Precedence precedence)
            {
                // allow for       a) =>      or     a!) =>


a! [](start = 53, length = 2)

"a!!" #Closed

Fixed, thanks!

cston · 2020-08-10T21:08:49Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                if (exclamationExclamation != null)
+                {
+                    exclamationExclamation = (exclamationExclamation.Kind != SyntaxKind.ExclamationExclamationToken)
+                        ? ConvertToMissingWithTrailingTrivia(exclamationExclamation, SyntaxKind.ExclamationExclamationToken)


ConvertToMissingWithTrailingTrivia(exclamationExclamation, SyntaxKind.ExclamationExclamationToken) [](start = 26, length = 98)

Should MergeTokens() be handling, rather than requiring each caller to do so?

Thanks for the tip, I've modified MergeTokens to handle the conversion. Shouldn't have merged so early, it'll be in the next PR.

cston · 2020-08-10T21:34:57Z

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


-        [Fact(Skip = "PROTOTYPE(BangBang)")]
+        [Fact]
        public void TestOptParamMethodDeclarationWithNullValidationNoSpaces()


TestOptParamMethodDeclarationWithNullValidationNoSpaces [](start = 20, length = 55)

Is this distinct from TestNullCheckedArgWithLeadingNewLine()?

cston · 2020-08-10T21:35:44Z

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


-        [Fact(Skip = "PROTOTYPE(BangBang)")]
+        [Fact]
        public void TestNullCheckedArgWithLeadingNewLine()


TestNullCheckedArgWithLeadingNewLine [](start = 20, length = 36)

The name seems misleading since there aren't any newlines.

cston · 2020-08-10T21:36:54Z

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


-        [Fact(Skip = "PROTOTYPE(BangBang)")]
+        [Fact]
        public void TestNullCheckedArgWithLeadingSpace()


TestNullCheckedArgWithLeadingSpace [](start = 20, length = 34)

Are the tokens in the following three tests distinct? If not, please consider merging into a single test method using `[Theory][InlineData("...")]...".

cston · 2020-08-10T21:38:29Z

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


-        [Fact(Skip = "PROTOTYPE(BangBang)")]
+        [Fact]
+        public void TestNullCheckedArgWithSpaceAfterEquals()


TestNullCheckedArgWithSpaceAfterEquals [](start = 20, length = 38)

Is this distinct from TestNullCheckedArgWithTrailingSpace()?

Kevin Sun added 9 commits July 28, 2020 15:14

Merge Fixes

4357d27

Fixed Merge Errors - Building now...

cf67517

Update IOperationTests_INullCheckedParameters.cs

fbfb995

PR changes (part 1)

a205226

Undo Debug.Assert on LocalRewriter

70b8751

Updated ControlFlowGraphBuilder

7852421

Fixed Testing

a600bf1

Tab for a cause

70433c2

BangBang Update

a3310ec

kevinsun-dev changed the base branch from master to features/param-nullchecking August 3, 2020 19:42

Kevin Sun added 5 commits August 3, 2020 16:16

Merge branch 'features/param-nullchecking' into features/param-nullch…

8689843

…ecking-fixes

Testing Fixes

fca8d64

Revise Testing Baselines

3a427ee

Updated CodeGen Tests

8d0be34

Update NullableReferenceTypesTests.cs

6845278

kevinsun-dev closed this Aug 5, 2020

kevinsun-dev reopened this Aug 5, 2020

Moved BangBang from Lexer to Parser

42fa7f4

kevinsun-dev force-pushed the features/param-nullchecking-fixes branch from 1b210e2 to 42fa7f4 Compare August 5, 2020 20:27

333fred closed this Aug 5, 2020

333fred reopened this Aug 5, 2020

sharwell marked this pull request as ready for review August 5, 2020 20:57

sharwell requested a review from a team as a code owner August 5, 2020 20:57

sharwell marked this pull request as draft August 5, 2020 20:57

333fred removed the request for review from a team August 5, 2020 21:02

jcouv added the Area-Compilers label Aug 6, 2020

Fix !! Syntax Changes

63270e5

kevinsun-dev marked this pull request as ready for review August 6, 2020 17:01

333fred reviewed Aug 6, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Parser/Lexer.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 6, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Parser/SlidingTextWindow.cs Outdated Show resolved Hide resolved

Kevin Sun added 2 commits August 7, 2020 13:54

Update LanguageParser.cs

213b9f0

Update LanguageParser.cs

321d854

Updated with Bad Syntax

1540086

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Show resolved Hide resolved

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs Show resolved Hide resolved

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs Outdated Show resolved Hide resolved

Kevin Sun added 2 commits August 10, 2020 09:28

Added ECC Parsing Tests

afea9f4

Removed Test Noise

9deea17

333fred reviewed Aug 10, 2020

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationParsingTests.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 10, 2020

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaParameterParsingTests.cs Outdated Show resolved Hide resolved

Update DeclarationParsingTests.cs

862becb

333fred reviewed Aug 10, 2020

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationParsingTests.cs Show resolved Hide resolved

Kevin Sun added 2 commits August 10, 2020 11:03

Add Prototype Flags

5633907

Update LanguageParser.cs

7ae3921

333fred approved these changes Aug 10, 2020

View reviewed changes

jaredpar approved these changes Aug 10, 2020

View reviewed changes

cston reviewed Aug 10, 2020

View reviewed changes

Added and changed tests

80eeb83

cston reviewed Aug 10, 2020

View reviewed changes

kevinsun-dev merged commit 3b0b5e1 into dotnet:features/param-nullchecking Aug 11, 2020

		@@ -10896,9 +10904,10 @@ private bool ScanParenthesizedImplicitlyTypedLambda(Precedence precedence)
		{
		// allow for a) => or a!) =>

Conversation

kevinsun-dev commented Aug 3, 2020

Uh oh!

Uh oh!

Uh oh!

333fred commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred commented Aug 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinsun-dev Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

333fred commented Aug 7, 2020 •

edited

Loading

kevinsun-dev Aug 10, 2020 •

edited

Loading

cston Aug 10, 2020 •

edited

Loading

cston Aug 10, 2020 •

edited

Loading

cston Aug 10, 2020 •

edited

Loading