Skip to content

Refactoring as operator to/from cast operator#48672

Merged
CyrusNajmabadi merged 70 commits intodotnet:masterfrom
MaStr11:RefactoringAsOperatorToFromCastOperator
Oct 23, 2020
Merged

Refactoring as operator to/from cast operator#48672
CyrusNajmabadi merged 70 commits intodotnet:masterfrom
MaStr11:RefactoringAsOperatorToFromCastOperator

Conversation

@MaStr11
Copy link
Contributor

@MaStr11 MaStr11 commented Oct 16, 2020 •
edited
Loading

Fixes #48621
Enables toggling between cast expressions:

var o = (object)1;
// vs.
var o = 1 as object;

o8uB3gWfB6

TODO

  • Initial (syntax only) CSharp fixer for both directions
  • Initial (syntax only) VB fixer for both directions
  • C# Take type into account for cast to as conversions
  • C# Test trivia handling
  • C# Test nested both directions
  • VB Take type into account for CType to TryCast conversions
  • VB Test CBool and the like
  • VB Test nesting
  • VB Test trivia handling
  • GetTitle
  • dynamic
  • Unconstraint generic (to ' as' for C# and VB)
  • Missing types (casted type can not be bound in "to ' as'" for C# and VB)

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Oct 16, 2020

Done with pass.

CyrusNajmabadi
CyrusNajmabadi reviewed Oct 19, 2020
View reviewed changes
@MaStr11 MaStr11 marked this pull request as ready for review October 19, 2020 19:28
@MaStr11 MaStr11 requested a review from a team as a code owner October 19, 2020 19:28
@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 19, 2020
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Oct 19, 2020

Please avoid touching anything for the moment as i refactor things :)

MaStr11
MaStr11 commented Oct 20, 2020
View reviewed changes
Copy link
Contributor Author

@MaStr11 MaStr11 left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring. It's much simpler/clearer now. And thanks for taking care of the naming too!

I just found some issues in one of the test cases.

src/EditorFeatures/CSharpTest/ConvertCast/ConvertTryCastToDirectCastTests.cs
}";
await new VerifyCS.Test
{
TestCode = InitialMarkup,
Copy link
Contributor Author

@MaStr11 MaStr11 Oct 20, 2020

Choose a reason for hiding this comment

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

If FixedCode is not set, the refactoring provider is not called at all. The setup must to look like this

Suggested change
TestCode = InitialMarkup,
TestCode = InitialMarkup,
FixedCode = InitialMarkup,
OffersEmptyRefactoring = false,

Copy link
Contributor Author

@MaStr11 MaStr11 Oct 20, 2020

Choose a reason for hiding this comment

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

It's a bit sad, that this case is not covered any longer (The refactoring was behaving like a code fixer in such cases).

src/EditorFeatures/CSharpTest/ConvertCast/ConvertTryCastToDirectCastTests.cs
await new VerifyCS.Test
{
TestCode = InitialMarkup,
CompilerDiagnostics = CompilerDiagnostics.None, // CS0077 is present, but we present the refactoring anyway (this may overlap with a diagnostic fixer)
Copy link
Contributor Author

@MaStr11 MaStr11 Oct 20, 2020

Choose a reason for hiding this comment

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

Should test for ExpectedDiagnostics instead.

This was another troubling test case I stumbled upon:

  • The refactoring was acting like a CodeFix: There was a diagnostic and after applying the code action it was gone
  • I was not able to cover this with ExpectedDiagnostics because the verifier tried to look up the diagnostic marker in the fixed source, even though I provided different ExpectedDiagnostics for TestState and FixedSate.

Maybe I missed something, but I gave up quickly and used CompilerDiagnostics.None as a recourse.

src/Features/Core/Portable/ConvertCast/AbstractConvertCastCodeRefactoringProvider.cs
var fromNodes = await context.GetRelevantNodesAsync<TFromExpression>().ConfigureAwait(false);
var from = fromNodes.FirstOrDefault(n => n.RawKind == FromKind);
if (from == null)
return;
Copy link
Contributor Author

@MaStr11 MaStr11 Oct 20, 2020

Choose a reason for hiding this comment

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

Suggested change
return;
{
return;
}

src/Features/Core/Portable/ConvertCast/AbstractConvertCastCodeRefactoringProvider.cs
return;

if (from.GetDiagnostics().Any(d => d.DefaultSeverity == DiagnosticSeverity.Error))
return;
Copy link
Contributor Author

@MaStr11 MaStr11 Oct 20, 2020

Choose a reason for hiding this comment

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

Suggested change
return;
{
return;
}

@CyrusNajmabadi CyrusNajmabadi merged commit faf6ff4 into dotnet:master Oct 23, 2020
@ghost ghost added this to the Next milestone Oct 23, 2020
@MaStr11 MaStr11 deleted the RefactoringAsOperatorToFromCastOperator branch October 30, 2020 14:04
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
@sharwell
Copy link
Contributor

sharwell commented May 21, 2021

@MaStr11 Thank you for this it is proving super useful!!

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

Reviewers

1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Reviewers whose approvals may not affect merge requirements

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.9.P2

Development

Successfully merging this pull request may close these issues.

Refactoring: Toggle between as operator and explicit cast expression

6 participants

@MaStr11 @CyrusNajmabadi @sharwell @jinujoseph @allisonchou @Dotnet-GitSync-Bot