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

Add new ConversionKind to IsImplicitConversion test #35095

Merged
merged 5 commits into from
May 24, 2019

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Apr 18, 2019

Fixes #34984

UnsetConversionKind was added by this commit but the switch statement wasn't updated.

@ryzngard ryzngard requested review from agocke and jcouv April 18, 2019 00:23
@ryzngard ryzngard requested a review from a team as a code owner April 18, 2019 00:23
@ryzngard
Copy link
Contributor Author

Is there a place I can add tests? Just adding this code to our CodeAction tests didn't trigger the bug, but likely somewhere in the compiler layer there's a good place to add one.

@jcouv
Copy link
Member

jcouv commented Apr 18, 2019

@ryzngard For testing, based on what I can see in the issue (stack trace), you need to get the conversion that is causing problems (likely from a semantic model call) and check IsExplicit on it. You need to debug the IDE code to figure out where the conversion came from (what syntax, regular or speculative semantic model, ...).

In terms of where to add the new compiler test(s), I see that Conversion.IsExplicit seems particularly covered in GetSemanticInfoTests.cs. Seems a good place.

@jcouv jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 18, 2019
@jcouv
Copy link
Member

jcouv commented Apr 19, 2019

Marked PR as "private" for now. Please ping if you have any other questions, or once ready for review after test is added. Thanks

@ryzngard ryzngard removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 23, 2019
@ryzngard
Copy link
Contributor Author

@jcouv test added to cover this bug, should be ready for review

var foreachSyntaxNode = root.DescendantNodes().OfType<ForEachStatementSyntax>().Single();
var foreachSymbolInfo = model.GetForEachStatementInfo(foreachSyntaxNode);

Assert.Equal(Conversion.UnsetConversion, foreachSymbolInfo.CurrentConversion);
Copy link
Member

@jcouv jcouv Apr 23, 2019

Choose a reason for hiding this comment

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

UnsetConversion [](start = 36, length = 15)

Feels like there is a bigger issue here. I'd expect the conversion to be Conversion.NoConversion (and Exists to be false).

I think that was changed by PR #33648 which introduced UnsetConversion, but not intentionally.
Let's discuss with @gafter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to continue with this PR as is to fix the crash. I've filed #35918 to track a deeper fix if needed.

@ryzngard
Copy link
Contributor Author

@jcouv @RikkiGibson please take another look at this. I've opened a separate issue for the open concern #35918

@ryzngard ryzngard requested review from jcouv and RikkiGibson and removed request for jcouv May 23, 2019 19:21
@gafter
Copy link
Member

gafter commented May 23, 2019

@jcouv and @agocke are out of the office right now so it may take a little while to complete this review.

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 5)

@ryzngard ryzngard merged commit 946a943 into dotnet:master May 24, 2019
gafter pushed a commit that referenced this pull request May 24, 2019
@gafter
Copy link
Member

gafter commented May 24, 2019

@ryzngard The compiler team requires two reviews before merging. Generally speaking you should let each team merge reviews for code maintained by that team. Hopefully @jcouv will be able to look at this on Tuesday. I am not going to revert this, but it is possible there will be more work on the basis of his review.

@gafter gafter self-assigned this May 24, 2019
@ryzngard
Copy link
Contributor Author

@gafter my apologies. I'll remember that for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExtractMethodCodeRefactoringProvider throws InvalidOperationException on highlighting non-existent property
5 participants