Captures Invalid Dimensions from an array variable declaration. #32843

YairHalberstadt · 2019-01-26T22:55:29Z

Initially this pull request enables the dimensions specified in error in a variable declaration such as this:

int[10] x;

To be captured in a BoundTypeExpression as BoundDimensionsOpt, and to be consumed via an added IgnoredDimensions Property in IVariableDeclarationOperation.

See #32464

In future pull requests I may look at including support for capturing the invalid dimensions in:

parameters
foreach and for loop variables
typeof expressions
type arguments
is type expressions
casts
as expressions
Return types
Field types
etc.

It is unlikely that I will do all of these however, so suggestions as to which would be highest priority would be useful.

…ds it to IVariableOperation

gafter · 2019-01-28T01:14:59Z

@YairHalberstadt There are a few failed tests.

YairHalberstadt · 2019-01-28T09:28:21Z

Should be done now. They were broken by the merge from Master.

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

AlekseyTs · 2019-01-28T19:58:14Z

src/Compilers/Core/Portable/Operations/IVariableDeclarationOperation.cs

+        /// Array dimensions supplied to an array declaration in error cases, ignored by the compiler. This is only used for the C# case of
+        /// RankSpecifierSyntax nodes on an ArrayTypeSyntax.
+        /// </summary>
+        ImmutableArray<IOperation> IgnoredDimensions { get; }


ImmutableArray IgnoredDimensions { get; } [](start = 8, length = 53)

It looks like we already expose this information through IVariableDeclaratorOperation.IgnoredArguments #Closed

That's for

int x[10]

Not

