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

File types binding #60977

Merged
merged 19 commits into from
May 28, 2022
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/BinderFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ internal BinderFactory(CSharpCompilation compilation, SyntaxTree syntaxTree, boo
// more than 50 items added before getting collected.
_binderCache = new ConcurrentCache<BinderCacheKey, Binder>(50);

_buckStopsHereBinder = new BuckStopsHereBinder(compilation);
_buckStopsHereBinder = new BuckStopsHereBinder(compilation, syntaxTree);
}

internal SyntaxTree SyntaxTree
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,12 @@ private static void CheckConstraintTypeVisibility(
// "Inconsistent accessibility: constraint type '{1}' is less accessible than '{0}'"
diagnostics.Add(ErrorCode.ERR_BadVisBound, location, containingSymbol, constraintType.Type);
}

if (constraintType.Type.IsFileTypeOrUsesFileTypes() && !containingSymbol.ContainingType.IsFileTypeOrUsesFileTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, location, constraintType.Type, containingSymbol);
}

diagnostics.Add(location, useSiteInfo);
}

Expand Down
42 changes: 41 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,6 +1306,38 @@ internal static ImmutableArray<Symbol> GetCandidateMembers(NamespaceOrTypeSymbol
}
}

private bool IsInScopeOfAssociatedSyntaxTree(Symbol symbol)
{
while (symbol is not null and not SourceMemberContainerTypeSymbol { IsFile: true })
{
symbol = symbol.ContainingType;
}

if (symbol is null)
{
// the passed-in symbol was not contained in a file type.
return true;
}

var tree = getSyntaxTreeForFileTypes();
return symbol.IsDefinedInSourceTree(tree, definedWithinSpan: null);

SyntaxTree getSyntaxTreeForFileTypes()
{
for (var binder = this; binder != null; binder = binder.Next)
{
if (binder is BuckStopsHereBinder lastBinder)
{
Debug.Assert(lastBinder.AssociatedSyntaxTree is not null);
return lastBinder.AssociatedSyntaxTree;
}
}

Debug.Assert(false);
return null;
}
}

/// <remarks>
/// Distinguish from <see cref="CanAddLookupSymbolInfo"/>, which performs an analogous task for Add*LookupSymbolsInfo*.
/// </remarks>
Expand All @@ -1322,8 +1354,12 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio
? ((AliasSymbol)symbol).GetAliasTarget(basesBeingResolved)
: symbol;

