Skip to content
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
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2493,7 +2493,8 @@ protected AssemblySymbol GetForwardedToAssembly(string name, int arity, ref Name

// NOTE: This won't work if the type isn't using CLS-style generic naming (i.e. `arity), but this code is
// only intended to improve diagnostic messages, so false negatives in corner cases aren't a big deal.
var metadataName = MetadataHelpers.ComposeAritySuffixedMetadataName(name, arity);
// File types can't be forwarded, so we won't attempt to determine a file identifier to attach to the metadata name.
var metadataName = MetadataHelpers.ComposeAritySuffixedMetadataName(name, arity, associatedFileIdentifier: null);
var fullMetadataName = MetadataHelpers.BuildQualifiedName(qualifierOpt?.ToDisplayString(SymbolDisplayFormat.QualifiedNameOnlyFormat), metadataName);
var result = GetForwardedToAssembly(fullMetadataName, diagnostics, location);
if ((object)result != null)
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7112,6 +7112,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_FileTypeBase" xml:space="preserve">
<value>File type '{0}' cannot be used as a base type of non-file type '{1}'.</value>
</data>
<data name="ERR_FileTypeNested" xml:space="preserve">
<value>File type '{0}' must be defined in a top level type; '{0}' is a nested type.</value>
</data>
<data name="IDS_FeatureUnsignedRightShift" xml:space="preserve">
<value>unsigned right shift</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ bool Cci.INamedTypeReference.MangleName
}
}

#nullable enable
string? Cci.INamedTypeReference.AssociatedFileIdentifier
{
get
{
return UnderlyingNamedType.AssociatedFileIdentifier();
}
}
#nullable disable

string Cci.INamedEntity.Name
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,16 @@ bool Cci.INamedTypeReference.MangleName
}
}

#nullable enable
string? Cci.INamedTypeReference.AssociatedFileIdentifier
{
get
{
return AdaptedNamedTypeSymbol.AssociatedFileIdentifier();
}
}
#nullable disable

string Cci.INamedEntity.Name
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ private void ReportExportedTypeNameCollisions(ImmutableArray<Cci.ExportedType> e
// exported types are not emitted in EnC deltas (hence generation 0):
string fullEmittedName = MetadataHelpers.BuildQualifiedName(
((Cci.INamespaceTypeReference)type.GetCciAdapter()).NamespaceName,
Cci.MetadataWriter.GetMangledName(type.GetCciAdapter(), generation: 0));
Cci.MetadataWriter.GetMetadataName(type.GetCciAdapter(), generation: 0));

// First check against types declared in the primary module
if (ContainsTopLevelType(fullEmittedName))
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2095,6 +2095,7 @@ internal enum ErrorCode
ERR_FileTypeDisallowedInSignature = 9300,
ERR_FileTypeNoExplicitAccessibility = 9301,
ERR_FileTypeBase = 9302,
ERR_FileTypeNested = 9303,

#endregion

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.SymbolDisplay;
using Roslyn.Utilities;

Expand Down Expand Up @@ -182,17 +183,28 @@ public override void VisitNamedType(INamedTypeSymbol symbol)
AddNullableAnnotations(symbol);

if ((format.CompilerInternalOptions & SymbolDisplayCompilerInternalOptions.IncludeContainingFileForFileTypes) != 0
// PROTOTYPE(ft): public API?
&& symbol.GetSymbol() is SourceMemberContainerTypeSymbol { IsFile: true } fileType)
&& symbol is Symbols.PublicModel.Symbol { UnderlyingSymbol: NamedTypeSymbol { AssociatedSyntaxTree: SyntaxTree tree } internalSymbol })
{
var tree = symbol.DeclaringSyntaxReferences[0].SyntaxTree;
var fileDescription = tree.FilePath is { Length: not 0 } path
? Path.GetFileNameWithoutExtension(path)
: $"<tree {fileType.DeclaringCompilation.SyntaxTrees.IndexOf(tree)}>";
var fileDescription = getDisplayFileName(tree) is { Length: not 0 } path
? path
: $"<tree {internalSymbol.DeclaringCompilation.SyntaxTrees.IndexOf(tree)}>";

builder.Add(CreatePart(SymbolDisplayPartKind.Punctuation, symbol, "@"));
builder.Add(CreatePart(SymbolDisplayPartKind.ModuleName, symbol, fileDescription));
}

static string getDisplayFileName(SyntaxTree tree)
{
if (tree.FilePath.Length == 0)
{
return "";
}

var pooledBuilder = PooledStringBuilder.GetInstance();
var sb = pooledBuilder.Builder;
GeneratedNames.AppendFileName(tree.FilePath, sb);
return pooledBuilder.ToStringAndFree();
}
}

private void VisitNamedTypeWithoutNullability(INamedTypeSymbol symbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ internal sealed override bool MangleName
get { return false; }
}

