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
19k stars 4.03k forks source link

System.ArgumentNullException in SyntaxNodeExtensions.RemoveNode #4412

Open default0 opened 9 years ago

default0 commented 9 years ago

The following Code causes an exception:

void M()
{
    ExpressionStatementSyntax expressionStatement = SyntaxFactory.ExpressionStatement(SyntaxFactory.ParseExpression("a + b"));
    expressionStatement.RemoveNode(expressionStatement.Expression, SyntaxRemoveOptions.KeepNoTrivia);
}

My assumption is that an AST with an ExpressionStatementSyntax that has no ExpressionSyntax is invalid, and thus an error occurs. The error message could be a lot more descriptive about that, though...

Exception:

   at Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ExpressionStatement(ExpressionSyntax expression, SyntaxToken semicolonToken)
   at Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionStatementSyntax.Update(ExpressionSyntax expression, SyntaxToken semicolonToken)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitExpressionStatement(ExpressionStatementSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionStatementSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.SyntaxRemover.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitListElement[TNode](TNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitBlock(BlockSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.SyntaxRemover.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitMethodDeclaration(MethodDeclarationSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.MethodDeclarationSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.SyntaxRemover.Visit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.RemoveNodes[TRoot](TRoot root, IEnumerable`1 nodes, SyntaxRemoveOptions options)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode.RemoveNodesCore(IEnumerable`1 nodes, SyntaxRemoveOptions options)
   at Microsoft.CodeAnalysis.SyntaxNodeExtensions.RemoveNode[TRoot](TRoot root, SyntaxNode node, SyntaxRemoveOptions options)
   at RoslynBugTest.Program.Main() in c:\Users\cs\Documents\Visual Studio 2013\Projects\RoslynBugTest\RoslynBugTest\Program.cs:Zeile 31.
   at System.AppDomain._nExecuteAssembly(RuntimeAssembly assembly, String[] args)
   at System.AppDomain.ExecuteAssembly(String assemblyFile, Evidence assemblySecurity, String[] args)
   at Microsoft.VisualStudio.HostingProcess.HostProc.RunUsersAssembly()
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
vamuzumd commented 4 years ago

I am facing the same issue when trying to remove 'Message' IdentifierNode from the ex?.GetBaseException.Message expression.

The stack trace is as follows:-

'parent.RemoveNode(messageNode, SyntaxRemoveOptions.KeepExteriorTrivia)' threw an exception of type 'System.ArgumentNullException'

at Microsoft.CodeAnalysis.CSharp.SyntaxFactory.MemberAccessExpression(SyntaxKind kind, ExpressionSyntax expression, SyntaxToken operatorToken, SimpleNameSyntax name)\r\n at Microsoft.CodeAnalysis.CSharp.Syntax.MemberAccessExpressionSyntax.Update(ExpressionSyntax expression, SyntaxToken operatorToken, SimpleNameSyntax name)\r\n at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.VisitMemberAccessExpression(MemberAccessExpressionSyntax node)\r\n at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.Visit(SyntaxNode node)\r\n at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.SyntaxRemover.Visit(SyntaxNode node)\r\n at Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxNodeRemover.RemoveNodes[TRoot](TRoot root, IEnumerable1 nodes, SyntaxRemoveOptions options)\r\n at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode.RemoveNodesCore(IEnumerable1 nodes, SyntaxRemoveOptions options)\r\n at Microsoft.CodeAnalysis.SyntaxNodeExtensions.RemoveNode[TRoot](TRoot root, SyntaxNode node, SyntaxRemoveOptions options)"

Any ETA on when this issue would be fixed?

CyrusNajmabadi commented 4 years ago

Any ETA on when this issue would be fixed?

There is nothing to "fix" here, outside of throwing a more clear exception. What you are trying to do is illegal. A member-access-expression (i.e. expr.b) is required to have a non-null left and non-null right. You cannot remove either side without causing an invariant exception (which manifests here as an argumentnullexception).

If you want to convert the above to ex?.GetbaseException., then right thing to do is to not RemoveNode on 'Message', but instead do a ReplaceNode of Message with an empty IdentifierSyntax.

You can get a sense for what tree you need to convert the 'before' into by using a quoter tool like https://roslynquoter.azurewebsites.net/ . This will show you what the expected tree form is for something like ex?.GetbaseException.. You will have to generate that form, otherwise the behavior of all further parts of the system is undefined.

windhandel commented 1 year ago

Hi @CyrusNajmabadi I think the real bug (similar API, not identical) is that you can't tell based on the exception whether it's the first or second parameter that's the cause of the exception.

For example:

documentEditor.InsertBefore(firstMemberOfClass, declarationExpression);
CyrusNajmabadi commented 1 year ago

@windhandel We'd welcome a PR that helped with that. Thanks!