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

FIx Redundant Cast incorrect for SimpleArguments #44516 #45102

Merged
merged 10 commits into from
Jul 6, 2020
Merged

FIx Redundant Cast incorrect for SimpleArguments #44516 #45102

merged 10 commits into from
Jul 6, 2020

Conversation

paul1956
Copy link
Contributor

@paul1956 paul1956 commented Jun 12, 2020 •
edited by sharwell
Loading

Add tests for Casts in SimpleArguments

Fixes #44516

@paul1956 paul1956 requested a review from a team as a code owner June 12, 2020 05:02
@CyrusNajmabadi
Copy link
Member

This is failing the test: TestVisualBasic_DoNotRemove_NecessaryCastForParamsArgument

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 12, 2020
@paul1956
Copy link
Contributor Author

@CyrusNajmabadi I believe TestVisualBasic_DoNotRemove_NecessaryCastForParamsArgument should fail as written. If I copy the result into Visual Studio and remove the DirectCast Visual Studio is happy with the results. If you agree I will correct or remove the test.

@CyrusNajmabadi
Copy link
Member

The test appears to be correct. If you remove the cast, the code will still compile, but the semantics will be different, leading to different runtime behavior.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 12, 2020 •
edited
Loading

Please update the pr to handle the new case, without regressing the existing cases. Thanks!

@paul1956
Copy link
Contributor Author

All tests pass without any changes and new tests added to test issue and they pass..

CyrusNajmabadi
CyrusNajmabadi approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Thanks!

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 26, 2020
View reviewed changes
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Utilities/CastAnalyzer.vb
Dim parentSimpleArgument = TryCast(parent, SimpleArgumentSyntax)
If TypeOf parentSimpleArgument?.Expression Is CastExpressionSyntax OrElse
TypeOf parentSimpleArgument?.Expression Is PredefinedCastExpressionSyntax Then
Return semanticModel.GetTypeInfo(parentSimpleArgument.Expression, cancellationToken).Type
Copy link
Member

Choose a reason for hiding this comment

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

i see tests for the PredefinedCastExpressionSyntax case. but not for CastExpressionSyntax

@paul1956 paul1956 requested review from a team as code owners June 28, 2020 23:06
@333fred
Copy link
Member

333fred commented Jun 29, 2020

This PR has a lot of unrelated files in it. Is there anything that the compiler team actually needs to review in it?

@jaredpar
Copy link
Member

This PR has a lot of unrelated files in it. Is there anything that the compiler team actually needs to review in it?

Agree. This really needs to be cleaned up before we can consider merging. This change shouldn't be impacting 500+ files. In addithion to making our history hard to follow it's not reasonable to review such a change.

jaredpar
jaredpar previously requested changes Jun 29, 2020
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Git history needs to be cleaned up before we can review

@paul1956
Copy link
Contributor Author

@jaredpar @333fred I don't know what happened I changed only 2 files. I could delete my fork/PR and start again with just replacing the 2 changed files.

@CyrusNajmabadi
Copy link
Member

Did you merge something into your branch?

@jnm2
Copy link
Contributor

jnm2 commented Jun 30, 2020 •
edited
Loading

The last commit has a stray second parent. Looks like an accidental merge to an old version of the same commits.

image

The fix:

  1. Undo the last commit while leaving its changes staged.

    git reset head~ --soft

  2. Create the commit as intended (but this time not as a merge commit).

    git commit -m 'Add comments explaining which tests hit which issue'

  3. Fix the pull request by force-pushing.

    git push -f

@paul1956
Copy link
Contributor Author

Did you merge something into your branch?

yes but I don't know how it happened. It should be fixed now.

@paul1956 paul1956 requested a review from jaredpar June 30, 2020 19:00
@jaredpar jaredpar dismissed their stale review June 30, 2020 20:52

dismiss

@paul1956
Copy link
Contributor Author

paul1956 commented Jul 6, 2020

Ping @jaredpar @333fred is there anything still holding this up?

@CyrusNajmabadi CyrusNajmabadi merged commit e92e529 into dotnet:master Jul 6, 2020
@ghost ghost added this to the Next milestone Jul 6, 2020
@CyrusNajmabadi
Copy link
Member

Thanks!

@paul1956 paul1956 deleted the fixCastAnalyzerIsUnnecessary branch July 6, 2020 08:56
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
@paul1956 paul1956 mentioned this pull request Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jaredpar jaredpar Awaiting requested review from jaredpar

Assignees

@CyrusNajmabadi CyrusNajmabadi

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
16.8.P1
Development

Successfully merging this pull request may close these issues.

Redundant Cast incorrect for SimpleArguments
7 participants
@paul1956 @CyrusNajmabadi @333fred @jaredpar @jnm2 @JoeRobich @jinujoseph