Parsing for Parameter Nullchecking by fayrose · Pull Request #35826 · dotnet/roslyn

fayrose · 2019-05-20T20:58:40Z

Contains parsing for general case, lambda case and lambda case with multiple parameters, as well as initial test cases.

Relates to #36024 (test plan)

This reverts commit 8a94ac7.

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

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

YairHalberstadt · 2019-05-20T21:15:46Z

In commit b567a0d you made a large number of white space changes that appear to be unintended.

Would you be able to revert that commit?

This reverts commit 5f53063.

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

This reverts commit 5c7612b.

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

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

CyrusNajmabadi · 2019-05-20T21:40:59Z

Do we have a tracking bug to update the "add null check" IDE feature to use this if the language version is high enough?

We should also have a feature that looks for null-param checking code and offers to convert to this form.

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

…into param-nullchecking

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

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

CyrusNajmabadi · 2019-05-28T19:37:53Z

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

+        public void TestAnonymousDelegateNullChecking()
+        {
+            UsingTree(@"
+delegate void Del(int x!);


this should likely be an error right? you shouldnt' be allowed to place these on delegate-decls or interface/abstract methods right?

Note: if these are not allowed, i would put the check for this not in hte parser.

Will create interface/abstract tests and take a look at delegates

Note: just to make sure we're on hte same page, even if this is not allowed by the language, the checks should not be in the parser. That will make parsing of these guys even more context sensitive, which adds complexity and makes incremental parsing harder/more-dangerous. So we should allow parsing of these, but then give a nice error later on (i.e. when making the actual parameter symbols) if these are not allowed for teh current sort of construct.

Another case: extern methods.

if you are already writing tests for abstract methods and interface methods then please go ahead, but I would approve the PR without them as I know you'll get to them when you do the semantic side of this feature.

When PRing to feature branches please feel free to handle suggestions for additions with // PROTOTYPE comments, as long as the implementation is correct for the scenarios you've tested so far.

RikkiGibson

Looks good. All I see remaining is:

Mark PROTOTYPE or fix diagnostics produced in TestNullCheckedSingleParamNoSpaces
Mark PROTOTYPE the places where TryEatToken could be used, to update after master gets merged into the feature branch

RikkiGibson · 2019-05-28T20:56:36Z

src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

+        /// <summary>
+        ///   Looks up a localized string similar to The attribute [EnumeratorCancellation] cannot be used on multiple parameters.
+        /// </summary>
+        internal static string ERR_MultipleEnumeratorCancellationAttributes {


Feels odd that this diff is in here. Maybe merging in latest master and rebuilding will clean it up.

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

RikkiGibson · 2019-05-28T21:20:37Z

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

            else
            {
                var name = this.ParseIdentifierToken();
+                var exclamation = this.CurrentToken.Kind == SyntaxKind.ExclamationToken ? this.EatToken(SyntaxKind.ExclamationToken) : null;


Perhaps a prototype comment for this and similar checks which can be resolved after you merge master into the feature branch.

RikkiGibson · 2019-05-28T21:30:18Z

Might be good to squash when you merge due to the number of commits.

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

CyrusNajmabadi · 2019-05-29T17:08:13Z

Overall, things look good. But i would attempt to unify the pattern you use for handling incorrect tokens. Note: i do not mean you need to extract a common method to handle them, simply that the codepaths should look and feel similar, since they're effectively doing the same thing.

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

333fred · 2019-05-29T20:30:18Z

LGTM (commit 48). The comment I left about leading trivia can be addressed via prototype for now if you determine I was correct.

333fred · 2019-05-29T20:31:07Z

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

+        [Fact]
+        public void TestOptParamMethodDeclarationWithNullValidationNoSpaces()
+        {
+            UsingStatement(@"void M(string name!=null) { }", expectedErrors: new DiagnosticDescription[]


!= [](start = 47, length = 2)

We should also add more tests around trivia before/after this node so we make sure that we're preserving. Can be added in a follow up PR to not hold this initial one up.

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

CyrusNajmabadi · 2019-05-30T17:30:56Z

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

                    arrow = AddTrailingSkippedSyntax(arrow, gt);
-                    AddLeadingSkippedSyntax(arrow, notEq.GetLeadingTrivia());
-                    exclamation = AddLeadingSkippedSyntax(arrow, SyntaxFactory.MissingToken(SyntaxKind.ExclamationToken));
+                    exclamation = SyntaxFactory.MissingToken(SyntaxKind.ExclamationToken);


👍 Hopefully, all the construction/shuffling of the raw information is clear here for you :) I try to keep a mental model of all the nodes and what's going on with them, which helps make certain things stand out as looking "wrong". i.e. we had the 'arrow' nodes appearing in two places. on the 'arrow' local itself, but also as leading skipped syntax for the exclamation. This violated the strict invariants of the tree.

Thanks!

CyrusNajmabadi · 2019-05-30T17:31:42Z

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

                        N(SyntaxKind.Parameter);
                        {
                            N(SyntaxKind.IdentifierToken, "x");
+                            N(SyntaxKind.ExclamationToken);


do our parsing tests actually validate that tree invariants are preserved? we should do that so that we catch cases where we are either missing data in the tree, or we have duplicated data.

Note: such a change can come in a later PR.

However, it is very important as literally every layer above is written with the assumption that the compiler is absolutely guaranteeing these tree invariants. So we have to be careful to abide by them, otherwise the whole system above will fall over :)

I believe there is a string equality assert that ensures the tree is correct (bc I kept running into that failing for a bit), but not 100% sure what you're referring to.

That's likely what i'm referring to. Note: if that assert fails, we def should never be checking in (to master). To me, that specific assert is a hard blocker as it means we're not abiding by a core invariant that everything else needs in order to function properly.

Thanks!

To be clear: merging into feature branch: totally ok. Merging into master with it possible to hit that assert (either inside our outside of tests): Def not ok. Thanks! :)

Yeah all tests would fail if that assert is hit; currently all passing :)

Or at least the no-space test would fail.

Lauren Rosenberg added 10 commits May 20, 2019 09:52

Updated XML and Apis to include exclamationToken

5d01914

Generated C#

608df67

Tests

c3be1cf

Parser changes

78797cd

Minor change for brevity

63372e3

Line ending fix

b567a0d

Removed extra line ending

5f53063

Reverted accidental sln change

8a94ac7

Line ending removal

3e9176d

Revert "Reverted accidental sln change"

3b6dd44

This reverts commit 8a94ac7.

fayrose requested a review from a team as a code owner May 20, 2019 20:58

CyrusNajmabadi reviewed May 20, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 20, 2019

View reviewed changes

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

cston reviewed May 20, 2019

View reviewed changes

src/Compilers/CSharp/Portable/PublicAPI.Shipped.txt Show resolved Hide resolved

Lauren Rosenberg added 3 commits May 20, 2019 14:22

Re-added removed API's, added brackets as requested

4696645

Revert "Removed extra line ending"

c82dd14

This reverts commit 5f53063.

Complete reversion

5c7612b

cston reviewed May 20, 2019

View reviewed changes

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

Revert "Complete reversion"

5f690c7

This reverts commit 5c7612b.

cston reviewed May 20, 2019

View reviewed changes

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

Finish reversion; pls work this time

2895a6d

CyrusNajmabadi reviewed May 20, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 20, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 20, 2019

View reviewed changes

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

Derp

71480c4

RikkiGibson added the Area-Compilers label May 20, 2019

Lauren Rosenberg added 2 commits May 21, 2019 09:05

Extracted case into helper method

139b448

Merge branch 'param-nullchecking' of https://github.com/fayrose/roslyn …

c6c4828

…into param-nullchecking

prototype comment

92320d0

CyrusNajmabadi reviewed May 28, 2019

View reviewed changes

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

Moved down initial param def

a3e49c3

CyrusNajmabadi reviewed May 28, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 28, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 28, 2019

View reviewed changes

RikkiGibson approved these changes May 28, 2019

View reviewed changes

CyrusNajmabadi reviewed May 28, 2019

View reviewed changes

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

Lauren Rosenberg added 2 commits May 29, 2019 09:34

Fixed additional error prob

bc0168f

Added addt'l prototype comment

eb3ffd6

CyrusNajmabadi reviewed May 29, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 29, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 29, 2019

View reviewed changes

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

333fred reviewed May 29, 2019

View reviewed changes

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

333fred reviewed May 29, 2019

View reviewed changes

Lauren Rosenberg added 3 commits May 30, 2019 09:56

Nit fix

31a5798

Moved test to more appropriate file

c2c188b

Enumerated cases per cyrus' comment

73d4e03

CyrusNajmabadi reviewed May 30, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 30, 2019

View reviewed changes

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

CyrusNajmabadi reviewed May 30, 2019

View reviewed changes

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

Removed trivia changes and fixed test

5e989b3

CyrusNajmabadi reviewed May 30, 2019

View reviewed changes

333fred approved these changes May 30, 2019

View reviewed changes

fayrose merged commit 3816bf8 into dotnet:features/param-nullchecking May 30, 2019

fayrose deleted the param-nullchecking branch May 30, 2019 20:31

Conversation

fayrose commented May 20, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YairHalberstadt commented May 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented May 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

RikkiGibson May 28, 2019 • 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!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented May 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented May 29, 2019

Uh oh!

Uh oh!

333fred commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 30, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fayrose commented May 20, 2019 •

edited by jcouv

Loading

RikkiGibson May 28, 2019 •

edited

Loading

CyrusNajmabadi May 30, 2019 •

edited

Loading