dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.96k stars 4.02k forks source link

Rule proposal: Simplify VisualBasic TryCast and DirectCast calls #48239

Open carlossanlop opened 4 years ago

carlossanlop commented 4 years ago

Describe the problem you are trying to solve

It's very common in the dotnet/roslyn-analyzers repo to have language-specific code to handle Syntax nodes.

In C#, it's pretty easy to cast an object inside a condition:

if (myOriginalInstance is MyType myCastedInstance)
{
    ...

But in VB, the casting is a bit more elaborated:

Dim myCastedInstance As MyType = TryCast(myOriginalInstance, MyType)
If (myCastedInstance IsNot Nothing Then
    ...

In the VB example, the left part of the definition of myCastedInstance does not need to have the data type of the variable specified. The data type can be inferred from the TryCast, since the second argument of that operator is the data type to cast into.

So that code can be simplified to:

Dim myCastedInstance = TryCast(myOriginalInstance, MyType)

Describe suggestions on how to achieve the rule

  1. Detect a definition of a variable:
    Dim variableName
  2. Detect if the data type is being specified:
    Dim variableName As MyType
  3. Detect if it's getting assigned the value returned by a TryCast or a DirectCast operator invocation.
    Dim variableName As MyType = TryCast(originalVariableName, MyType)
  4. Maybe also check if the type detected in step 2 is the same as the same as the second argument in the TryCast in step 3.
  5. Remove the data type specification detected in step 2:
    Dim variable = TryCast(originalVariableName, MyType)
Evangelink commented 4 years ago

IMO this needs to be moved to roslyn repo and be handled as part of the useless code.

Youssef1313 commented 4 years ago

I can work on this if the team accepts the proposal.

CyrusNajmabadi commented 4 years ago

I can work on this if the team accepts the proposal.

I would champion this. Note: this would not just be for TryCast/DirectCast, but would be similar to the 'var' rules for C# where you could say:

image

these cases would fall under "when variable type is apparent" bucket

AdamSpeight2008 commented 4 years ago

Should be moved to VBLang. It is also disingenuous and disrespectful to other VBLang proposals, when they've been informed the VB.net is receiving any more updates (including tooling)

CyrusNajmabadi commented 4 years ago

Should be moved to VBLang.

@AdamSpeight2008 This is not a VBLang issue.

It is also disingenuous and disrespectful to other VBLang proposals, when they've been informed the VB.net is receiving any more updates (including tooling)

I don't know what you're talking about. We are constantly working on VB. As i said, I would support this and will bring to the next design meeting to be done. If you would like to discuss this more, you can come to gitter/discord and discuss it with me. Other comments about this on this issue will be treated as off-topic and moderated as such.

AdamSpeight2008 commented 4 years ago

It simplifies to Dim myCastedInstance = TryCast(myOriginalInstance, MyType), if Option Infer On is set, otherwise it changes the type of myCastedInstance to Object and not MyType. That behaviour can not change for compatibility reasons.

CyrusNajmabadi commented 4 years ago

It simplifies to Dim myCastedInstance = TryCast(myOriginalInstance, MyType), if Option Infer On

Yup. That's why I support this and would champion this.

That behaviour can not change for compatibility reasons.

Just like with 'use var' we would not offer if it changed semantics.