int[10] x ``` #Closed

AlekseyTs · 2019-01-28T22:00:26Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-            var boundDeclType = new BoundTypeExpression(typeSyntax, aliasOpt, inferredType: isVar, type: declTypeOpt.TypeSymbol);
+
+            ImmutableArray<BoundExpression> dimensionsOpt;
+            if (typeSyntax is ArrayTypeSyntax arrayTypeSyntax && arrayTypeSyntax.RankSpecifiers.Count > 0)


typeSyntax is ArrayTypeSyntax arrayTypeSyntax [](start = 16, length = 45)

Do we need to worry about type syntax other than ArrayTypeSyntax? For example, a generic type with an array type as a type argument. #Closed

In an array creation that would also be invalid. For now I'm just trying to capture cases where the syntax would be valid in an ArrayCreationExpression, but invalid in a variable declaration, as I feel these are probably the most common cases.

I think there a huge amount of scope to do more here. This is meant to be an initial pull request with the minimum changes required to be useful.

For now I'm just trying to capture cases where the syntax would be valid in an ArrayCreationExpression, but invalid in a variable declaration, as I feel these are probably the most common cases.

Why are these scenarios more interesting? What has changed in variable declarations around array type syntax?

I think there a huge amount of scope to do more here. This is meant to be an initial pull request with the minimum changes required to be useful.

I do not think I support this approach. It becomes very hard to make changes to features that has shipped due to backward compatibility considerations. This PR goes to master. It is not clear to me what is the final plan, how do we think it should look like at the end and what is the plan for us to get there before we ship. I'd rather have clarity about all the scenarios that we should handle here.

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

I believe we would have to put the member on a different node of the IOperation tree, so I view this as two seperate APIs.

I listed a large number of potential places where we could expose this information at the top of the pull request. I think we have to decide exactly which of them should go in the initial PR.

I think that we should handle all types here, not just array types. Dig through any type and collect sizes from all array types within it.

In reply to: 251623089 [](ancestors = 251623089,251617370)

Do we need to worry about type syntax other than ArrayTypeSyntax? For example, a generic type with an array type as a type argument.

This always ends up being parsed as a BinaryExpression, as when a generic type declaration cannot be parsed correctly, it seems we parse < as Less Than.

I can't think of any cases where we would parse an expression type as type syntax which would contain an invalid dimension other than an ArrayTypeSyntax.

This always ends up being parsed as a BinaryExpression, as when a generic type declaration cannot be parsed correctly, it seems we parse < as Less Than.

I can't think of any cases where we would parse an expression type as type syntax which would contain an invalid dimension other than an ArrayTypeSyntax.

Can one create such a tree manually by using SyntaxFactory? Also, I might be wrong, but I think with the recent changes to the parser an element type of the array type could be a nullable array type with sizes as well. Also, since we are going to do the collection of misplaced sizes for other contexts too, and in those contexts parser's behavior around handling of < might be slightly different, or it might change in the future, I think it would be more robust to create a helper that doesn't make any assumption on how the parser works, simply visits the syntax (by using a syntax visitor) and collects all size expressions from array types in the syntax.

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

AlekseyTs · 2019-01-28T22:02:08Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                var sizes = ArrayBuilder<BoundExpression>.GetInstance();
+                // We don't want to report any errors from this, as the syntax is invalid anyway
+                var dummyDiagnostics = DiagnosticBag.GetInstance();
+                foreach (var expressionSyntax in arrayTypeSyntax.RankSpecifiers[0].Sizes)


RankSpecifiers[0] [](start = 65, length = 17)

What about other rank specifiers? #Closed

As above

In an array creation that would also be invalid. For now I'm just trying to capture cases where the syntax would be valid in an ArrayCreationExpression, but invalid in a variable declaration, as I feel these are probably the most common cases.

I think there a huge amount of scope to do more here. This is meant to be an initial pull request with the minimum changes required to be useful.

As above

I do not think I support this approach. I see no good reason to handle some sizes, but not the others.

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

Is it important that we distinguish in the bound tree and the Semantic Model between:

int[10][] x; int[][10] x;

If so we will need to add a new type of BoundNode which contains this information as well as the BoundExpression for the invalid dimensions.

Is it important that we distinguish in the bound tree and the Semantic Model between:

csharp
int[10][] x;
int[][10] x;

I cannot think of any need for this at the moment. The sizes are misplaced expressions.

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

AlekseyTs · 2019-01-28T22:03:43Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

@@ -2858,6 +2848,26 @@ private BoundExpression BindArrayCreationExpression(ArrayCreationExpressionSynta
                : BindArrayCreationWithInitializer(diagnostics, node, node.Initializer, type, arraySizes);
        }

+        private BoundExpression BindArrayRankSpecifier(ExpressionSyntax arrayRankSpecifier, SyntaxNode node, DiagnosticBag diagnostics)


arrayRankSpecifier [](start = 72, length = 18)

The name feels misleading, this isn't a syntax for a rank specifier. #Closed

AlekseyTs · 2019-01-28T22:12:35Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                var dummyDiagnostics = DiagnosticBag.GetInstance();
+                foreach (var expressionSyntax in arrayTypeSyntax.RankSpecifiers[0].Sizes)
+                {
+                    var size = BindArrayRankSpecifier(expressionSyntax, typeSyntax, dummyDiagnostics);


var size = BindArrayRankSpecifier(expressionSyntax, typeSyntax, dummyDiagnostics); [](start = 20, length = 82)

It looks like, when a declaration statement contains multiple declarators, we are going to bind sizes multiple times. Moreover, the bound tree is going to have multiple distinct bound nodes associated with the same syntax. This is likely to create problems with SemanticModel and probably in other components. I think the sizes should be bound once (where the typeSyntax is bound) and the bound tree shouldn't contain duplicate nodes associated with sizes. #Closed

I don't think we do, and I have tested for that in the semantic model, but I will take another look when I get the chance.

What I believe happens is that multiple declarations all have access to the same BoundTypeExpression. There is no obvious way to avoid this. In the Semantic model we just take the first one.

Thinking back to it, a Multiple Local Declaration just contains a list of boundLocalDeclarations, and no BoundTypeExpression. Unless we plan to change that, the current approach is I think most sensible.

We already repeat the BoundTypeExpression multiple times in the tree, and the dimensions is a member of the BoundTypeExpression, so I do not see that this is any worse than what we already have.

We already repeat the BoundTypeExpression multiple times in the tree, and the dimensions is a member of the BoundTypeExpression, so I do not see that this is any worse than what we already have.

Yes, we probably have duplicates for BoundTypeExpression today, this is not good and we'd rather not make problem worse by duplicating even more nodes. SemanticModel builds a map from a syntax node to bound nodes associated with the syntax. In general, it is not Ok to have duplicates in that list of associated bound nodes. This might not cause observable problems with BoundTypeExpression because those are not real expressions. Or problems exist, but we simply not aware of them. I am not comfortable with duplicating bound nodes for expressions, because a lot more things happen for regular expressions.

In any case, It is not OK to bind the same expression over and over again when we can easily avoid that.

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

I'll give it a try when I get a chance.

@AlekseyTs

I think the sizes should be bound once (where the typeSyntax is bound) and the bound tree shouldn't contain duplicate nodes associated with sizes.

Where exactly do you mean by this?

As far as I can see there is no single location where we Bind a Type Syntax. Instead we create a BoundTypeExpression in a number of places, including here.

As far as I can see in a variable declaration the only time the TypeSyntax is Bound is when Binding the BoundLocalDeclaration.

Since a BoundMultipleLocalDeclarations is just a list of of BoundlocalDeclarations, there is no way that I can see to avoid this being bound multiple times.

Perhaps the real problem is with our definition of a BoundMultipleLocalDeclaration and BoundlocalDeclaration.

 <Node Name="BoundLocalDeclaration" Base="BoundStatement"> <Field Name="LocalSymbol" Type="LocalSymbol"/> <Field Name="DeclaredType" Type="BoundTypeExpression"/> <Field Name="InitializerOpt" Type="BoundExpression" Null="allow"/> <Field Name="ArgumentsOpt" Type="ImmutableArray<BoundExpression>" Null="allow"/> </Node>  <Node Name="BoundMultipleLocalDeclarations" Base="BoundStatement"> <Field Name="LocalDeclarations" Type="ImmutableArray<BoundLocalDeclaration>"/> </Node>

if (variableCount == 1) { return BindVariableDeclaration(kind, isVar, variableList[0], typeSyntax, declType, alias, diagnostics, node); } else { BoundLocalDeclaration[] boundDeclarations = new BoundLocalDeclaration[variableCount]; int i = 0; foreach (var variableDeclarationSyntax in variableList) { boundDeclarations[i++] = BindVariableDeclaration(kind, isVar, variableDeclarationSyntax, typeSyntax, declType, alias, diagnostics); } return new BoundMultipleLocalDeclarations(node, boundDeclarations.AsImmutableOrNull()); }

We are making a fundamental distinction in the tree between a Declaration of multiple locals and a declaration of only one. When we declare multiple locals we create a list of local declarations, and when we are declaring only one we bind the single declaration directly. I don't see why such a distinction should exist.

Instead every declaration can contain an arbitrary number of locals. There is no need to special case the situation where it contains only one. This means that every single declaration should be a BoundMultipleLocalDeclarations, and every single BoundLocalDeclaration is just an item in the list of BoundMultipleLocalDeclarations. This means we can move DeclaredType to the containing BoundMultipleLocalDeclarations, which will avoid it being duplicated in the tree.

If you agree this should be done, I will obviously do so in a seperate PR as part of a seperate issue.

This means we can move DeclaredType to the containing BoundMultipleLocalDeclarations, which will avoid it being duplicated in the tree.

I think this would be fine. It looks like the BoundTypeExpression.InferredType can be moved to BoundLocalDeclaration.
Alternatively, we could make BoundLocalDeclaration.DeclaredType optional, and within BoundMultipleLocalDeclarations set it only for the first BoundLocalDeclaration. The BoundTypeExpression.InferredType should be moved in this case too. This approach would require less changes in the bound tree structure and you might find that attractive.

If you agree this should be done, I will obviously do so in a seperate PR as part of a seperate issue.

I think it would be fine to make this change in context of this PR because it is blocked by this issue. It would be fine to do this in a separate PR, but then we will have to wait for it to go through and then continue this PR on top of that work. #Closed

I tried my suggested solution first. Whilst I preferred this from a purity perspective, and it also simplifies the binding code in a number of situations, it leads to issues in error recovery for cases like this:

public class C { public void M() { var a = 14, b = 78.3; //error CS0819: Implicitly-typed variables cannot have multiple declarators int c = b; //error CS0266: Cannot implicitly convert type 'double' to 'int'. An explicit conversion exists (are you missing a cast?) int d = a; } }

https://sharplab.io/#v2:C4LglgNgPgAgzAAhgJgQYQQbwLACgEFKIwAsCAsgBQCUWehDCAbgIYBOCLCAvAgIwkANAgBGPBAHYAHADo4AbnqMCS5WAB2wBAGNxIxfmUINWgCbiWBhgF881oA=

Currently a is inferred as int, and b as double. Hence int c = b would give us an error, whilst int d = a would not;.

However this change would make that impossible, as type would now be per declaration statement, not per declaration.

So I'm going to try your approach.

However this change would make that impossible, as type would now be per declaration statement, not per declaration.

So I'm going to try your approach.

I am pretty sure that with either approach we could keep the same types (as the types before the change) for the locals, but I let you to decide what approach to use.

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

AlekseyTs · 2019-01-28T23:04:34Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IVariableDeclaration.cs

+{
+    void M1()
+    {
+        /*<bind>*/int[10] x;/*</bind>*/


10 [](start = 22, length = 2)

It would be good to have tests that try to get IOperation nodes for the sizes directly. #Closed

AlekseyTs · 2019-01-28T23:13:47Z

    public override void VisitVariableDeclaration(IVariableDeclarationOperation operation)

I would expect to see some changes in TestOperationVisitor.VisitVariableDeclaration(IVariableDeclarationOperation operation). Make sure all compiler tests pass with IOperation hook enabled build -testDesktop -testIOperation. #Closed

Refers to: src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs:474 in d2e7c06. [](commit_id = d2e7c06, deletion_comment = False)

AlekseyTs · 2019-01-28T23:17:24Z

It looks like we need some tests for GetTypeInfo, GetSymbolInfo and other SemanticModel APIs applicable to expressions. The tests should target the illegal dimension sizes #Closed

AlekseyTs · 2019-01-28T23:18:46Z

Done with review pass (iteration 4) #Closed

AlekseyTs · 2019-01-28T23:54:55Z

In an offline discussion with @gafter we thought it would be good to test scenarios where expression locals are declared within the invalid size expression (declaration pattern or out variable declarations can be used for that). Including cases when the locals are referenced later in otherwise legal expressions. #Closed

… from all ranks. Fix bug when declaring local variable inside invalid dimension.

YairHalberstadt · 2019-02-06T08:48:20Z

I have made the changes suggested in the review. I have also run -testIOperation, and have fixed all failing tests that looked like they were relevant (some appear to have been failing already, mostly to do with new language features.)

YairHalberstadt · 2019-02-06T08:57:00Z

By the way, I am going away for a week and a half tomorrow, so I might be able to reply to messages here, but I wont be able to work on this.

YairHalberstadt · 2019-02-06T12:56:23Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IVariableDeclaration.cs

+        [CompilerTrait(CompilerFeature.IOperation)]
+        [Fact]
+        public void IVariableDeclaration_InvalidIgnoredDimensions_TestSemanticModel()
+        {


I imagine this test shouldn't go here, but I wasn't sure where it should go, so I put it here for now.

AlekseyTs · 2019-02-11T17:51:05Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1184,7 +1184,7 @@ public override BoundNode VisitLocalDeclaration(BoundLocalDeclaration node)
                return null;
            }

-            bool inferredType = node.DeclaredType.InferredType;
+            bool inferredType = node.DeclaredTypeOpt?.InferredType ?? false; // a declaration with an inferred type will always store its DeclaredType


node.DeclaredTypeOpt?.InferredType ?? false; // a declaration with an inferred type will always store its DeclaredType [](start = 32, length = 118)

Is InferredType value the only reason for duplicating BoundTypeExpression for the var case? Please move the property to BoundLocalDeclaration and get rid of the duplication. #Closed

A different type may be inferred for each local declaration, so without duplication we will lose that information.

A different type may be inferred for each local declaration, so duplication will pose that information.

How and where does that help? We still have only one syntax node for the var keyword. I am not suggesting to make any changes to how we infer the type of the local and what type do we capture in the LocalSymbol.

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

How and where does that help? We still have only one syntax node for the var keyword. I am not suggesting to make any changes to how we infer the type of the local and what type do we capture in the LocalSymbol.

In a var case, for every bound local we do this:

if (isVar) { aliasOpt = null; bindType = true; initializerOpt = BindInferredVariableInitializer(diagnostics, value, valueKind, localSymbol.RefKind, declarator); // If we got a good result then swap the inferred type for the "var" TypeSymbol initializerType = initializerOpt?.Type; if ((object)initializerType != null) { declTypeOpt = TypeSymbolWithAnnotations.Create(initializerType);

We then use the declTypeOpt when creating the boundDeclType as follows:

boundDeclType = new BoundTypeExpression(typeSyntax, aliasOpt, inferredType: isVar, dimensionsOpt: invalidDimensions.ToImmutableAndFree(), type: declTypeOpt.TypeSymbol); } return new BoundLocalDeclaration(associatedSyntaxNode, localSymbol, boundDeclType, initializerOpt, arguments, hasErrors);

So every boundNode will have a different type.

So every boundNode will have a different type.

How and where does that help? I understand we were doing this before. The question is why do we have to keep doing this? We are changing things for other scenarios.

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

AlekseyTs · 2019-03-14T18:53:55Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs

+                case SyntaxKind.PredefinedType:
+                    break;
+                default:
+                    throw ExceptionUtilities.Unreachable;


throw ExceptionUtilities.Unreachable; [](start = 20, length = 37)

Since we are switching on the Kind, it might be better to UnexpectedValue```` helper instead of Unreachable```. #Closed