internal sealed override SyntaxTree? AssociatedSyntaxTree => null;

public sealed override int Arity
{
get { return 0; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ internal sealed override bool MangleName
get { return this.Arity > 0; }
}

internal sealed override SyntaxTree? AssociatedSyntaxTree => null;

internal sealed override ImmutableArray<TypeWithAnnotations> TypeArgumentsWithAnnotationsNoUseSiteDiagnostics
{
get { return GetTypeParametersAsTypeArguments(); }
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/ErrorTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,8 @@ internal override bool MangleName
get { return _originalDefinition.MangleName; }
}

internal override SyntaxTree? AssociatedSyntaxTree => _originalDefinition.AssociatedSyntaxTree;

internal override DiagnosticInfo? ErrorInfo
{
get { return _originalDefinition.ErrorInfo; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ internal override bool MangleName
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

public override Symbol? ContainingSymbol
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,8 @@ internal abstract override bool MangleName
get;
}

internal override SyntaxTree AssociatedSyntaxTree => null;

internal abstract int MetadataArity
{
get;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ internal override bool MangleName
return mangleName;
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

/// <summary>
/// Get the arity of the missing type.
/// </summary>
Expand Down
19 changes: 18 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -479,21 +479,38 @@ public virtual bool IsImplicitClass
/// </summary>
public abstract override string Name { get; }

#nullable enable
/// <summary>
/// Return the name including the metadata arity suffix.
/// </summary>
public override string MetadataName
{
get
{
return MangleName ? MetadataHelpers.ComposeAritySuffixedMetadataName(Name, Arity) : Name;
var fileIdentifier = this.AssociatedFileIdentifier();
// If we have a fileIdentifier, the type will definitely use CLS arity encoding for nonzero arity.
Debug.Assert(!(fileIdentifier != null && !MangleName && Arity > 0));
return fileIdentifier != null || MangleName
? MetadataHelpers.ComposeAritySuffixedMetadataName(Name, Arity, fileIdentifier)
: Name;
}
}

/// <summary>
/// If this type is a file type, returns the syntax tree where this type is visible. Otherwise, returns null.
/// </summary>
internal abstract SyntaxTree? AssociatedSyntaxTree { get; }
#nullable disable

/// <summary>
/// Should the name returned by Name property be mangled with [`arity] suffix in order to get metadata name.
/// Must return False for a type with Arity == 0.
/// </summary>
/// <remarks>
/// Some types with Arity > 0 still have MangleName == false. For example, EENamedTypeSymbol.
/// Note that other differences between source names and metadata names exist and are not controlled by this property,
/// such as the 'AssociatedFileIdentifier' prefix for file types.
/// </remarks>
internal abstract bool MangleName
{
// Intentionally no default implementation to force consideration of appropriate implementation for each new subclass
Expand Down
40 changes: 40 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/NamespaceOrTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@
// 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;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Symbols;
using Roslyn.Utilities;

Expand Down Expand Up @@ -243,6 +246,7 @@ internal virtual NamedTypeSymbol LookupMetadataType(ref MetadataTypeName emitted
Debug.Assert(!emittedTypeName.IsNull);

NamespaceOrTypeSymbol scope = this;
Debug.Assert(scope is not MergedNamespaceSymbol);

if (scope.Kind == SymbolKind.ErrorType)
{
Expand Down Expand Up @@ -326,6 +330,35 @@ internal virtual NamedTypeSymbol LookupMetadataType(ref MetadataTypeName emitted
}

Done:
if (isTopLevel
Copy link
Member

@jcouv jcouv Jun 28, 2022

Choose a reason for hiding this comment

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

I agree that handling lookup for error cases quickly gets complicated :-/ Not blocking this PR, but it would be good to check what Aleksey thinks on supporting "correct" behavior of public APIs in error cases. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that handling lookup for error cases quickly gets complicated :-/ Not blocking this PR, but it would be good to check what Aleksey thinks on supporting "correct" behavior of public APIs in error cases.

It is not clear what specific scenarios we are talking about. Could you please elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

@AlekseyTs The scenario is GetTypeByMetadataName on a nested file-type (class C { file class D { } }), ie. an error scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

The scenario is GetTypeByMetadataName on a nested file-type (``class C { file class D { } }```), ie. an error scenario.

I think in this case the file modifier on a nested type should be ignored and class D should be treated as a regular nested type. In other words, I think that the best implementation strategy here will be to not have nested file types in the symbol model, under no condition.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect. That's the approach Rikki took and it makes things simpler. Thanks

@RikkiGibson If we take that approach, let's verify the symbol for class D has !IsFile then.

Copy link
Member Author

@RikkiGibson RikkiGibson Jun 29, 2022

Choose a reason for hiding this comment

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

We're going to add an item to test plan and ensure the 'IsFile' API behaves as described here in a subsequent PR.

&& scope is not PENamespaceSymbol
&& (emittedTypeName.ForcedArity == -1 || emittedTypeName.ForcedArity == emittedTypeName.InferredArity)
Copy link
Contributor

Choose a reason for hiding this comment

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

&& (emittedTypeName.ForcedArity == -1 || emittedTypeName.ForcedArity == emittedTypeName.InferredArity)

It would be good to have a test backing this condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test plan item

&& GeneratedNameParser.TryParseFileTypeName(
emittedTypeName.UnmangledTypeName,
out string? displayFileName,
out int ordinal,
out string? sourceName))
{
// also do a lookup for file types from source.
namespaceOrTypeMembers = scope.GetTypeMembers(sourceName);
foreach (var named in namespaceOrTypeMembers)
{
if (named.AssociatedSyntaxTree is SyntaxTree tree
&& getDisplayName(tree) == displayFileName
&& named.DeclaringCompilation.GetSyntaxTreeOrdinal(tree) == ordinal
&& named.Arity == emittedTypeName.InferredArity)
{
if ((object?)namedType != null)
{
namedType = null;
break;
}

namedType = named;
}
}
}

if ((object?)namedType == null)
{
if (isTopLevel)
Expand All @@ -339,6 +372,13 @@ internal virtual NamedTypeSymbol LookupMetadataType(ref MetadataTypeName emitted
}

return namedType;

static string getDisplayName(SyntaxTree tree)
{
var sb = PooledStringBuilder.GetInstance();
GeneratedNames.AppendFileName(tree.FilePath, sb);
return sb.ToStringAndFree();
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ internal override bool MangleName
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

public AssemblySymbol EmbeddingAssembly
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ internal override bool MangleName
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

public NamedTypeSymbol UnderlyingSymbol
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ internal override bool MangleName
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

public string Guid
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ internal override bool MangleName
}
}

internal override SyntaxTree? AssociatedSyntaxTree => null;

internal override DiagnosticInfo? ErrorInfo
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -826,8 +826,7 @@ internal override ManagedKind GetManagedKind(ref CompoundUseSiteInfo<AssemblySym

internal bool IsFile => HasFlag(DeclarationModifiers.File);

/// <summary>If this symbol is only available within a single syntax tree, returns that syntax tree. Otherwise, returns null.</summary>
internal SyntaxTree? AssociatedSyntaxTree => IsFile ? declaration.Declarations[0].Location.SourceTree : null;
internal sealed override SyntaxTree? AssociatedSyntaxTree => IsFile ? declaration.Declarations[0].Location.SourceTree : null;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private bool HasFlag(DeclarationModifiers flag) => (_declModifiers & flag) != 0;
Expand Down Expand Up @@ -1743,6 +1742,11 @@ protected void AfterMembersChecks(BindingDiagnosticBag diagnostics)
}
}

if (IsFile && (object?)ContainingType != null)
{
diagnostics.Add(ErrorCode.ERR_FileTypeNested, location, this);
}

return;

bool hasBaseTypeOrInterface(Func<NamedTypeSymbol, bool> predicate)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ internal enum GeneratedNameKind
ReusableHoistedLocalField = '7',
LambdaCacheField = '9',
FixedBufferField = 'e',
FileType = 'F',
AnonymousType = 'f',
TransparentIdentifier = 'h',
AnonymousTypeField = 'i',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Text.RegularExpressions;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
Expand Down Expand Up @@ -170,5 +171,35 @@ internal static bool TryParseAnonymousTypeParameterName(string typeParameterName
propertyName = null;
return false;
}

// A full metadata name for a generic file type looks like:
// <ContainingFile>FN__ClassName`A
// where 'N' is the syntax tree ordinal, 'A' is the arity,
// and 'ClassName' is the source name of the type.
//
// The "unmangled" name of a generic file type looks like:
// <ContainingFile>FN__ClassName
private static readonly Regex s_fileTypeOrdinalPattern = new Regex(@"<([a-zA-Z_0-9]*)>F(\d)+__", RegexOptions.Compiled);

/// <remarks>
/// This method will work with either unmangled or mangled type names as input, but it does not remove any arity suffix if present.
/// </remarks>
internal static bool TryParseFileTypeName(string generatedName, [NotNullWhen(true)] out string? displayFileName, out int ordinal, [NotNullWhen(true)] out string? originalTypeName)
{
if (s_fileTypeOrdinalPattern.Match(generatedName) is Match { Success: true, Groups: var groups, Index: var index, Length: var length }
&& int.TryParse(groups[2].Value, out ordinal))
{
displayFileName = groups[1].Value;

var prefixEndsAt = index + length;
originalTypeName = generatedName.Substring(prefixEndsAt);
return true;
}

ordinal = -1;
displayFileName = null;
originalTypeName = null;
return false;
}
}
}
Loading