if (!IsInScopeOfAssociatedSyntaxTree(unwrappedSymbol))
{
return LookupResult.Empty();
}
// Check for symbols marked with 'Microsoft.CodeAnalysis.Embedded' attribute
if (!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) && unwrappedSymbol.IsHiddenByCodeAnalysisEmbeddedAttribute())
else if (!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) && unwrappedSymbol.IsHiddenByCodeAnalysisEmbeddedAttribute())
{
return LookupResult.Empty();
}
Expand Down Expand Up @@ -1522,6 +1558,10 @@ internal bool CanAddLookupSymbolInfo(Symbol symbol, LookupOptions options, Looku
{
return false;
}
else if (!IsInScopeOfAssociatedSyntaxTree(symbol))
Copy link
Member

Choose a reason for hiding this comment

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

IsInScopeOfAssociatedSyntaxTree

Isn't this covered by the previous else if? Specifically, won't this.IsAccessible(symbol, ...) handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we're treating this as a "parallel" concept to accessibility. So IsAccessible, IsAtLeastAsVisibleAs, etc. don't consider file types.

{
return false;
}
else if ((options & LookupOptions.MustBeInstance) != 0 && !IsInstance(symbol))
{
return false;
Expand Down
23 changes: 23 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1816,6 +1816,15 @@ Symbol resultSymbol(
Debug.Assert(!Symbol.Equals(first, second, TypeCompareKind.ConsiderEverything) || !Symbol.Equals(originalSymbols[best.Index], originalSymbols[secondBest.Index], TypeCompareKind.ConsiderEverything),
"Why does the LookupResult contain the same symbol twice?");

if (best.IsFromFile && !secondBest.IsFromFile)
{
// a lookup of a file type is "better" than a lookup of a non-file type; no need to further diagnose
// PROTOTYPE(ft): some "single symbol" diagnostics are missed here for similar reasons
// that make us miss diagnostics when reporting WRN_SameFullNameThisAggAgg.
//
return first;
}

CSDiagnosticInfo info;
bool reportError;

Expand Down Expand Up @@ -2133,6 +2142,7 @@ private static AssemblySymbol GetContainingAssembly(Symbol symbol)
private enum BestSymbolLocation
{
None,
FromFile,
FromSourceModule,
FromAddedModule,
FromReferencedAssembly,
Expand Down Expand Up @@ -2180,6 +2190,14 @@ public bool IsFromCompilation
}
}

public bool IsFromFile
{
get
{
return _location == BestSymbolLocation.FromFile;
}
}

public bool IsNone
{
get
Expand Down Expand Up @@ -2281,6 +2299,11 @@ private BestSymbolInfo GetBestSymbolInfo(ArrayBuilder<Symbol> symbols, out BestS

private static BestSymbolLocation GetLocation(CSharpCompilation compilation, Symbol symbol)
{
if (symbol is SourceMemberContainerTypeSymbol { IsFile: true })
Copy link
Member

Choose a reason for hiding this comment

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

is SourceMemberContainerTypeSymbol { IsFile: true }

Will we get here with a member of a file type? If so, should BestSymbolLocation.FromFile be returned in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we might, in some cases where the member access is not qualified. The only relevant case I can think of, though, involves using static.

// File1.cs
using static C.D;

M();

file class C
{
    public class D
    {
        public static void M() { }
    }
}

// File2.cs
class C
{
    public class D
    {
        public static void M() { }
    }
}

I'll add the test. Let me know if any other scenarios come to mind that might hit this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this test and it "just worked". I suspect this is because we "broke the tie" when binding the using static C.D, and binding the call is just a simple lookup within that.

{
return BestSymbolLocation.FromFile;
}

var containingAssembly = symbol.ContainingAssembly;
if (containingAssembly == compilation.SourceAssembly)
{
Expand Down
12 changes: 11 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/BuckStopsHereBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,21 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal class BuckStopsHereBinder : Binder
{
internal BuckStopsHereBinder(CSharpCompilation compilation)
internal BuckStopsHereBinder(CSharpCompilation compilation, SyntaxTree? associatedSyntaxTree)
: base(compilation)
{
this.AssociatedSyntaxTree = associatedSyntaxTree;
}

/// <summary>
/// In non-speculative scenarios, the syntax tree being bound.
/// In speculative scenarios, the syntax tree from the original compilation used as the speculation context.
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
/// This is <see langword="null"/> in some scenarios, such as the binder used for <see cref="CSharpCompilation.Conversions" />
/// or the binder used to bind usings in <see cref="CSharpCompilation.UsingsFromOptionsAndDiagnostics"/>.
/// PROTOTYPE(ft): what about in EE scenarios?
/// </summary>
internal readonly SyntaxTree? AssociatedSyntaxTree;

internal override ImportChain? ImportChain
{
get
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7088,6 +7088,15 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExpressionTreeContainsUTF8StringLiterals" xml:space="preserve">
<value>An expression tree may not contain UTF8 string conversion or literal.</value>
</data>
<data name="ERR_FileTypeDisallowedInSignature" xml:space="preserve">
<value>File type '{0}' cannot be used in a member signature in non-file type '{1}'.</value>
</data>
<data name="ERR_FileTypeNoExplicitAccessibility" xml:space="preserve">
<value>File type '{0}' cannot use accessibility modifiers.</value>
</data>
<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="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 @@ -34,7 +34,9 @@ public static UsingsFromOptionsAndDiagnostics FromOptions(CSharpCompilation comp
}

var diagnostics = new DiagnosticBag();
var usingsBinder = new InContainerBinder(compilation.GlobalNamespace, new BuckStopsHereBinder(compilation));
// PROTOTYPE(ft): should usings from compilation options be allowed to bring file types into scope?
// it doesn't seem to work for non-file types within the compilaton, so can probably do the same for file types.
var usingsBinder = new InContainerBinder(compilation.GlobalNamespace, new BuckStopsHereBinder(compilation, associatedSyntaxTree: null));
var boundUsings = ArrayBuilder<NamespaceOrTypeAndUsingDirective>.GetInstance();
var uniqueUsings = PooledHashSet<NamespaceOrTypeSymbol>.GetInstance();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ internal Conversions Conversions
{
if (_conversions == null)
{
Interlocked.CompareExchange(ref _conversions, new BuckStopsHereBinder(this).Conversions, null);
Interlocked.CompareExchange(ref _conversions, new BuckStopsHereBinder(this, associatedSyntaxTree: null).Conversions, null);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}

return _conversions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ private void BindName(XAttribute attribute, CSharpSyntaxNode originatingSyntax,
"Why are we processing a documentation comment that is not attached to a member declaration?");

var nameDiagnostics = BindingDiagnosticBag.GetInstance(_diagnostics);
Binder binder = MakeNameBinder(isParameter, isTypeParameterRef, _memberSymbol, _compilation);
Binder binder = MakeNameBinder(isParameter, isTypeParameterRef, _memberSymbol, _compilation, originatingSyntax.SyntaxTree);
DocumentationCommentCompiler.BindName(attrSyntax, binder, _memberSymbol, ref _documentedParameters, ref _documentedTypeParameters, nameDiagnostics);
RecordBindingDiagnostics(nameDiagnostics, sourceLocation); // Respects DocumentationMode.
nameDiagnostics.Free();
Expand All @@ -545,9 +545,9 @@ private void BindName(XAttribute attribute, CSharpSyntaxNode originatingSyntax,

// NOTE: We're not sharing code with the BinderFactory visitor, because we already have the
// member symbol in hand, which makes things much easier.
private static Binder MakeNameBinder(bool isParameter, bool isTypeParameterRef, Symbol memberSymbol, CSharpCompilation compilation)
private static Binder MakeNameBinder(bool isParameter, bool isTypeParameterRef, Symbol memberSymbol, CSharpCompilation compilation, SyntaxTree syntaxTree)
{
Binder binder = new BuckStopsHereBinder(compilation);
Binder binder = new BuckStopsHereBinder(compilation, syntaxTree);

// All binders should have a containing symbol.
Symbol containingSymbol = memberSymbol.ContainingSymbol;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,14 @@ public bool Equals(TypeDeclarationIdentity other)
return false;
}

if ((object)thisDecl.Location.SourceTree != otherDecl.Location.SourceTree
&& ((thisDecl.Modifiers & DeclarationModifiers.File) != 0
|| (otherDecl.Modifiers & DeclarationModifiers.File) != 0))
{
// declarations of 'file' types are only the same type if they are in the same file
return false;
}

if (thisDecl._kind == DeclarationKind.Enum || thisDecl._kind == DeclarationKind.Delegate)
{
// oh, so close, but enums and delegates cannot be partial
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2069,6 +2069,11 @@ internal enum ErrorCode
ERR_CannotBeConvertedToUTF8 = 9026,
ERR_ExpressionTreeContainsUTF8StringLiterals = 9027,

// PROTOTYPE(ft): compress these before feature merge
ERR_FileTypeDisallowedInSignature = 9300,
ERR_FileTypeNoExplicitAccessibility = 9301,
ERR_FileTypeBase = 9302,

#endregion

// Note: you will need to re-generate compiler code after adding warnings (eng\generate-compiler-code.cmd)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ internal BoundExpression MakeInvocationExpression(
private sealed class SyntheticBinderImpl : BuckStopsHereBinder
{
private readonly SyntheticBoundNodeFactory _factory;
internal SyntheticBinderImpl(SyntheticBoundNodeFactory factory) : base(factory.Compilation)
internal SyntheticBinderImpl(SyntheticBoundNodeFactory factory) : base(factory.Compilation, associatedSyntaxTree: null)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
_factory = factory;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

using System.Collections.Immutable;
using System.Diagnostics;
using System.IO;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand Down Expand Up @@ -179,6 +180,19 @@ public override void VisitNamedType(INamedTypeSymbol symbol)
{
VisitNamedTypeWithoutNullability(symbol);
AddNullableAnnotations(symbol);

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

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

private void VisitNamedTypeWithoutNullability(INamedTypeSymbol symbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ protected sealed override void MethodChecks(BindingDiagnosticBag diagnostics)
}

this.CheckEffectiveAccessibility(_lazyReturnType, _lazyParameters, diagnostics);
this.CheckFileTypeUsage(_lazyReturnType, _lazyParameters, diagnostics);

if (_lazyIsVararg && (IsGenericMethod || ContainingType.IsGenericType || _lazyParameters.Length > 0 && _lazyParameters[_lazyParameters.Length - 1].IsParams))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ internal static void AddDelegateMembers(
diagnostics.Add(ErrorCode.ERR_BadVisDelegateReturn, delegateType.Locations[0], delegateType, invoke.ReturnType);
}

var delegateTypeIsFile = delegateType.IsFileTypeOrUsesFileTypes();
if (!delegateTypeIsFile && invoke.ReturnType.IsFileTypeOrUsesFileTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, delegateType.Locations[0], invoke.ReturnType, delegateType);
}

for (int i = 0; i < invoke.Parameters.Length; i++)
{
var parameterSymbol = invoke.Parameters[i];
Expand All @@ -106,6 +112,10 @@ internal static void AddDelegateMembers(
// Inconsistent accessibility: parameter type '{1}' is less accessible than delegate '{0}'
diagnostics.Add(ErrorCode.ERR_BadVisDelegateParam, delegateType.Locations[0], delegateType, parameterSymbol.Type);
}
else if (!delegateTypeIsFile && parameterSymbol.Type.IsFileTypeOrUsesFileTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, delegateType.Locations[0], parameterSymbol.Type, delegateType);
}
var parameterSyntax = syntax.ParameterList.Parameters[i];
if (parameterSyntax.ExclamationExclamationToken.Kind() == SyntaxKind.ExclamationExclamationToken)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,10 @@ private DeclarationModifiers MakeAndCheckTypeModifiers(
{
result |= defaultAccess;
}
else if ((result & DeclarationModifiers.File) != 0)
{
diagnostics.Add(ErrorCode.ERR_FileTypeNoExplicitAccessibility, Locations[0], this);
}

if (missingPartial)
{
Expand Down Expand Up @@ -776,6 +780,11 @@ internal override ManagedKind GetManagedKind(ref CompoundUseSiteInfo<AssemblySym

internal bool IsNew => HasFlag(DeclarationModifiers.New);

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;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private bool HasFlag(DeclarationModifiers flag) => (_declModifiers & flag) != 0;

Expand Down Expand Up @@ -1219,15 +1228,15 @@ private Dictionary<string, ImmutableArray<NamedTypeSymbol>> GetTypeMembersDictio
private Dictionary<string, ImmutableArray<NamedTypeSymbol>> MakeTypeMembers(BindingDiagnosticBag diagnostics)
{
var symbols = ArrayBuilder<NamedTypeSymbol>.GetInstance();
var conflictDict = new Dictionary<(string, int), SourceNamedTypeSymbol>();
var conflictDict = new Dictionary<(string name, int arity, SyntaxTree? syntaxTree), SourceNamedTypeSymbol>();
try
{
foreach (var childDeclaration in declaration.Children)
{
var t = new SourceNamedTypeSymbol(this, childDeclaration, diagnostics);
this.CheckMemberNameDistinctFromType(t, diagnostics);

var key = (t.Name, t.Arity);
var key = (t.Name, t.Arity, t.AssociatedSyntaxTree);
SourceNamedTypeSymbol? other;
if (conflictDict.TryGetValue(key, out other))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ protected sealed override DeclarationModifiers Modifiers

protected void TypeChecks(TypeSymbol type, BindingDiagnosticBag diagnostics)
{
if (type.IsStatic)
if (type.IsFileTypeOrUsesFileTypes() && !ContainingType.IsFileTypeOrUsesFileTypes())
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, this.ErrorLocation, type, ContainingType);
}
else if (type.IsStatic)
{
// Cannot declare a variable of static type '{0}'
diagnostics.Add(ErrorCode.ERR_VarDeclIsStaticClass, this.ErrorLocation, type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,27 @@ protected void CheckEffectiveAccessibility(TypeWithAnnotations returnType, Immut
diagnostics.Add(Locations[0], useSiteInfo);
}

protected void CheckFileTypeUsage(TypeWithAnnotations returnType, ImmutableArray<ParameterSymbol> parameters, BindingDiagnosticBag diagnostics)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
if (ContainingType.IsFileTypeOrUsesFileTypes())
{
return;
}

if (returnType.Type.IsFileTypeOrUsesFileTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Locations[0], returnType.Type, ContainingType);
}

foreach (var param in parameters)
{
if (param.Type.IsFileTypeOrUsesFileTypes())
{
diagnostics.Add(ErrorCode.ERR_FileTypeDisallowedInSignature, Locations[0], param.Type, ContainingType);
}
}
}

protected void MakeFlags(
MethodKind methodKind,
DeclarationModifiers declarationModifiers,
Expand Down
Loading