AlekseyTs

LGTM (iteration 19)

AlekseyTs · 2019-03-14T18:58:51Z

@YairHalberstadt Could you please resolve the merge conflicts? Also, would it be fine if we squash commits into one in the process of merging this PR?

YairHalberstadt · 2019-03-14T19:03:35Z

Will do, and that will be fine.

AlekseyTs · 2019-03-15T18:34:38Z

@YairHalberstadt It looks like DeeplyNestedGeneric unit-test (https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs#L103) fails on Windows_Desktop_Unit_Tests release_64. Does it fail for you locally?

YairHalberstadt · 2019-03-16T21:43:25Z

I've investigated the issue properly now, and can confirm, that whilst previously it was safe to visit a generic to depth of 730, as a result of VisitRankSpecifiers, it is now only safe to visit a generic to a depth of 680.

More worryingly the stack guard no longer works.

I will make two commits: The first will fix the test, the second will add a stack guard to VisitRankSpecifiers.

If you don't think a stack guard is necessary, I will revert the second commit.

YairHalberstadt · 2019-03-16T21:51:59Z

I think a better idea, is that since VisitRankSpecifiers is only necessary to provide better tooling and information, to simply not visit beyond a depth of 20 at all.
Does that make sense?

… unit test. EndToEndTests.DeeplyNestedGeneric is now back to testing a generic to a depth of 730.

YairHalberstadt · 2019-03-17T16:00:29Z

I've made my suggested change of only visiting rank specifiers to a depth of 20.
An alternative would be to make VisitRankSpecifiers mostly tail recursive as follows:

        internal static void VisitRankSpecifiers<TArg>(this TypeSyntax type, Action<ArrayRankSpecifierSyntax, TArg> action, TArg argument)
        {
start:
            switch (type.Kind())
            {
                case SyntaxKind.ArrayType:
                    var arrayTypeSyntax = (ArrayTypeSyntax)type;
                    arrayTypeSyntax.ElementType.VisitRankSpecifiers(action, argument);
                    foreach (var rankSpecifier in arrayTypeSyntax.RankSpecifiers)
                    {
                        action(rankSpecifier, argument);
                    }
                    break;
                case SyntaxKind.NullableType:
                    var nullableTypeSyntax = (NullableTypeSyntax)type;
                    type = nullableTypeSyntax.ElementType;
                    goto start;
                case SyntaxKind.PointerType:
                    var pointerTypeSyntax = (PointerTypeSyntax)type;
                    type = pointerTypeSyntax.ElementType;
                    goto start;
                case SyntaxKind.TupleType:
                    var tupleTypeSyntax = (TupleTypeSyntax)type;
                    var elementsCount = tupleTypeSyntax.Elements.Count;
                    if (elementsCount == 0)
                        break;

                    for (int index = 0; index < elementsCount - 1; index++)
                    {
                        var element = tupleTypeSyntax.Elements[index];
                        element.Type.VisitRankSpecifiers(action, argument);
                    }

                    type = tupleTypeSyntax.Elements[elementsCount - 1].Type;
                    goto start;
                case SyntaxKind.RefType:
                    var refTypeSyntax = (RefTypeSyntax)type;
                    type = refTypeSyntax.Type;
                    goto start;
                case SyntaxKind.GenericName:
                    var genericNameSyntax = (GenericNameSyntax)type;
                    var argsCount = genericNameSyntax.TypeArgumentList.Arguments.Count;
                    Debug.Assert(argsCount != 0);

                    for (int index = 0; index < argsCount - 1; index++)
                    {
                        var typeArgument = genericNameSyntax.TypeArgumentList.Arguments[index];
                        typeArgument.VisitRankSpecifiers(action, argument);
                    }

                    type = genericNameSyntax.TypeArgumentList.Arguments[argsCount - 1];
                    goto start;
                case SyntaxKind.QualifiedName:
                    var qualifiedNameSyntax = (QualifiedNameSyntax)type;
                    qualifiedNameSyntax.Left.VisitRankSpecifiers(action, argument);
                    type = qualifiedNameSyntax.Right;
                    goto start;
                case SyntaxKind.AliasQualifiedName:
                    var aliasQualifiedNameSyntax = (AliasQualifiedNameSyntax)type;
                    type = aliasQualifiedNameSyntax.Name;
                    goto start;
                case SyntaxKind.IdentifierName:
                case SyntaxKind.OmittedTypeArgument:
                case SyntaxKind.PredefinedType:
                    break;
                default:
                    throw ExceptionUtilities.UnexpectedValue(type.Kind());
            }

Since 70% of generic types in CoreFx have only one type argument, this would significantly reduce the risk of a stack overflow.

…tDepthOf21

AlekseyTs · 2019-03-18T21:41:18Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs

+            {
+                // Currently we only use this method to provide a better tooling experience when compiling incorrect syntax.
+                // Since this is not a must-have, we don't risk causing a stack overflow exception, so don't visit to a depth greater than 20.
+                const int MAX_RECURSION_DEPTH = 20;


const int MAX_RECURSION_DEPTH = 20; [](start = 16, length = 35)

I do not think we should use a hard-coded limit.

We need to make sure it is always called with the same limit, or some locals will not be bound properly. What is the best way to do that without hard coding the limit?

We need to make sure it is always called with the same limit, or some locals will not be bound properly. What is the best way to do that without hard coding the limit?

No limit based on depth.

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

Are you saying I should revert this commit, and use a stack guard instead?

Are you saying I should revert this commit, and use a stack guard instead?

Perhaps we should try getting rid of the recursion and using an array builder to manually keep track of the stack of TypeSyntax nodes to look at. Another thing that might help reducing the stack frame size for this method is to avoid using locals in the switch cases by inlining expressions where it makes sense and extracting body of the case into a separate function in other cases.

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

Also, it might be worth passing the argument to the delegate by reference since we are using a tuple for that.

In reply to: 266658685 [](ancestors = 266658685,266656829)

I showed above how we could use jumps to significantly reduce the depth of recursion by removing all the tail recursive calls.

That combined with inlining expressions and passing the tuple by ref would probably avoid issues in all but the most pathological of cases.

If that is not enough on its own, then we could use an array builder to store the type syntaxes as suggested, but that would risk allocations on the common code path for the sake of a very rare code case.

Passing the argument by ref increases the depth of recursion by 8.
Inlining and extracting local functions made no difference.
Replacing recursion with jumps avoids the problem entirely in the test case, as no stack overflow occurs inside VisitRankSpecifiers.

A pathological case would look like this:

(((((((..., int), int), int), int), int), int), int) x = default; //or ValueTuple<ValueTuple<ValueTuple<..., int>,int>,int> x = default;

However testing them, a stack overflow occurred in a completely unrelated bit of code long before VisitRankSpecifiers caused an issue, so I will not add a stack guard for now unless you think it's necessary.

AlekseyTs · 2019-03-18T21:42:10Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs

+        {
+            VisitRankSpecifiersInternal(type, action, argument, 0);
+
+            void VisitRankSpecifiersInternal(TypeSyntax typeInternal, Action<ArrayRankSpecifierSyntax, TArg> actionInternal, TArg argumentInternal, int recursionDepth)


VisitRankSpecifiersInternal [](start = 17, length = 27)

Our convention is to start names of local functions with a small letter.

AlekseyTs · 2019-03-18T21:44:28Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs

+        {
+            VisitRankSpecifiersInternal(type, action, argument, 0);
+
+            void VisitRankSpecifiersInternal(TypeSyntax typeInternal, Action<ArrayRankSpecifierSyntax, TArg> actionInternal, TArg argumentInternal, int recursionDepth)


VisitRankSpecifiersInternal [](start = 17, length = 27)

Can this local function be marked as static?

…y ref, and replacing recursion with loops where possible.

AlekseyTs · 2019-03-19T15:07:32Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs

+                case SyntaxKind.GenericName:
+                    var genericNameSyntax = (GenericNameSyntax)type;
+                    var argsCount = genericNameSyntax.TypeArgumentList.Arguments.Count;
+                    Debug.Assert(argsCount != 0);


Debug.Assert(argsCount != 0); [](start = 20, length = 29)

Is it not possible to violate this assumption by building the node manually? #Closed

Consider replacing the assert with:

if (argsCount == 0) break;

similar to the way TupleType is handled

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

AlekseyTs · 2019-03-19T15:13:02Z

Done with review pass (iteration 27)

AlekseyTs

LGTM (iteration 28)

AlekseyTs · 2019-03-20T17:23:21Z

@YairHalberstadt Could you please resolve the merge conflict?

YairHalberstadt · 2019-03-21T09:39:17Z

I have fixed the merge conflicts.

AlekseyTs · 2019-03-21T21:07:22Z

@YairHalberstadt Thank you for the contribution!

YairHalberstadt added 2 commits January 26, 2019 22:25

Captures Invalid Dimensions from an array variable declaration and ad…

902121a

…ds it to IVariableOperation

Merged From Master

9989dff

YairHalberstadt requested a review from a team as a code owner January 26, 2019 22:55

jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 27, 2019

gafter self-assigned this Jan 27, 2019

gafter added this to the 16.0.P4 milestone Jan 27, 2019

Fix breaks caused by merge from master

4b0abf6

YairHalberstadt commented Jan 28, 2019

View reviewed changes

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

Ignore diagnostics from binding invalid dimensions

d2e7c06

AlekseyTs reviewed Jan 28, 2019

View reviewed changes

YairHalberstadt added 2 commits February 3, 2019 13:48

Make BoundLocalDeclaration.DeclaredType optional

90c64ae

Bind invalid dimensions from nested array types in a type, as well as…

d985358

… from all ranks. Fix bug when declaring local variable inside invalid dimension.

Merged from master

c8d93b0

YairHalberstadt commented Feb 6, 2019

View reviewed changes

AlekseyTs reviewed Feb 11, 2019

View reviewed changes

gafter added this to the 16.1.P1 milestone Mar 14, 2019

AlekseyTs reviewed Mar 14, 2019

View reviewed changes

AlekseyTs approved these changes Mar 14, 2019

View reviewed changes

YairHalberstadt added 2 commits March 15, 2019 09:03

merged from master

9c8aea1

throw UnexpectedValue not Unreachable from VisitRankSpecifiers

a19133d

YairHalberstadt added 3 commits March 16, 2019 21:52

Only test DeeplyNestedGeneric to a depth of 670

8add215

Only Visit Rank Specifiers to a depth of 20 (arbitrarily chosen), and…

1a910c7

… unit test. EndToEndTests.DeeplyNestedGeneric is now back to testing a generic to a depth of 730.

Fix typo

1c860cd

Fix IVariableDeclaration_InvalidIgnoredDimensions_OutVarDeclaration_A…

6cc1577

…tDepthOf21

AlekseyTs reviewed Mar 18, 2019

View reviewed changes

YairHalberstadt added 2 commits March 19, 2019 07:45

Revert fixes to deeply nested generic

224539f

Limit the stack used by VisitRankSpecifiers by passing the argument b…

798881f

…y ref, and replacing recursion with loops where possible.

AlekseyTs reviewed Mar 19, 2019

View reviewed changes

Don't assume a generic type must have some type arguments.

2d379e7

AlekseyTs approved these changes Mar 19, 2019

View reviewed changes

merged from master

57e8803

AlekseyTs merged commit 7faa12f into dotnet:master Mar 21, 2019

gafter mentioned this pull request Mar 21, 2019

Capture extra invalid dimensions for SemanticModel and IOperation #32464

Closed

Captures Invalid Dimensions from an array variable declaration. #32843

Captures Invalid Dimensions from an array variable declaration. #32843

Conversation

YairHalberstadt commented Jan 26, 2019

gafter commented Jan 28, 2019

YairHalberstadt commented Jan 28, 2019

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Jan 28, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YairHalberstadt Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 28, 2019 • edited Loading

AlekseyTs commented Jan 28, 2019 • edited Loading

AlekseyTs commented Jan 28, 2019 • edited Loading

AlekseyTs commented Jan 28, 2019 • edited Loading

YairHalberstadt commented Feb 6, 2019

YairHalberstadt commented Feb 6, 2019

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 14, 2019

YairHalberstadt commented Mar 14, 2019

AlekseyTs commented Mar 15, 2019

YairHalberstadt commented Mar 16, 2019

YairHalberstadt commented Mar 16, 2019

YairHalberstadt commented Mar 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 19, 2019

AlekseyTs Jan 28, 2019 •

edited

Loading

YairHalberstadt Jan 28, 2019 •

edited by gafter

Loading

AlekseyTs Jan 28, 2019 •

edited

Loading

AlekseyTs Jan 28, 2019 •

edited

Loading

YairHalberstadt Feb 3, 2019 •

edited

Loading

AlekseyTs Jan 28, 2019 •

edited

Loading

AlekseyTs Jan 28, 2019 •

edited

Loading

AlekseyTs Feb 1, 2019 •

edited

Loading

AlekseyTs Jan 28, 2019 •

edited

Loading

AlekseyTs commented Jan 28, 2019 •

edited

Loading

AlekseyTs commented Jan 28, 2019 •

edited

Loading

AlekseyTs commented Jan 28, 2019 •

edited

Loading

AlekseyTs commented Jan 28, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

YairHalberstadt Feb 11, 2019 •

edited

Loading

AlekseyTs Mar 14, 2019 •

edited

Loading

AlekseyTs Mar 18, 2019 •

edited

Loading

AlekseyTs Mar 19, 2019 •

edited

Loading