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

Bind child expressions within bad stackalloc expression to natural type #72454

Merged
merged 6 commits into from
Mar 15, 2024
Merged

Bind child expressions within bad stackalloc expression to natural type #72454

merged 6 commits into from
Mar 15, 2024

Conversation

cston
Copy link
Member

@cston cston commented Mar 8, 2024

Fixes #72448

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 8, 2024
@cston cston marked this pull request as ready for review March 8, 2024 20:34
@cston cston requested a review from a team as a code owner March 8, 2024 20:35
AlekseyTs
AlekseyTs reviewed Mar 14, 2024
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs Outdated
@@ -3355,7 +3357,8 @@ public override BoundNode VisitDefaultExpression(BoundDefaultExpression node)

public override BoundNode VisitUnconvertedObjectCreationExpression(BoundUnconvertedObjectCreationExpression node)
{
throw ExceptionUtilities.Unreachable();
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

throw ExceptionUtilities.Unreachable();

I think the whole point is to prevent BoundUnconvertedObjectCreationExpression from escaping initial binding, and that is the kind of the change I would expect instead. #Closed

Copy link
Member Author

@cston cston Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the whole point is to prevent BoundUnconvertedObjectCreationExpression from escaping initial binding, and that is the kind of the change I would expect instead.

That makes sense, thanks. Fixed.

AlekseyTs
AlekseyTs reviewed Mar 14, 2024
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated
@@ -309,19 +309,6 @@ private int RootSlot(int slot)
}
}

#if DEBUG
protected override void VisitRvalue(BoundExpression node, bool isKnownToBeAnLvalue = false)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

VisitRvalue

I think we should keep this method in its original form. Initial binding should be changed instead to maintain the invariant. #Closed

AlekseyTs
AlekseyTs reviewed Mar 14, 2024
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
Debug.Assert(node is BoundUnconvertedConditionalOperator);
Debug.Assert(_returnTypesOpt is not null || _disableDiagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Debug.Assert(_returnTypesOpt is not null || _disableDiagnostics);

The motivation for this change is not obvious #Closed

AlekseyTs
AlekseyTs reviewed Mar 14, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs Outdated
@@ -305,7 +305,9 @@ public CSharpOperationFactory(SemanticModel semanticModel)
case BoundKind.TypeExpression:
case BoundKind.TypeOrValueExpression:
case BoundKind.UnconvertedCollectionExpression:

case BoundKind.UnconvertedConditionalOperator:
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

case BoundKind.UnconvertedConditionalOperator:

Given other comments for this PR, I think this change is not expected. Besides, we should not be creating NoneOperation for language constructs that have IOperation representation (do not confuse with specific bound nodes). Effectively, NoneOperation represents a design debt for IOperation feature. I do not think the issue we are dealing with falls into the category. It looks like we are simply failing to properly handle an error scenario for a language construct that does have IOperation representation. #Closed

AlekseyTs
AlekseyTs reviewed Mar 14, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs Outdated
@@ -305,7 +305,9 @@ public CSharpOperationFactory(SemanticModel semanticModel)
case BoundKind.TypeExpression:
case BoundKind.TypeOrValueExpression:
case BoundKind.UnconvertedCollectionExpression:
Copy link
Contributor

Choose a reason for hiding this comment

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

case BoundKind.UnconvertedCollectionExpression:

This looks like a bug. Due to the reasons outlined in the comment for case BoundKind.UnconvertedConditionalOperator:, this case should be removed.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2), tests are not looked at.

@cston cston changed the title DefiniteAssignment: allow unconverted object creation expressions within bad expressions Validate child expressions within bad expressions have been converted to natural type Mar 15, 2024
@cston cston changed the title Validate child expressions within bad expressions have been converted to natural type Bind child expressions within bad stackalloc expression to natural type Mar 15, 2024
AlekseyTs
AlekseyTs approved these changes Mar 15, 2024
View reviewed changes
Copy link
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 6)

@cston cston requested a review from a team March 15, 2024 14:07
@AlekseyTs AlekseyTs mentioned this pull request Mar 15, 2024
Closed
jcouv
jcouv approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 6)

@jcouv jcouv self-assigned this Mar 15, 2024
@cston cston merged commit 8d932e9 into dotnet:main Mar 15, 2024
24 checks passed
@cston cston deleted the 72448 branch March 15, 2024 16:52
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 15, 2024
@dotnet-bot dotnet-bot mentioned this pull request Mar 21, 2024
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024
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

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.10 P3
Development

Successfully merging this pull request may close these issues.

Another COR_E_FAILFAST (-2146232797), courtesy of new()
4 participants
@cston @AlekseyTs @jcouv @RikkiGibson