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

VB: Recognize and check an unmanaged constraint #75665

Merged
merged 5 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 5 additions & 57 deletions src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
Expand Down Expand Up @@ -122,7 +123,9 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet<Symbol> p
/// </summary>
internal static ManagedKind GetManagedKind(NamedTypeSymbol type, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
var (isManaged, hasGenerics) = IsManagedTypeHelper(type);
// The code below should be kept in sync with NamedTypeSymbol.GetManagedKind in VB

var (isManaged, hasGenerics) = INamedTypeSymbolInternal.Helpers.IsManagedTypeHelper(type);
var definitelyManaged = isManaged == ThreeState.True;
if (isManaged == ThreeState.Unknown)
{
Expand Down Expand Up @@ -201,7 +204,7 @@ internal static ManagedKind GetManagedKind(NamedTypeSymbol type, ref CompoundUse
}
else
{
var result = IsManagedTypeHelper(fieldNamedType);
var result = INamedTypeSymbolInternal.Helpers.IsManagedTypeHelper(fieldNamedType);
hasGenerics = hasGenerics || result.hasGenerics;
// NOTE: don't use ManagedKind.get on a NamedTypeSymbol - that could lead
// to infinite recursion.
Expand Down Expand Up @@ -235,60 +238,5 @@ internal static ManagedKind GetManagedKind(NamedTypeSymbol type, ref CompoundUse

internal static TypeSymbol NonPointerType(this FieldSymbol field) =>
field.HasPointerType ? null : field.Type;

/// <summary>
/// Returns True or False if we can determine whether the type is managed
/// without looking at its fields and Unknown otherwise.
/// Also returns whether or not the given type is generic.
/// </summary>
private static (ThreeState isManaged, bool hasGenerics) IsManagedTypeHelper(NamedTypeSymbol type)
{
// To match dev10, we treat enums as their underlying types.
if (type.IsEnumType())
{
type = type.GetEnumUnderlyingType();
}

// Short-circuit common cases.
switch (type.SpecialType)
{
case SpecialType.System_Void:
case SpecialType.System_Boolean:
case SpecialType.System_Char:
case SpecialType.System_SByte:
case SpecialType.System_Byte:
case SpecialType.System_Int16:
case SpecialType.System_UInt16:
case SpecialType.System_Int32:
case SpecialType.System_UInt32:
case SpecialType.System_Int64:
case SpecialType.System_UInt64:
case SpecialType.System_Decimal:
case SpecialType.System_Single:
case SpecialType.System_Double:
case SpecialType.System_IntPtr:
case SpecialType.System_UIntPtr:
case SpecialType.System_ArgIterator:
case SpecialType.System_RuntimeArgumentHandle:
return (ThreeState.False, false);
case SpecialType.System_TypedReference:
return (ThreeState.True, false);
case SpecialType.None:
default:
// CONSIDER: could provide cases for other common special types.
break; // Proceed with additional checks.
}

bool hasGenerics = type.IsGenericType;
switch (type.TypeKind)
{
case TypeKind.Enum:
return (ThreeState.False, hasGenerics);
case TypeKind.Struct:
return (ThreeState.Unknown, hasGenerics);
default:
return (ThreeState.True, hasGenerics);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes(ConsList<
}

// - presence of unmanaged pattern has to be matched with `valuetype`
// - IsUnmanagedAttribute is allowed iff there is an unmanaged pattern
// - IsUnmanagedAttribute is allowed if there is an unmanaged pattern
if (hasUnmanagedModreqPattern && (_flags & GenericParameterAttributes.NotNullableValueTypeConstraint) == 0 ||
hasUnmanagedModreqPattern != peModule.HasIsUnmanagedAttribute(_handle))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,70 @@ public static void Main()
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/75681")]
public void LoadingUnmanagedTypeModifier_ModreqGeneric()
{
var ilSource = IsUnmanagedAttributeIL + @"
.class public auto ansi beforefieldinit TestRef
extends [mscorlib]System.Object
{
.method public hidebysig instance void
M1<valuetype .ctor (class [mscorlib]System.ValueType modreq(System.Runtime.InteropServices.UnmanagedType`1)) T>() cil managed
{
.param type T
.custom instance void System.Runtime.CompilerServices.IsUnmanagedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 2 (0x2)
.maxstack 8
IL_0000: nop
IL_0001: ret
} // end of method TestRef::M1

.method public hidebysig specialname rtspecialname
instance void .ctor() cil managed
{
// Code size 8 (0x8)
.maxstack 8
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: nop
IL_0007: ret
} // end of method TestRef::.ctor
}

.class public auto ansi beforefieldinit System.Runtime.InteropServices.UnmanagedType`1<T>
extends [mscorlib]System.Object
{
.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
.maxstack 8

IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ret
}
}
";

var reference = CompileIL(ilSource, prependDefaultHeader: false);

var code = @"
public class Test
{
public static void Main()
{
var obj = new TestRef();

obj.M1<int>();
}
}";

CreateCompilation(code, references: new[] { reference }).VerifyDiagnostics(
// An error is expected here, see https://github.com/dotnet/roslyn/issues/75681
);
}

[Fact]
public void LoadingUnmanagedTypeModifier_AttributeWithoutModreq()
{
Expand Down
37 changes: 25 additions & 12 deletions src/Compilers/Core/Portable/Binding/UseSiteInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -270,14 +270,21 @@ public void AddDiagnostics(ICollection<DiagnosticInfo>? diagnostics)

foreach (var diagnosticInfo in diagnostics)
{
if (_diagnostics.Add(diagnosticInfo) && diagnosticInfo?.Severity == DiagnosticSeverity.Error)
{
RecordPresenceOfAnError();
}
AccumulateDiagnosticInfoAndRecordPresenceOfAnError(diagnosticInfo);
}
}
}

private void AccumulateDiagnosticInfoAndRecordPresenceOfAnError(DiagnosticInfo diagnosticInfo)
{
Debug.Assert(_diagnostics is not null);

if (_diagnostics.Add(diagnosticInfo) && diagnosticInfo?.Severity == DiagnosticSeverity.Error)
{
RecordPresenceOfAnError();
}
}

public void AddDiagnostics(IReadOnlyCollection<DiagnosticInfo>? diagnostics)
{
if (!AccumulatesDiagnostics)
Expand All @@ -291,10 +298,7 @@ public void AddDiagnostics(IReadOnlyCollection<DiagnosticInfo>? diagnostics)

foreach (var diagnosticInfo in diagnostics)
{
if (_diagnostics.Add(diagnosticInfo) && diagnosticInfo?.Severity == DiagnosticSeverity.Error)
{
RecordPresenceOfAnError();
}
AccumulateDiagnosticInfoAndRecordPresenceOfAnError(diagnosticInfo);
}
}
}
Expand All @@ -312,14 +316,23 @@ public void AddDiagnostics(ImmutableArray<DiagnosticInfo> diagnostics)

foreach (var diagnosticInfo in diagnostics)
{
if (_diagnostics.Add(diagnosticInfo) && diagnosticInfo?.Severity == DiagnosticSeverity.Error)
{
RecordPresenceOfAnError();
}
AccumulateDiagnosticInfoAndRecordPresenceOfAnError(diagnosticInfo);
}
}
}

public void AddDiagnosticInfo(DiagnosticInfo diagnosticInfo)
{
if (!AccumulatesDiagnostics)
{
return;
}

_diagnostics ??= new HashSet<DiagnosticInfo>();

AccumulateDiagnosticInfoAndRecordPresenceOfAnError(diagnosticInfo);
}

public void AddDependencies(UseSiteInfo<TAssemblySymbol> info)
{
if (!_hasErrors && AccumulatesDependencies)
Expand Down
65 changes: 65 additions & 0 deletions src/Compilers/Core/Portable/Symbols/INamedTypeSymbolInternal.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Diagnostics;

namespace Microsoft.CodeAnalysis.Symbols
{
Expand All @@ -16,5 +17,69 @@ internal interface INamedTypeSymbolInternal : ITypeSymbolInternal

ImmutableArray<ISymbolInternal> GetMembers();
ImmutableArray<ISymbolInternal> GetMembers(string name);

/// <summary>
/// True if this type or some containing type has type parameters.
/// </summary>
bool IsGenericType { get; }

internal static class Helpers
{
/// <summary>
/// Returns True or False if we can determine whether the type is managed
/// without looking at its fields and Unknown otherwise.
/// Also returns whether or not the given type is generic.
/// </summary>
public static (ThreeState isManaged, bool hasGenerics) IsManagedTypeHelper(INamedTypeSymbolInternal type)
Copy link
Member

@cston cston Oct 31, 2024

Choose a reason for hiding this comment

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

IsManagedTypeHelper

Consider use a top-level class and make this an extension method. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider use a top-level class and make this an extension method.

I prefer to keep the code as is. I do not foresee us using this helper much in the future. Therefore, in my opinion there is no good reason to "pollute" name lookup binding space with it. If we could use Default Interface Implementations feature in our code, I would make this method a member of INamedTypeSymbolInternal.

{
// To match dev10, we treat enums as their underlying types.
if (type.TypeKind == TypeKind.Enum)
{
Debug.Assert(type.EnumUnderlyingType is not null);
type = type.EnumUnderlyingType;
}

// Short-circuit common cases.
switch (type.SpecialType)
{
case SpecialType.System_Void:
case SpecialType.System_Boolean:
case SpecialType.System_Char:
case SpecialType.System_SByte:
case SpecialType.System_Byte:
case SpecialType.System_Int16:
case SpecialType.System_UInt16:
case SpecialType.System_Int32:
case SpecialType.System_UInt32:
case SpecialType.System_Int64:
case SpecialType.System_UInt64:
case SpecialType.System_Decimal:
case SpecialType.System_Single:
case SpecialType.System_Double:
case SpecialType.System_IntPtr:
case SpecialType.System_UIntPtr:
case SpecialType.System_ArgIterator:
case SpecialType.System_RuntimeArgumentHandle:
return (ThreeState.False, false);
case SpecialType.System_TypedReference:
return (ThreeState.True, false);
case SpecialType.None:
default:
// CONSIDER: could provide cases for other common special types.
break; // Proceed with additional checks.
}

bool hasGenerics = type.IsGenericType;
switch (type.TypeKind)
{
case TypeKind.Enum:
return (ThreeState.False, hasGenerics);
case TypeKind.Struct:
return (ThreeState.Unknown, hasGenerics);
default:
return (ThreeState.True, hasGenerics);
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public void BasicPathMapWindows(string filePath, string workingDirectory, string
""specifiedLanguageVersion"": ""VisualBasic15"",
""preprocessorSymbols"": {
""TARGET"": ""exe"",
""VBC_VER"": ""16.9""
""VBC_VER"": ""17.13""
}
}
}
Expand Down Expand Up @@ -360,7 +360,7 @@ public void FeatureFlag()
""specifiedLanguageVersion"": ""Default"",
""preprocessorSymbols"": {{
""TARGET"": ""library"",
""VBC_VER"": ""16.9"",
""VBC_VER"": ""17.13"",
""_MYTYPE"": ""Empty""
}}
}}
Expand All @@ -385,7 +385,7 @@ public void FeatureFlag()
""specifiedLanguageVersion"": ""Default"",
""preprocessorSymbols"": {{
""TARGET"": ""library"",
""VBC_VER"": ""16.9"",
""VBC_VER"": ""17.13"",
""_MYTYPE"": ""Empty""
}}
}}
Expand Down
9 changes: 7 additions & 2 deletions src/Compilers/Test/Utilities/VisualBasic/BasicTestSource.vb
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,17 @@ Public Structure BasicTestSource

Dim source = TryCast(Value, String)
If source IsNot Nothing Then
Return New SyntaxTree() {VisualBasicSyntaxTree.ParseText(SourceText.From(source, encoding:=Nothing, SourceHashAlgorithms.Default), parseOptions)}
Return New SyntaxTree() _
{
VisualBasicSyntaxTree.ParseText(
SourceText.From(source, encoding:=Nothing, SourceHashAlgorithms.Default),
If(parseOptions, TestOptions.RegularLatest))
}
End If

Dim sources = TryCast(Value, String())
If sources IsNot Nothing Then
Return sources.Select(Function(s) VisualBasicSyntaxTree.ParseText(SourceText.From(s, encoding:=Nothing, SourceHashAlgorithms.Default), parseOptions)).ToArray()
Return sources.Select(Function(s) VisualBasicSyntaxTree.ParseText(SourceText.From(s, encoding:=Nothing, SourceHashAlgorithms.Default), If(parseOptions, TestOptions.RegularLatest))).ToArray()
End If

Dim tree = TryCast(Value, SyntaxTree)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ Friend Module CompilationUtils
MarkupTestFile.GetSpans(codeWithMarker, codeWithoutMarker, spans)

Dim text = SourceText.From(codeWithoutMarker, Encoding.UTF8)
Return (VisualBasicSyntaxTree.ParseText(text, parseOptions, If(programElement.@name, "")), spans)
Return (VisualBasicSyntaxTree.ParseText(text, If(parseOptions, TestOptions.RegularLatest), If(programElement.@name, "")), spans)
End Function

' Find a node inside a tree.
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Test/Utilities/VisualBasic/Extensions.vb
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,12 @@ Friend Module Extensions

<Extension>
Friend Function ReduceExtensionMethod(this As MethodSymbol, instanceType As TypeSymbol) As MethodSymbol
Return this.ReduceExtensionMethod(instanceType, CompoundUseSiteInfo(Of AssemblySymbol).Discarded)
Return this.ReduceExtensionMethod(instanceType, CompoundUseSiteInfo(Of AssemblySymbol).Discarded, LanguageVersion.Latest)
End Function

<Extension>
Friend Function ReduceExtensionMethod(this As MethodSymbol, instanceType As TypeSymbol, proximity As Integer) As MethodSymbol
Return this.ReduceExtensionMethod(instanceType, proximity, CompoundUseSiteInfo(Of AssemblySymbol).Discarded)
Return this.ReduceExtensionMethod(instanceType, proximity, CompoundUseSiteInfo(Of AssemblySymbol).Discarded, LanguageVersion.Latest)
End Function

<Extension>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ Friend Module ParserTestUtilities
encoding = Encoding.UTF8
End If

Dim tree = VisualBasicSyntaxTree.ParseText(SourceText.From(source, encoding), options:=If(options, VisualBasicParseOptions.Default), path:=fileName)
Dim tree = VisualBasicSyntaxTree.ParseText(SourceText.From(source, encoding), options:=If(options, TestOptions.RegularLatest), path:=fileName)
Dim root = tree.GetRoot()
' Verify FullText
Assert.Equal(source, root.ToFullString)
Expand Down
Loading
Loading