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

Support unbound generic types in 'nameof' operator. #75368

Merged
merged 51 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
b363549
Add initial tests
CyrusNajmabadi Oct 3, 2024
f1f276b
Initial work
CyrusNajmabadi Oct 3, 2024
d5d1423
more working
CyrusNajmabadi Oct 3, 2024
f16971d
Better parsing
CyrusNajmabadi Oct 3, 2024
6d9a068
more work
CyrusNajmabadi Oct 3, 2024
b4ff542
names
CyrusNajmabadi Oct 3, 2024
092b89c
nesting
CyrusNajmabadi Oct 3, 2024
d0b2d12
Fixup tests
CyrusNajmabadi Oct 3, 2024
2d0155a
Fixup tests
CyrusNajmabadi Oct 3, 2024
db46266
Fixup tests
CyrusNajmabadi Oct 3, 2024
074bd23
Fix check
CyrusNajmabadi Oct 3, 2024
52dd592
Merge remote-tracking branch 'upstream/main' into openTypes
CyrusNajmabadi Oct 8, 2024
08e46f4
Fix test
CyrusNajmabadi Oct 8, 2024
5f1fd05
Merge branch 'main' into openTypes
CyrusNajmabadi Oct 9, 2024
bbdb37e
Merge remote-tracking branch 'upstream/main' into openTypes
CyrusNajmabadi Oct 10, 2024
2198c85
message id
CyrusNajmabadi Oct 10, 2024
ce2286a
Merge branch 'main' into openTypes
CyrusNajmabadi Nov 4, 2024
116d579
Merge remote-tracking branch 'upstream/main' into openTypes
CyrusNajmabadi Nov 14, 2024
da8e09e
Update src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
CyrusNajmabadi Nov 14, 2024
3147497
ADd tests
CyrusNajmabadi Nov 14, 2024
1df770b
Remove unnecessary options
CyrusNajmabadi Nov 14, 2024
10b1414
Merge branch 'openTypes' of https://github.com/CyrusNajmabadi/roslyn …
CyrusNajmabadi Nov 14, 2024
5a695dc
Add tests
CyrusNajmabadi Nov 14, 2024
4fcadd9
Simplify tests
CyrusNajmabadi Nov 14, 2024
7aff344
Share open type binding
CyrusNajmabadi Nov 14, 2024
f006bbb
Update src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
CyrusNajmabadi Nov 14, 2024
74a3fe0
Update src/Compilers/CSharp/Portable/Binder/TypeofBinder.cs
CyrusNajmabadi Nov 14, 2024
a671848
Add tests
CyrusNajmabadi Nov 14, 2024
ae6727f
Add tests
CyrusNajmabadi Nov 14, 2024
74bdd91
Merge branch 'openTypes' of https://github.com/CyrusNajmabadi/roslyn …
CyrusNajmabadi Nov 14, 2024
7a6328a
Simplify
CyrusNajmabadi Nov 14, 2024
cf0df29
Add semantic model test
CyrusNajmabadi Nov 14, 2024
5af3f7a
Update test
CyrusNajmabadi Nov 14, 2024
48688a5
Add testS
CyrusNajmabadi Nov 14, 2024
d0b7fa5
Update test
CyrusNajmabadi Nov 15, 2024
13e7eba
Update test
CyrusNajmabadi Nov 15, 2024
1e6366f
Add docs and tests
CyrusNajmabadi Nov 15, 2024
739a4ef
Update test
CyrusNajmabadi Nov 15, 2024
401ec47
Add more checks
CyrusNajmabadi Nov 15, 2024
9feec81
Merge branch 'main' into openTypes
CyrusNajmabadi Nov 15, 2024
6f6e987
Merge remote-tracking branch 'upstream/main' into openTypes
CyrusNajmabadi Nov 19, 2024
9497a5e
rename
CyrusNajmabadi Nov 19, 2024
7ed0bfe
do not use collection expression
CyrusNajmabadi Nov 19, 2024
30c1a45
do not use expr body or out var
CyrusNajmabadi Nov 19, 2024
efeea4c
Add tesets
CyrusNajmabadi Nov 19, 2024
f77d4d0
Adding tests
CyrusNajmabadi Nov 19, 2024
0a3e50a
use explicit else clause
CyrusNajmabadi Nov 19, 2024
21d4a4e
validate runtime behavior
CyrusNajmabadi Nov 19, 2024
0be4c20
Do not use expression bodies or patterns
CyrusNajmabadi Nov 19, 2024
a0e8e13
Add typeof teest
CyrusNajmabadi Nov 19, 2024
4d19b86
Add nameof teest
CyrusNajmabadi Nov 19, 2024
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
42 changes: 29 additions & 13 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1373,22 +1373,38 @@ private NamedTypeSymbol ConstructNamedTypeUnlessTypeArgumentOmitted(SyntaxNode t
{
if (typeArgumentsSyntax.Any(SyntaxKind.OmittedTypeArgument))
{
// Note: lookup won't have reported this, since the arity was correct.
// CONSIDER: the text of this error message makes sense, but we might want to add a separate code.
Error(diagnostics, ErrorCode.ERR_BadArity, typeSyntax, type, MessageID.IDS_SK_TYPE.Localize(), typeArgumentsSyntax.Count);
if (this.IsInsideNameof)
{
// Inside a nameof an open-generic type is acceptable. Fall through and bind the remainder accordingly.
CheckFeatureAvailability(typeSyntax, MessageID.IDS_FeatureUnboundGenericTypesInNameof, diagnostics);

// From the spec:
//
// Member lookup on an unbound type expression will be performed the same way as for a `this`
// expression within that type declaration.
//
// So we want to just return the originating type symbol as is (e.g. List<T> in nameof(List<>)).
// This is distinctly different than how typeof(List<>) works, where it returns an unbound generic
// type.
jcouv marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
// Note: lookup won't have reported this, since the arity was correct.
// CONSIDER: the text of this error message makes sense, but we might want to add a separate code.

// If the syntax looks like an unbound generic type, then they probably wanted the definition.
// Give an error indicating that the syntax is incorrect and then use the definition.
// CONSIDER: we could construct an unbound generic type symbol, but that would probably be confusing
// outside a typeof.
Error(diagnostics, ErrorCode.ERR_BadArity, typeSyntax, type, MessageID.IDS_SK_TYPE.Localize(), typeArgumentsSyntax.Count);
}

// If the syntax looks like an unbound generic type, then they probably wanted the definition.
// Give an error indicating that the syntax is incorrect and then use the definition.
// CONSIDER: we could construct an unbound generic type symbol, but that would probably be confusing
// outside a typeof.
return type;
}
else
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
// we pass an empty basesBeingResolved here because this invocation is not on any possible path of
// infinite recursion in binding base clauses.
return ConstructNamedType(type, typeSyntax, typeArgumentsSyntax, typeArguments, basesBeingResolved: null, diagnostics: diagnostics);
}

// we pass an empty basesBeingResolved here because this invocation is not on any possible path of
// infinite recursion in binding base clauses.
return ConstructNamedType(type, typeSyntax, typeArgumentsSyntax, typeArguments, basesBeingResolved: null, diagnostics: diagnostics);
}

/// <remarks>
Expand Down
12 changes: 10 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/NameofBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
Expand All @@ -19,19 +21,25 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal sealed class NameofBinder : Binder
{
private readonly SyntaxNode _nameofArgument;
private readonly ExpressionSyntax _nameofArgument;
Copy link
Member Author

Choose a reason for hiding this comment

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

This always had to be an ExpressionSyntax. Making strongly typed and to match the sig of OpenTypeVisitor.VisitThis

private readonly WithTypeParametersBinder? _withTypeParametersBinder;
private readonly Binder? _withParametersBinder;
private ThreeState _lazyIsNameofOperator;

internal NameofBinder(SyntaxNode nameofArgument, Binder next, WithTypeParametersBinder? withTypeParametersBinder, Binder? withParametersBinder)
private readonly Dictionary<GenericNameSyntax, bool>? _allowedMap;

internal NameofBinder(ExpressionSyntax nameofArgument, Binder next, WithTypeParametersBinder? withTypeParametersBinder, Binder? withParametersBinder)
: base(next)
{
_nameofArgument = nameofArgument;
_withTypeParametersBinder = withTypeParametersBinder;
_withParametersBinder = withParametersBinder;
OpenTypeVisitor.Visit(nameofArgument, out _allowedMap);
}

protected override bool IsUnboundTypeAllowed(GenericNameSyntax syntax)
=> _allowedMap != null && _allowedMap.TryGetValue(syntax, out bool allowed) && allowed;

private bool IsNameofOperator
{
get
Expand Down
99 changes: 99 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/OpenTypeVisitor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace Microsoft.CodeAnalysis.CSharp;

internal partial class Binder
{
/// <summary>
/// This visitor walks over a type expression looking for open types.
///
/// Open types are allowed if an only if:
/// <list type="number">
/// <item>There is no constructed generic type elsewhere in the visited syntax; and</item>
/// <item>The open type is not used as a type argument or array/pointer/nullable element type.</item>
/// </list>
///
/// Open types can be used both in <c>typeof(...)</c> and <c>nameof(...)</c> expressions.
/// </summary>
protected sealed class OpenTypeVisitor : CSharpSyntaxVisitor
Copy link
Member Author

Choose a reason for hiding this comment

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

extracted out of TypeOfBinder so it can be used by TypeOfBinder and NameOfBinder.

{
private Dictionary<GenericNameSyntax, bool>? _allowedMap;
private bool _seenConstructed;

/// <param name="typeSyntax">The argument to typeof.</param>
/// <param name="allowedMap">
/// Keys are GenericNameSyntax nodes representing unbound generic types.
/// Values are false if the node should result in an error and true otherwise.
/// </param>
public static void Visit(ExpressionSyntax typeSyntax, out Dictionary<GenericNameSyntax, bool>? allowedMap)
{
OpenTypeVisitor visitor = new OpenTypeVisitor();
visitor.Visit(typeSyntax);
allowedMap = visitor._allowedMap;
}

public override void VisitGenericName(GenericNameSyntax node)
{
SeparatedSyntaxList<TypeSyntax> typeArguments = node.TypeArgumentList.Arguments;
if (node.IsUnboundGenericName)
{
_allowedMap ??= [];
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
_allowedMap[node] = !_seenConstructed;
}
else
{
_seenConstructed = true;
foreach (TypeSyntax arg in typeArguments)
{
Visit(arg);
}
}
}

public override void VisitQualifiedName(QualifiedNameSyntax node)
{
bool seenConstructedBeforeRight = _seenConstructed;

// Visit Right first because it's smaller (to make backtracking cheaper).
Visit(node.Right);

bool seenConstructedBeforeLeft = _seenConstructed;

Visit(node.Left);

// If the first time we saw a constructed type was in Left, then we need to re-visit Right
if (!seenConstructedBeforeRight && !seenConstructedBeforeLeft && _seenConstructed)
{
Visit(node.Right);
}
}

public override void VisitAliasQualifiedName(AliasQualifiedNameSyntax node)
{
Visit(node.Name);
}

public override void VisitArrayType(ArrayTypeSyntax node)
{
_seenConstructed = true;
Visit(node.ElementType);
}

public override void VisitPointerType(PointerTypeSyntax node)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 19, 2024

Choose a reason for hiding this comment

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

VisitPointerType

Should VisitFunctionPointerType be overridden as well? #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.

Not sure. @333fred do function pointers need special handling here?

Copy link
Member Author

Choose a reason for hiding this comment

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

note: i've added some typeof and nameof tests taht exercise func-ptrs. The results seem expected to me. But i wouldn't mind SMEs here validating.

Copy link
Member

Choose a reason for hiding this comment

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

I've played with the change a bit and looked through the logic, and I can't see a scenario where it should matter; since we use DefaultVisit for the function pointer, no nested generic types will be marked as allowed. And for any containers, it seems that the following should always hold:

  • The container is a generic type, and we won't parse that as an unbound generic type if there are any arguments (ie, IsUnboundGenericName won't be true for something like Dictionary<,delegate*<List<>>)
  • The container is an array or nullable type, and that's always marked as seen constructed.
  • The container is a tuple type, and that's never visited by this visitor, so nothing underneath is allowed to be open.

I think that covers all the possibilities, so I don't expect there to be any issues. However, @CyrusNajmabadi, please add a test with 3 generics used like this:
C<, delegate*<int>, List<>>

Copy link
Member Author

Choose a reason for hiding this comment

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

@333fred tests added. test results seemed sensible to me, so i'm moving forward with submitting this. tnx.

{
_seenConstructed = true;
Visit(node.ElementType);
}

public override void VisitNullableType(NullableTypeSyntax node)
{
_seenConstructed = true;
Visit(node.ElementType);
}
}
}
92 changes: 1 addition & 91 deletions src/Compilers/CSharp/Portable/Binder/TypeofBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,96 +32,6 @@ internal TypeofBinder(ExpressionSyntax typeExpression, Binder next)
}

protected override bool IsUnboundTypeAllowed(GenericNameSyntax syntax)
{
bool allowed;
return _allowedMap != null && _allowedMap.TryGetValue(syntax, out allowed) && allowed;
}

/// <summary>
/// This visitor walks over a type expression looking for open types.
/// Open types are allowed if an only if:
/// 1) There is no constructed generic type elsewhere in the visited syntax; and
/// 2) The open type is not used as a type argument or array/pointer/nullable
/// element type.
/// </summary>
private class OpenTypeVisitor : CSharpSyntaxVisitor
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to sibling type so it could be shared.

{
private Dictionary<GenericNameSyntax, bool> _allowedMap;
private bool _seenConstructed;

/// <param name="typeSyntax">The argument to typeof.</param>
/// <param name="allowedMap">
/// Keys are GenericNameSyntax nodes representing unbound generic types.
/// Values are false if the node should result in an error and true otherwise.
/// </param>
public static void Visit(ExpressionSyntax typeSyntax, out Dictionary<GenericNameSyntax, bool> allowedMap)
{
OpenTypeVisitor visitor = new OpenTypeVisitor();
visitor.Visit(typeSyntax);
allowedMap = visitor._allowedMap;
}

public override void VisitGenericName(GenericNameSyntax node)
{
SeparatedSyntaxList<TypeSyntax> typeArguments = node.TypeArgumentList.Arguments;
if (node.IsUnboundGenericName)
{
if (_allowedMap == null)
{
_allowedMap = new Dictionary<GenericNameSyntax, bool>();
}
_allowedMap[node] = !_seenConstructed;
}
else
{
_seenConstructed = true;
foreach (TypeSyntax arg in typeArguments)
{
Visit(arg);
}
}
}

public override void VisitQualifiedName(QualifiedNameSyntax node)
{
bool seenConstructedBeforeRight = _seenConstructed;

// Visit Right first because it's smaller (to make backtracking cheaper).
Visit(node.Right);

bool seenConstructedBeforeLeft = _seenConstructed;

Visit(node.Left);

// If the first time we saw a constructed type was in Left, then we need to re-visit Right
if (!seenConstructedBeforeRight && !seenConstructedBeforeLeft && _seenConstructed)
{
Visit(node.Right);
}
}

public override void VisitAliasQualifiedName(AliasQualifiedNameSyntax node)
{
Visit(node.Name);
}

public override void VisitArrayType(ArrayTypeSyntax node)
{
_seenConstructed = true;
Visit(node.ElementType);
}

public override void VisitPointerType(PointerTypeSyntax node)
{
_seenConstructed = true;
Visit(node.ElementType);
}

public override void VisitNullableType(NullableTypeSyntax node)
{
_seenConstructed = true;
Visit(node.ElementType);
}
}
=> _allowedMap != null && _allowedMap.TryGetValue(syntax, out bool allowed) && allowed;
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}
}
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -4932,6 +4932,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureNameof" xml:space="preserve">
<value>nameof operator</value>
</data>
<data name="IDS_FeatureUnboundGenericTypesInNameof" xml:space="preserve">
<value>Unbound generic types in nameof operator</value>
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="IDS_FeatureDictionaryInitializer" xml:space="preserve">
<value>dictionary initializer</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ internal enum MessageID
IDS_OverloadResolutionPriority = MessageBase + 12848,

IDS_FeatureFirstClassSpan = MessageBase + 12849,
IDS_FeatureUnboundGenericTypesInNameof = MessageBase + 12850,
jcouv marked this conversation as resolved.
Show resolved Hide resolved
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -476,6 +477,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
// C# preview features.
case MessageID.IDS_FeatureFieldKeyword:
case MessageID.IDS_FeatureFirstClassSpan:
case MessageID.IDS_FeatureUnboundGenericTypesInNameof:
return LanguageVersion.Preview;

// C# 13.0 features.
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6052,6 +6052,15 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
return result;
}

// Allow for any chain of errant commas in the generic name. like `Dictionary<,int>` or
// `Dictionary<int,,>` We still want to think of these as generics, just with missing type-arguments, vs
// some invalid tree-expression that we would otherwise form.
if (this.CurrentToken.Kind == SyntaxKind.CommaToken)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
lastScannedType = default;
continue;
}

lastScannedType = this.ScanType(out _);
switch (lastScannedType)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading