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

Correctly report mismatches in foreach types #34926

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
4 participants
@333fred
Copy link
Member

333fred commented Apr 11, 2019

Fixes #29971. Also updates the location of the error reporting to be the variable being iterated over, rather than the type of that variable. @dotnet/roslyn-compiler for review.

Correctly report mismatches in foreach types, and adjust location of …
…the warning to apply to the iteration variable.

@333fred 333fred requested a review from dotnet/roslyn-compiler Apr 11, 2019

@@ -5040,6 +5061,16 @@ public override void VisitForEachIterationVariables(BoundForEachStatement node)
// declare and assign all iteration variables
TypeWithAnnotations sourceType = node.EnumeratorInfoOpt?.ElementTypeWithAnnotations ?? default;
TypeWithState sourceState = sourceType.ToTypeWithState();

#pragma warning disable IDE0055 // Fix formatting

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Apr 11, 2019

Contributor

is there something wrong with the formatting in here? It seems fine..

Perhaps it wants the { on the same indent level as the var?

This comment has been minimized.

Copy link
@gafter

gafter Apr 15, 2019

Member

It is a known issue with the formatter.


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

// mismatches
comp.VerifyDiagnostics(
// (8,18): warning CS8619: Nullability of reference types in value of type '(string, string?)' doesn't match target type 'string'.
// foreach ((var x, var y) in F())

This comment has been minimized.

Copy link
@gafter

gafter Apr 11, 2019

Member

These new diagnostics make no sense to me.

This comment has been minimized.

Copy link
@333fred

333fred Apr 13, 2019

Author Member

As mentioned in the prototype comment, this is a spurious warning. Chris is working on a separate fix for tuple types in foreach statements.

@gafter
Copy link
Member

gafter left a comment

Finished reviewing Commit 1. I have a hard time with the new diagnostics where indicated.

@gafter

gafter approved these changes Apr 15, 2019

Copy link
Member

gafter left a comment

:shipit:

@chsienki
Copy link
Contributor

chsienki left a comment

:shipit:

@333fred 333fred merged commit b304977 into dotnet:master Apr 16, 2019

1 check passed

license/cla All CLA requirements met.
Details

@333fred 333fred deleted the 333fred:foreach-loop-variable-mismatch branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.