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 Inline temporary variable adds unnecessary cast #70423

Merged
merged 7 commits into from
Nov 23, 2024
Merged

fix Inline temporary variable adds unnecessary cast #70423

merged 7 commits into from
Nov 23, 2024

Conversation

pyoub
Copy link
Contributor

@pyoub pyoub commented Oct 18, 2023 •
edited
Loading

trying to fix Inline temporary variable adds unnecessary cast

Fixes: #69869

@pyoub pyoub requested a review from a team as a code owner October 18, 2023 02:41
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 18, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 18, 2023
@pyoub pyoub changed the title fix Inline temporary variable adds unnecessary cast #69869 fix Inline temporary variable adds unnecessary cast Oct 18, 2023
DoctorKrolic
DoctorKrolic reviewed Oct 18, 2023
View reviewed changes
...s/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs Outdated
@@ -414,7 +414,7 @@ ExpressionSyntax CreateExpressionToInline()

return SyntaxFactory.ArrayCreationExpression(arrayType, arrayInitializer);
}
else if (isVar && expression is ObjectCreationExpressionSyntax or ArrayCreationExpressionSyntax or CastExpressionSyntax)
else if (isVar && expression is ObjectCreationExpressionSyntax or ArrayCreationExpressionSyntax or CastExpressionSyntax or InvocationExpressionSyntax)
Copy link
Contributor

Choose a reason for hiding this comment

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

That is most likely not the right fix unfortunatelly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

i agree with DoctorKrolic. thsi cahnge doesn't really make sense. the comment below this explains the rational for why we don't need to add the cast. however, for an invocatino expression we still do need to cast to preserve semantics in many cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi can i have an example when we need to do the cast on InvocationExpressionSyntax ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, i think i missed that this iwas the is var case. In that case, it's likely safe to whitelist invocations as well. Can you update the comment inside to make this clearer? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

CyrusNajmabadi
CyrusNajmabadi reviewed Oct 20, 2023
View reviewed changes
...ditorFeatures/DiagnosticsTestUtilities/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs Outdated
@@ -22,7 +22,6 @@
using Microsoft.CodeAnalysis.Editor.UnitTests.Diagnostics;
using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Remote;
Copy link
Member

Choose a reason for hiding this comment

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

undo this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

undo change
400c42e 
add comment
CyrusNajmabadi
CyrusNajmabadi reviewed Oct 24, 2023
View reviewed changes
...s/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs Outdated
{
// if we have an InvocationExpressionSyntax `var x = Math.Round(1.1D);` there's no need to do any casting.
Copy link
Member

Choose a reason for hiding this comment

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

needs an explanation why this is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that clear ?

@CyrusNajmabadi
Copy link
Member

Thanks!

@pyoub
Copy link
Contributor Author

pyoub commented Oct 26, 2023

i don't know which tests are failing

@CyrusNajmabadi CyrusNajmabadi merged commit bb7610d into dotnet:main Nov 23, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 23, 2024
@CyrusNajmabadi
Copy link
Member

Thanks!

@dotnet-bot dotnet-bot mentioned this pull request Nov 25, 2024
Closed
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
@dotnet-bot dotnet-bot mentioned this pull request Nov 26, 2024
Closed
@dotnet-bot dotnet-bot mentioned this pull request Dec 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DoctorKrolic DoctorKrolic DoctorKrolic left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Milestone
17.13 P2
Development

Successfully merging this pull request may close these issues.

Inline temporary variable adds unnecessary cast
4 participants
@pyoub @CyrusNajmabadi @DoctorKrolic @jjonescz