Skip to content

AbstractFlowPass.VisitStackAllocArrayCreation should use the regular way for visiting initializer#60210

Merged
jcouv merged 6 commits intodotnet:mainfrom
jcouv:stackalloc-region
Mar 25, 2022
Merged

AbstractFlowPass.VisitStackAllocArrayCreation should use the regular way for visiting initializer#60210
jcouv merged 6 commits intodotnet:mainfrom
jcouv:stackalloc-region

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Mar 16, 2022 •
edited
Loading

Fixes #60134

Fix is same as we recently did for arrays in PR #57612
FYI @bernd5

@ghost ghost added the Area-Compilers label Mar 16, 2022
@jcouv jcouv self-assigned this Mar 16, 2022
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 17, 2022

@dotnet/roslyn-compiler for second review (one-line fix)

@jcouv jcouv marked this pull request as ready for review March 17, 2022 03:12
@jcouv jcouv requested a review from a team as a code owner March 17, 2022 03:12
AlekseyTs
AlekseyTs previously approved these changes Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@jcouv jcouv enabled auto-merge (squash) March 17, 2022 23:48
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 18, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 18, 2022

Azure Pipelines successfully started running 4 pipeline(s).

@jcouv jcouv marked this pull request as draft March 18, 2022 17:24
auto-merge was automatically disabled March 18, 2022 17:24

Pull request was converted to draft

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 18, 2022

Investigating bootstrap issue (relates to nullable tracking for semantic model)...

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 18, 2022

Sorry for growing the PR. The bootstrap issue revealed that nullability analysis of stackalloc needed to be updated. I've shared the code with analysis of regular array creations.

@jcouv jcouv marked this pull request as ready for review March 18, 2022 22:11
@jcouv jcouv requested review from AlekseyTs and cston March 18, 2022 22:18
cston
cston reviewed Mar 18, 2022
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
// public T Item;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "Item").WithArguments("D<T>.Item", "").WithLocation(15, 14)
);
}
Copy link
Copy Markdown
Contributor

@cston cston Mar 18, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

}

Are we testing with a null element type? For instance:

#nullable enable
class Program
{
    unsafe static void Main()
    {
        var s0 = stackalloc[] { };
        var s1 = stackalloc[] { null };
        var s2 = stackalloc[] { x => x };
    }
}
``` #Resolved

@jcouv jcouv dismissed AlekseyTs’s stale review March 21, 2022 22:24

Made some required changes since your review

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
return null;
}

bool isInferred = node.Syntax.Kind() == SyntaxKind.ImplicitArrayCreationExpression;
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

node.Syntax

Syntax is not supposed to convey semantic information. We should avoid doing analysis based on that. #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022

Choose a reason for hiding this comment

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

However I see that this is a preexisting condition.

Copy link
Copy Markdown
Member Author

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

Filed #60376

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
return null;
}

private TypeSymbol VisitArrayInitialization(TypeSymbol type, bool isInferred, BoundArrayInitialization initialization, bool hasErrors)
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

VisitArrayInitialization

The name feels somewhat misleading because it looks like the helper is used for types other than arrays. #ByDesign

Copy link
Copy Markdown
Member Author

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

The bound node is called BoundArrayInitialization, so this is following our normal naming convention

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
static TypeWithAnnotations getSpanElementType(NamedTypeSymbol namedType)
{
Debug.Assert(namedType.Name == "Span");
Debug.Assert(namedType.OriginalDefinition.TypeParameters.Count() == 1);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

OriginalDefinition.TypeParameters.Count()

Arity? #Resolved

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
static TypeSymbol setSpanElementType(NamedTypeSymbol namedType, TypeWithAnnotations elementType)
{
Debug.Assert(namedType.Name == "Span");
Debug.Assert(namedType.OriginalDefinition.TypeParameters.Count() == 1);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

OriginalDefinition.TypeParameters.Count()

Arity? #Resolved

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
}

Debug.Assert(node.Type is not null);
bool isInferred = node.Syntax.Kind() == SyntaxKind.ImplicitStackAllocArrayCreationExpression;
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

node.Syntax

We should avoid using associated syntax node for the purpose of analysis. #Pending

AlekseyTs
AlekseyTs reviewed Mar 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
}

Debug.Assert(node.Type is not null);
bool isInferred = node.Syntax.Kind() == SyntaxKind.ImplicitStackAllocArrayCreationExpression;
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

isInferred

Are we testing scenario when this is true? #Resolved

Copy link
Copy Markdown
Member Author

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

Yes, Stackalloc_InferredType

AlekseyTs
AlekseyTs approved these changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 5)

@jcouv jcouv enabled auto-merge (squash) March 24, 2022 23:47
@jcouv jcouv merged commit ac603f8 into dotnet:main Mar 25, 2022
@ghost ghost added this to the Next milestone Mar 25, 2022
@jcouv jcouv deleted the stackalloc-region branch March 25, 2022 03:34
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
@RikkiGibson RikkiGibson mentioned this pull request Aug 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers

Projects

None yet

Milestone

17.2.P3

Development

Successfully merging this pull request may close these issues.

AnalyzeDataFlow fails for stackalloc array creation initializer expression

4 participants

@jcouv @AlekseyTs @cston @allisonchou