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

Confirm dead code removal in IsReplaceableByVar was ok #15094

Closed
jcouv opened this issue Nov 8, 2016 · 6 comments
Closed

Confirm dead code removal in IsReplaceableByVar was ok #15094

jcouv opened this issue Nov 8, 2016 · 6 comments
Assignees
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Nov 8, 2016

I commented out a block of code below and then removed it without any test breaks. I still have some doubt whether this is a case of missing test or that code was truly unused.

        private static bool IsReplaceableByVar(
            this TypeSyntax simpleName,
            SemanticModel semanticModel,
            out TypeSyntax replacementNode,
            out TextSpan issueSpan,
            OptionSet optionSet,
            CancellationToken cancellationToken)
        {
            replacementNode = null;
            issueSpan = default(TextSpan);

            if (!optionSet.GetOption(SimplificationOptions.PreferImplicitTypeInLocalDeclaration))
            {
                return false;
            }

            // If it is already var
            if (simpleName.IsVar)
            {
                return false;
            }

            var candidateReplacementNode = SyntaxFactory.IdentifierName("var")
                                            .WithLeadingTrivia(simpleName.GetLeadingTrivia())
                                            .WithTrailingTrivia(simpleName.GetTrailingTrivia());
            var candidateIssueSpan = simpleName.Span;

            // If there exists a Type called var , fail.
            var checkSymbol = semanticModel.GetSpeculativeSymbolInfo(simpleName.SpanStart, candidateReplacementNode, SpeculativeBindingOption.BindAsTypeOrNamespace).Symbol;
            if (checkSymbol != null && checkSymbol.IsKind(SymbolKind.NamedType) && ((INamedTypeSymbol)checkSymbol).TypeKind == TypeKind.Class && checkSymbol.Name == "var")
            {
                return false;
            }

            // If the simpleName is the type of the Variable Declaration Syntax belonging to LocalDeclaration, For Statement or Using statement
            if (simpleName.IsParentKind(SyntaxKind.VariableDeclaration) &&
                ((VariableDeclarationSyntax)simpleName.Parent).Type == simpleName &&
                simpleName.Parent.Parent.IsKind(SyntaxKind.LocalDeclarationStatement, SyntaxKind.ForStatement, SyntaxKind.UsingStatement))
            {
                if (simpleName.Parent.IsParentKind(SyntaxKind.LocalDeclarationStatement) &&
                    ((LocalDeclarationStatementSyntax)simpleName.Parent.Parent).Modifiers.Any(n => n.Kind() == SyntaxKind.ConstKeyword))
                {
                    return false;
                }

                var variableDeclaration = (VariableDeclarationSyntax)simpleName.Parent;

                // Check the Initialized Value to see if it is allowed to be in the Var initialization
                if (variableDeclaration.Variables.Count != 1 ||
                    !variableDeclaration.Variables.Single().Initializer.IsKind(SyntaxKind.EqualsValueClause))
                {
                    return false;
                }

                var variable = variableDeclaration.Variables.Single();
                var initializer = variable.Initializer;
                var identifier = variable.Identifier;

                if (EqualsValueClauseNotSuitableForVar(identifier, simpleName, initializer, semanticModel, cancellationToken))
                {
                    return false;
                }

                replacementNode = candidateReplacementNode;
                issueSpan = candidateIssueSpan;
                return true;
            }

            if (simpleName.IsParentKind(SyntaxKind.ForEachStatement) &&
                ((ForEachStatementSyntax)simpleName.Parent).Type == simpleName)
            {
                replacementNode = candidateReplacementNode;
                issueSpan = candidateIssueSpan;
                return true;
            }

            //if (simpleName.IsParentKind(SyntaxKind.TypedVariableComponent))
            //{
            //    replacementNode = candidateReplacementNode;
            //    issueSpan = candidateIssueSpan;
            //    return true;
            //}
            // TODO REVIEW Are we lacking tests? It seems this code should be needed for DeclarationExpression

            return false;
        }
@jcouv
Copy link
Member Author

jcouv commented Jan 8, 2017

@CyrusNajmabadi This is a small question left from PR #14871. Do you have thoughts (was this code needed)?

@CyrusNajmabadi
Copy link
Member

I can look later tonight!

@CyrusNajmabadi
Copy link
Member

Looks like we've had this code forever. @dpoeschl @DustinCampbell Do you know what this code is for? Apperantly, it's not called at all, so it seems like it's safe to remove.

@CyrusNajmabadi
Copy link
Member

My mistake. I thought you meant hte entire function wasn't necessary. You just meant the part you commented out. That bit seems to have been added by @gafter in dbe4fb5. So he's probably best to determine why it's there and if it's needed.

@gafter
Copy link
Member

gafter commented Jan 9, 2017

I believe I added that code when I overhauled the syntax model around deconstruction and added the concept of components. Components were since removed from the syntax model, and were replaced by declaration expressions. I expect the code can safely be removed.

@jcouv
Copy link
Member Author

jcouv commented Jan 9, 2017

Thanks!
I'll go ahead and close.

@jcouv jcouv closed this as completed Jan 9, 2017
@jcouv jcouv added the Resolution-Answered The question has been answered label Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants