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

Bind native integers in cref #61431

Merged
merged 5 commits into from
May 28, 2022
Merged

Bind native integers in cref #61431

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4985e9d
Bind native integers in cref
jcouv May 20, 2022
fc0e8a2
Add test with member named nint
jcouv May 24, 2022
ea79435
Fix escaped nint in cref
jcouv May 27, 2022
1da55d6
Let nint/nuint in cref bind to field
jcouv May 27, 2022
c4feec6
Don't bind escaped nint to nint
jcouv May 27, 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
34 changes: 24 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,13 @@ private ImmutableArray<Symbol> BindNameMemberCref(NameMemberCrefSyntax syntax, N

int arity;
string memberName;
string memberNameText;

if (nameSyntax != null)
{
arity = nameSyntax.Arity;
memberName = nameSyntax.Identifier.ValueText;
memberNameText = nameSyntax.Identifier.Text;
}
else
{
Expand All @@ -161,7 +163,7 @@ private ImmutableArray<Symbol> BindNameMemberCref(NameMemberCrefSyntax syntax, N
containerOpt = BindNamespaceOrTypeSymbolInCref(syntax.Name);

arity = 0;
memberName = WellKnownMemberNames.InstanceConstructorName;
memberName = memberNameText = WellKnownMemberNames.InstanceConstructorName;
}

if (string.IsNullOrEmpty(memberName))
Expand All @@ -170,7 +172,7 @@ private ImmutableArray<Symbol> BindNameMemberCref(NameMemberCrefSyntax syntax, N
return ImmutableArray<Symbol>.Empty;
}

ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, arity, syntax.Parameters != null, diagnostics);
ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, memberNameText, arity, syntax.Parameters != null, diagnostics);

if (sortedSymbols.IsEmpty)
{
Expand All @@ -192,7 +194,7 @@ private ImmutableArray<Symbol> BindIndexerMemberCref(IndexerMemberCrefSyntax syn
{
const int arity = 0;

ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, WellKnownMemberNames.Indexer, arity, syntax.Parameters != null, diagnostics);
ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, WellKnownMemberNames.Indexer, memberNameText: WellKnownMemberNames.Indexer, arity, syntax.Parameters != null, diagnostics);

if (sortedSymbols.IsEmpty)
{
Expand Down Expand Up @@ -240,7 +242,7 @@ private ImmutableArray<Symbol> BindOperatorMemberCref(OperatorMemberCrefSyntax s
return ImmutableArray<Symbol>.Empty;
}

ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, arity, syntax.Parameters != null, diagnostics);
ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, memberNameText: memberName, arity, syntax.Parameters != null, diagnostics);

if (sortedSymbols.IsEmpty)
{
Expand Down Expand Up @@ -286,7 +288,7 @@ private ImmutableArray<Symbol> BindConversionOperatorMemberCref(ConversionOperat
memberName = WellKnownMemberNames.ExplicitConversionName;
}

ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, arity, syntax.Parameters != null, diagnostics);
ImmutableArray<Symbol> sortedSymbols = ComputeSortedCrefMembers(syntax, containerOpt, memberName, memberNameText: memberName, arity, syntax.Parameters != null, diagnostics);

