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

Stack overflow in Rosyln CodeAnalysis when ambiguous name is used #71039

Closed
MoFtZ opened this issue Nov 30, 2023 · 2 comments · Fixed by #71140
Closed

Stack overflow in Rosyln CodeAnalysis when ambiguous name is used #71039

MoFtZ opened this issue Nov 30, 2023 · 2 comments · Fixed by #71140
Assignees
Milestone

Comments

@MoFtZ
Copy link

MoFtZ commented Nov 30, 2023

Version Used:

.NET 8.0 (SDK 8.0.100, Windows x64 Installer)

Steps to Reproduce:

  1. Create a net8.0 project.
  2. Create a C# file (Context.cs) containing a class having a Property and ReturnType with the same/ambiguous name.
namespace ILGPU
{
    public class Context
    {
        // RuntimeSystem is the name of a Property in the Context class.
        // RuntimeSystem is also the name of a Class.
        public const string RuntimeAssemblyName = RuntimeSystem.AssemblyName;
        public RuntimeSystem RuntimeSystem { get; }
    }
}
  1. Create a C# file (AssemblyAttributes.cs) that indirectly references this ambiguous name.
using ILGPU;
using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo(Context.RuntimeAssemblyName)]
  1. Run the command dotnet build.

I have attached code that reproduces the issue, following the above steps.
MinimalRepro.zip

Diagnostic Id:

Running dotnet build generates a stack overflow in Roslyn.

Command line output:

C:\MinimalRepro>dotnet build
MSBuild version 17.8.3+195e7f5a3 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error : Stack overflow. [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.<BindNamespaceOrTypeOrAliasSymbol>g__bindAlias|1004_3(<>c__DisplayClass1004_0 ByRef) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindNamespaceOrTypeSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindQualifiedName(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Syntax.SimpleNameSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindNamespaceOrTypeSymbol(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.100\Roslyn\Microsoft.CSharp.Core.targets(84,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindQualifiedName(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Syntax.SimpleNameSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Roslyn.Utilities.ConsList`1<Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol>, Boolean) [C:\MinimalRepro\ILGPU.csproj::TargetFramework=net8.0]
<snip>

Repeats "BindNamespaceOrTypeOrAliasSymbol, BindNamespaceOrTypeSymbol, BindQualifiedName".

Expected Behavior:

Expecting net8.0 to compile the existing code without any issues.

This is a regression, as the use of RuntimeSystem as both a property name and class name was not an issue on previous compilers (net48, net6.0, net7.0, etc).

Actual Behavior:

We are trying to enable support for .NET 8 for our existing .NET class library.

In net8.0, Roslyn is getting confused with RuntimeSystem. and generating a Stack overflow at compile-time.

Known workarounds:

  1. Change [assembly: InternalsVisibleTo(Context.RuntimeAssemblyName)] to `[assembly: InternalsVisibleTo("ILGPU")]
  2. Removing public RuntimeSystem RuntimeSystem { get; }
  3. Renaming the RuntimeSystem property.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 30, 2023
@jaredpar jaredpar assigned cston and 333fred and unassigned cston Dec 4, 2023
@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 4, 2023
@jaredpar jaredpar added this to the 17.9 milestone Dec 4, 2023
@333fred
Copy link
Member

333fred commented Dec 7, 2023

@MoFtZ just to make sure, you're not expecting the minimal repo version you submitted there to compile, right? Only a more complete example with various other types? Because your zip does not compile on .NET 7, so I'm just making sure.

@MoFtZ
Copy link
Author

MoFtZ commented Dec 7, 2023

hi @333fred, yes, that is correct. You should get compiler errors for the missing RuntimeSystem class.

333fred added a commit to 333fred/roslyn that referenced this issue Dec 7, 2023
We were attempt to resolve diagnostics in ColorColor attribute argument scenarios while we were in the middle of attribute argument binding. This then caused us to need to bind attributes to resolve the diagnostic, entering an infinite loop that eventually overflows. To solve this, I've avoided binding diagnostics in this scenario by passing around a BindingDiagnosticBag, rather than an `ImmutableBindingDiagnostic<AssemblySymbol>`. Doing this does require some limited and controlled mutation in the tree; to help ensure future maintainability, the mutation only occurs in one direction (the binder taking ownership of the bag). However, if we're uncomfortable with the mutation approach, we can instead simply leak the BindingDiagnosticBags in this scenario. This is a small enough source of them that we're unlikely to see major issues not pooling them. Fixes dotnet#71039.
333fred added a commit that referenced this issue Dec 15, 2023
We were attempting to resolve diagnostics in ColorColor attribute argument scenarios while we were in the middle of attribute argument binding. This then caused us to need to bind attributes to resolve the diagnostic, entering an infinite loop that eventually overflows. To solve this, I've allowed `ImmutableBindingDiagnostics` to avoid eagerly resolving diagnostics during construction. To better convey this new contract, I've also renamed the type to `ReadOnlyBindingDiagnostics`. As there are extensive uses of `ReadOnlyBindingDiagnostics`, and many should continue to eagerly resolve diagnostics, I've left it as resolving by default, and only adjusted this one codepath. Fixes #71039.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants