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.

Sign up for GitHub

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

Jump to bottom

Fix null-ref in AliasSymbol.Equals #59649

Merged
merged 2 commits into from
Feb 19, 2022
Merged

Fix null-ref in AliasSymbol.Equals #59649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad74c90
Fix null-ref in AliasSymbol.Equals
jcouv Feb 18, 2022
a2a4584
Mark ContainingSymbol as not-nullable
jcouv Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/Compilers/CSharp/Portable/Symbols/AliasSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ internal abstract class AliasSymbol : Symbol
private readonly ImmutableArray<Location> _locations; // NOTE: can be empty for the "global" alias.
private readonly string _aliasName;
private readonly bool _isExtern;
private readonly Symbol? _containingSymbol;
private readonly Symbol _containingSymbol;

protected AliasSymbol(string aliasName, Symbol? containingSymbol, ImmutableArray<Location> locations, bool isExtern)
protected AliasSymbol(string aliasName, Symbol containingSymbol, ImmutableArray<Location> locations, bool isExtern)
{
Debug.Assert(locations.Length == 1 || (locations.IsEmpty && aliasName == "global")); // It looks like equality implementation depends on this condition.
Debug.Assert(containingSymbol is not null);

_locations = locations;
_aliasName = aliasName;
Expand All @@ -69,7 +70,7 @@ internal static AliasSymbol CreateGlobalNamespaceAlias(NamespaceSymbol globalNam
return new AliasSymbolFromResolvedTarget(globalNamespace, "global", globalNamespace, ImmutableArray<Location>.Empty, isExtern: false);
}

internal static AliasSymbol CreateCustomDebugInfoAlias(NamespaceOrTypeSymbol targetSymbol, SyntaxToken aliasToken, Symbol? containingSymbol, bool isExtern)
internal static AliasSymbol CreateCustomDebugInfoAlias(NamespaceOrTypeSymbol targetSymbol, SyntaxToken aliasToken, Symbol containingSymbol, bool isExtern)
{
return new AliasSymbolFromResolvedTarget(targetSymbol, aliasToken.ValueText, containingSymbol, ImmutableArray.Create(aliasToken.GetLocation()), isExtern);
}
Expand Down Expand Up @@ -200,7 +201,7 @@ public override Accessibility DeclaredAccessibility
/// return that as the "containing" symbol, even though the alias isn't a member of the
/// namespace as such.
/// </summary>
public sealed override Symbol? ContainingSymbol
public sealed override Symbol ContainingSymbol
{
get
{
Expand Down Expand Up @@ -253,7 +254,7 @@ public override bool Equals(Symbol? obj, TypeCompareKind compareKind)

return (object?)other != null &&
Equals(this.Locations.FirstOrDefault(), other.Locations.FirstOrDefault()) &&
this.ContainingAssembly.Equals(other.ContainingAssembly, compareKind);
Equals(this.ContainingSymbol, other.ContainingSymbol, compareKind);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ContainingSymbol

Is there a reason to switch to ContainingSymbol? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think we want the symbols not to be equal in the scenario with two compilations. But if we use ContainingAssembly (which is null) then they would be equal.
If we use ContainingSymbol (which is the global namespace) then they are not equal, which I think is correct.

We could only compare using ContainingSymbol when ContainingAssembly is null, if that seems better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a change in behavior then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous behavior for that scenario was a crash (null-ref). I don't think there's a change of behavior aside from that.
Am I missing something?

}

public override int GetHashCode()
Expand Down Expand Up @@ -358,7 +359,7 @@ internal BindingDiagnosticBag AliasTargetDiagnostics
private NamespaceSymbol ResolveExternAliasTarget(BindingDiagnosticBag diagnostics)
{
NamespaceSymbol? target;
if (!ContainingSymbol!.DeclaringCompilation.GetExternAliasTarget(Name, out target))
if (!ContainingSymbol.DeclaringCompilation.GetExternAliasTarget(Name, out target))
{
diagnostics.Add(ErrorCode.ERR_BadExternAlias, Locations[0], Name);
}
Expand All @@ -371,7 +372,7 @@ private NamespaceSymbol ResolveExternAliasTarget(BindingDiagnosticBag diagnostic

private NamespaceOrTypeSymbol ResolveAliasTarget(NameSyntax syntax, BindingDiagnosticBag diagnostics, ConsList<TypeSymbol>? basesBeingResolved)
{
var declarationBinder = ContainingSymbol!.DeclaringCompilation.GetBinderFactory(syntax.SyntaxTree).GetBinder(syntax).WithAdditionalFlags(BinderFlags.SuppressConstraintChecks | BinderFlags.SuppressObsoleteChecks);
var declarationBinder = ContainingSymbol.DeclaringCompilation.GetBinderFactory(syntax.SyntaxTree).GetBinder(syntax).WithAdditionalFlags(BinderFlags.SuppressConstraintChecks | BinderFlags.SuppressObsoleteChecks);
return declarationBinder.BindNamespaceOrTypeSymbol(syntax, diagnostics, basesBeingResolved).NamespaceOrTypeSymbol;
}

Expand All @@ -385,7 +386,7 @@ internal sealed class AliasSymbolFromResolvedTarget : AliasSymbol
{
private readonly NamespaceOrTypeSymbol _aliasTarget;

internal AliasSymbolFromResolvedTarget(NamespaceOrTypeSymbol target, string aliasName, Symbol? containingSymbol, ImmutableArray<Location> locations, bool isExtern)
internal AliasSymbolFromResolvedTarget(NamespaceOrTypeSymbol target, string aliasName, Symbol containingSymbol, ImmutableArray<Location> locations, bool isExtern)
: base(aliasName, containingSymbol, locations, isExtern)
{
_aliasTarget = target;
Expand Down
40 changes: 40 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4450,6 +4450,46 @@ void M() { }
Assert.Equal("DEBUG", model.GetConstantValue(root.DescendantNodes().OfType<InvocationExpressionSyntax>().Single()));
}

[Fact]
public void EqualsOnAliasSymbolWithNullContainingAssembly_NotEquals()
{
var text = @"
[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute("".NETCoreApp, Version = v6.0"", FrameworkDisplayName = """")]
";
var alias1 = getGlobalAlias(CreateCompilation(text));
var alias2 = getGlobalAlias(CreateCompilation(text));

Assert.Equal("<global namespace>", alias1.ContainingSymbol.ToTestDisplayString());
Assert.Null(alias1.ContainingAssembly);
Assert.False(alias1.Equals(alias2));

static IAliasSymbol getGlobalAlias(CSharpCompilation compilation)
{
var tree = compilation.SyntaxTrees.Single();
var node = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Where(ident => ident.Identifier.Text == "global").Single();
return compilation.GetSemanticModel(tree).GetAliasInfo(node);
}
}

[Fact]
public void EqualsOnAliasSymbolWithNullContainingAssembly_Equals()
{
var text = @"
[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute("".NETCoreApp, Version = v6.0"", FrameworkDisplayName = """")]
[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute("".NETCoreApp, Version = v6.0"", FrameworkDisplayName = """")]
";
var compilation = CreateCompilation(text);
var tree = compilation.SyntaxTrees.Single();
var nodes = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Where(ident => ident.Identifier.Text == "global").ToArray();
var model = compilation.GetSemanticModel(tree);
var alias1 = model.GetAliasInfo(nodes[0]);
var alias2 = model.GetAliasInfo(nodes[1]);

Assert.Equal("<global namespace>", alias1.ContainingSymbol.ToTestDisplayString());
Assert.Null(alias1.ContainingAssembly);
Assert.True(alias1.Equals(alias2));
}

#region "regression helper"
private void Regression(string text)
{
Expand Down