if (sortedSymbols.IsEmpty)
{
Expand Down Expand Up @@ -324,17 +326,18 @@ private ImmutableArray<Symbol> BindConversionOperatorMemberCref(ConversionOperat
/// <remarks>
/// Never returns null.
/// </remarks>
private ImmutableArray<Symbol> ComputeSortedCrefMembers(CSharpSyntaxNode syntax, NamespaceOrTypeSymbol? containerOpt, string memberName, int arity, bool hasParameterList, BindingDiagnosticBag diagnostics)
private ImmutableArray<Symbol> ComputeSortedCrefMembers(CSharpSyntaxNode syntax, NamespaceOrTypeSymbol? containerOpt, string memberName, string memberNameText, int arity, bool hasParameterList, BindingDiagnosticBag diagnostics)
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var result = ComputeSortedCrefMembers(containerOpt, memberName, arity, hasParameterList, ref useSiteInfo);
var result = ComputeSortedCrefMembers(containerOpt, memberName, memberNameText, arity, hasParameterList, syntax, diagnostics, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);
return result;
}

private ImmutableArray<Symbol> ComputeSortedCrefMembers(NamespaceOrTypeSymbol? containerOpt, string memberName, int arity, bool hasParameterList, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
private ImmutableArray<Symbol> ComputeSortedCrefMembers(NamespaceOrTypeSymbol? containerOpt, string memberName, string memberNameText, int arity, bool hasParameterList,
CSharpSyntaxNode syntax, BindingDiagnosticBag diagnostics, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
// Since we may find symbols without going through the lookup API,
// Since we may find symbols without going through the lookup API,
// expose the symbols via an ArrayBuilder.
ArrayBuilder<Symbol> builder;
{
Expand All @@ -349,8 +352,19 @@ private ImmutableArray<Symbol> ComputeSortedCrefMembers(NamespaceOrTypeSymbol? c
diagnose: false,
useSiteInfo: ref useSiteInfo);

if (memberNameText is "nint" or "nuint"
&& containerOpt is null
&& arity == 0
&& !hasParameterList
&& !IsViableType(result))
Copy link
Member

@cston cston May 23, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

!IsViableType(result)

Are we testing members other than types? For instance:

/// <see cref="nint"/>
class C { int @nint; }
``` #Closed

{
result.Free(); // Won't be using this.
CheckFeatureAvailability(syntax, MessageID.IDS_FeatureNativeInt, diagnostics);
builder = ArrayBuilder<Symbol>.GetInstance();
builder.Add(this.GetSpecialType(memberName == "nint" ? SpecialType.System_IntPtr : SpecialType.System_UIntPtr, diagnostics, syntax).AsNativeInteger());
Copy link
Contributor

@Neme12 Neme12 May 27, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

This should also likely be memberNameText? A test for nuint that would catch this would be useful as well. #Resolved

}
// CONSIDER: Dev11 also checks for a constructor in the event of an ambiguous result.
if (result.IsMultiViable)
else if (result.IsMultiViable)
{
// Dev11 doesn't consider members from System.Object when the container is an interface.
// Lookup should already have dropped such members.
Expand Down
99 changes: 99 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10305,6 +10305,105 @@ public class C
comp.VerifyDiagnostics();
}

[Theory]
[InlineData("nint")]
[InlineData("nuint")]
[InlineData("System.IntPtr")]
[InlineData("System.UIntPtr")]
public void XmlDoc_Cref(string type)
{
var src = $$"""
/// <summary>Summary <see cref="{{type}}"/>.</summary>
class C { }
""";
var comp = CreateNumericIntPtrCompilation(src, references: new[] { MscorlibRefWithoutSharingCachedSymbols }, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal(type, cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var nintSymbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.True(nintSymbol.IsNativeIntegerType);
}

[Fact]
public void XmlDoc_Cref_Alias()
{
var src = """
using @nint = System.String;

/// <summary>Summary <see cref="nint"/>.</summary>
class C { }
""";

var comp = CreateNumericIntPtrCompilation(src, references: new[] { MscorlibRefWithoutSharingCachedSymbols }, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.False(symbol.IsNativeIntegerType);
Assert.Equal("System.String", symbol.ToTestDisplayString());
}

[Fact]
public void XmlDoc_Cref_Member()
{
var src = """
/// <summary>Summary <see cref="nint"/>.</summary>
Copy link
Contributor

@Neme12 Neme12 May 27, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

I think a test should be added where this is <see cref="@nint"/> and make sure that it does refer to the field in that case. Also, I would probably remove the @ from the field in that case just to make sure that it isn't just plain string matching. #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.

That was a good scenario. Fixed a bug. Thanks!

public class C
{
/// <summary></summary>
public int nint;
}
""";

var comp = CreateNumericIntPtrCompilation(src, references: new[] { MscorlibRefWithoutSharingCachedSymbols }, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.True(symbol.IsNativeIntegerType);
Copy link
Member

@333fred 333fred May 25, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

This appears to be a breaking change, as this code will currently reference the nint field. Is this what you expected, and if so, do we need to document it? #Resolved

Copy link
Member Author

@jcouv jcouv May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's an expected change.
Since this is narrow (requires something named nint and within xmldoc as opposed to code), this is more in the bug fix category and I think this doesn't warrant additional documentation (aside from the issue already filed for the record).

Copy link
Contributor

Choose a reason for hiding this comment

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

My 2 cents: I find it understandable and reasonable to make a breaking change like this, especially since it only relates to xmldoc. But I find it odd that this will still work for types named nint (no breaking change there), but not for fields named nint. It's much more likely that someone will have a field named nint than having a type named nint, because having lowercase types is much more rare than lowercase fields, where camel case is the standard naming convention. If it's OK to break lowercase fields, then I'd say that it's definitely OK to break lowercase types, because the latter is a lot less likely to occur in real code than the former.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, with this break, I would expect to be able to add @ and make it @nint to make it refer to the field (just like it's possible with all predefined keyword types). Does that work? If so, is there a test for that? If not, I think a test should be added.

Assert.Equal("nint", symbol.ToTestDisplayString());
}

[Fact]
public void XmlDoc_Cref_Member_Escaped()
Copy link
Contributor

@Neme12 Neme12 May 27, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

It would be a good idea to make this a Theory and test nuint as well and verify that it does indeed correspond to
IntPtr vs UIntPtr. Right now it seems like this won't work because of an omission in the code. #Resolved

{
var src = """
/// <summary>Summary <see cref="@nint"/>.</summary>
public class C
{
/// <summary></summary>
public int nint;
}
""";

var comp = CreateNumericIntPtrCompilation(src, references: new[] { MscorlibRefWithoutSharingCachedSymbols }, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("@nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (IFieldSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.Equal("System.Int32 C.nint", symbol.ToTestDisplayString());
}

private void VerifyNoNativeIntegerAttributeEmitted(CSharpCompilation comp)
{
// PEVerify is skipped because it reports "Type load failed" because of the above corlib,
Expand Down
149 changes: 149 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14813,5 +14813,154 @@ public static void M10(IntPtr{{s1Nullable}} x) { }

CompileAndVerify(comp, expectedOutput: "M1 M2 M3 M4 M5 M6 M7 M8 M9 M10");
}

[Theory]
[InlineData("nint")]
[InlineData("nuint")]
public void XmlDoc_Cref(string type)
{
var src = $$"""
/// <summary>Summary <see cref="{{type}}"/>.</summary>
class C { }
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal(type, cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var nintSymbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.True(nintSymbol.IsNativeIntegerType);
}

[Fact]
public void XmlDoc_Cref_IntPtr()
{
var src = """
/// <summary>Summary <see cref="System.IntPtr"/>.</summary>
class C { }
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("System.IntPtr", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var nintSymbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.False(nintSymbol.IsNativeIntegerType);
}

[Fact]
public void XmlDoc_Cref_Alias()
{
var src = """
using @nint = System.String;

/// <summary>Summary <see cref="nint"/>.</summary>
class C { }
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.False(symbol.IsNativeIntegerType);
Assert.Equal("System.String", symbol.ToTestDisplayString());
}

[Fact]
public void XmlDoc_Cref_Member()
{
var src = """
/// <summary>Summary <see cref="nint"/>.</summary>
public class C
{
/// <summary></summary>
public int nint;
}
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.True(symbol.IsNativeIntegerType);
Copy link
Member

@cston cston May 24, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

symbol.IsNativeIntegerType

If we're not binding to the field in this case, when would we expect cref="nint" to bind to a member? What if the member is class C { public class nint { } }? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

In short, what cases does the !IsViableType(result) check allow?

Copy link
Member Author

Choose a reason for hiding this comment

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

See XmlDoc_Cref_Alias, XmlDoc_Cref_NamedType and XmlDoc_Cref_TypeParameter for IsViableType(result) being true.
See XmlDoc_Cref_Member for IsViableType(result) being false.

The IsViableType check is the same we use in BindNonGenericSimpleNamespaceOrTypeOrAliasSymbol for binding "nint".

Copy link
Member

@cston cston May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. Given that cref="nint" binds to a peer type or a type parameter, I expected it to also bind to a field.

Assert.Equal("nint", symbol.ToTestDisplayString());
}

[Fact]
public void XmlDoc_Cref_NamedType()
{
var src = """
interface @nint { }

/// <summary>Summary <see cref="nint"/>.</summary>
class C { }
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics();

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (INamedTypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.False(symbol.IsNativeIntegerType);
Assert.Equal("nint", symbol.ToTestDisplayString());
Assert.Equal(TypeKind.Interface, symbol.TypeKind);
}

[Fact]
public void XmlDoc_Cref_TypeParameter()
{
var src = """
struct Outer<@nint>
{
/// <summary>Summary <see cref="nint"/>.</summary>
class C { }
}
""";

var comp = CreateCompilation(src, parseOptions: TestOptions.RegularWithDocumentationComments);
comp.VerifyDiagnostics(
// (3,37): warning CS1723: XML comment has cref attribute 'nint' that refers to a type parameter
// /// <summary>Summary <see cref="nint"/>.</summary>
Diagnostic(ErrorCode.WRN_BadXMLRefTypeVar, "nint").WithArguments("nint").WithLocation(3, 37)
);

var tree = comp.SyntaxTrees.Single();
var docComments = tree.GetCompilationUnitRoot().DescendantTrivia().Select(trivia => trivia.GetStructure()).OfType<DocumentationCommentTriviaSyntax>();
var cref = docComments.First().DescendantNodes().OfType<XmlCrefAttributeSyntax>().First().Cref;
Assert.Equal("nint", cref.ToString());

var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
var symbol = (ITypeSymbol)model.GetSymbolInfo(cref).Symbol;
Assert.False(symbol.IsNativeIntegerType);
Assert.Equal("nint", symbol.ToTestDisplayString());
Assert.Equal(TypeKind.TypeParameter, symbol.TypeKind);
}
}
}