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.

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

Fixes #49529 - Where on CSharp Scripting Environment compiler couldn't determine the 'var' type on the new switch style #55017

Merged
merged 5 commits into from
Jul 23, 2021

Conversation

ProIcons
Copy link
Contributor

@ProIcons ProIcons commented Jul 21, 2021

Fixes #49529

  • Added ExecutableBinder on chain

- Added ExecutableBinder on chain
@ProIcons ProIcons requested a review from a team as a code owner July 21, 2021 18:35
@ProIcons ProIcons changed the title Fixes #49529 Fixes #49529 - Where on CSharp Scripting Environment compiler couldn't determine the 'var' type on the new switch style Jul 21, 2021
@333fred
Copy link
Member

333fred commented Jul 21, 2021

Please add tests for this fix 🙂

@ProIcons ProIcons requested a review from a team as a code owner July 21, 2021 20:18
@dnfadmin
Copy link

dnfadmin commented Jul 21, 2021

CLA assistant check
All CLA requirements met.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Only a couple small comments.

src/Scripting/CSharpTest/ScriptTests.cs Show resolved Hide resolved
src/Scripting/CSharpTest/ScriptTests.cs Outdated Show resolved Hide resolved
@@ -481,7 +481,8 @@ internal sealed override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol>
fieldsBeingBound = new ConsList<FieldSymbol>(this, fieldsBeingBound);

var initializerBinder = new ImplicitlyTypedFieldBinder(binder, fieldsBeingBound);
var initializerOpt = initializerBinder.BindInferredVariableInitializer(diagnostics, RefKind.None, (EqualsValueClauseSyntax)declarator.Initializer, declarator);
var executableBinder = new ExecutableCodeBinder((EqualsValueClauseSyntax)declarator.Initializer, this, initializerBinder);
Copy link
Member

Choose a reason for hiding this comment

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

For future compiler team reviewers, this matches non-inferred variable binding: https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Binder/Binder_Initializers.cs,312. This path is missing an ExecutableCodeBinder, which means that things like SwitchExpressionBinders aren't created for the code in the initializer. That then causes the null ref in the bug report.

@333fred 333fred requested a review from a team July 21, 2021 20:50
Copy link
Member

@333fred 333fred 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 4). @dotnet/roslyn-compiler for a second review.

Copy link

@patrickklaeren patrickklaeren left a comment

Choose a reason for hiding this comment

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

LGTM

@333fred
Copy link
Member

333fred commented Jul 23, 2021

@dotnet/roslyn-compiler for a second review.

@333fred 333fred added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 23, 2021
@333fred 333fred merged commit f27e633 into dotnet:main Jul 23, 2021
@ghost ghost added this to the Next milestone Jul 23, 2021
@333fred
Copy link
Member

333fred commented Jul 23, 2021

Thanks @ProIcons!

@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullReferenceException when using switch expression with CSharpScript
6 participants