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

Entrypoint detection stages #19093

Merged
merged 18 commits into from
May 10, 2017
Merged
Show file tree
Hide file tree
Changes from 11 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
9 changes: 6 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2060,28 +2060,31 @@ private AssemblySymbol GetForwardedToAssembly(string fullName, int arity, Diagno
return null;
}

internal static void CheckFeatureAvailability(SyntaxNode syntax, MessageID feature, DiagnosticBag diagnostics, Location locationOpt = null)
internal static bool CheckFeatureAvailability(SyntaxNode syntax, MessageID feature, DiagnosticBag diagnostics, Location locationOpt = null)
{
var options = (CSharpParseOptions)syntax.SyntaxTree.Options;
if (options.IsFeatureEnabled(feature))
{
return;
return true;
}

var location = locationOpt ?? syntax.GetLocation();
string requiredFeature = feature.RequiredFeature();
if (requiredFeature != null)
{
diagnostics.Add(ErrorCode.ERR_FeatureIsExperimental, location, feature.Localize(), requiredFeature);
return;
return false;
}

LanguageVersion availableVersion = options.LanguageVersion;
LanguageVersion requiredVersion = feature.RequiredVersion();
if (requiredVersion > availableVersion)
{
diagnostics.Add(availableVersion.GetErrorCode(), location, feature.Localize(), new CSharpRequiredLanguageVersion(requiredVersion));
return false;
}

return true;
}
}
}
29 changes: 21 additions & 8 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

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

6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -3596,9 +3596,6 @@ Give the compiler some way to differentiate the methods. For example, you can gi
<data name="ERR_SecurityCriticalOrSecuritySafeCriticalOnAsyncInClassOrStruct" xml:space="preserve">
<value>Async methods are not allowed in an Interface, Class, or Structure which has the 'SecurityCritical' or 'SecuritySafeCritical' attribute.</value>
</data>
<data name="ERR_MainCantBeAsync" xml:space="preserve">
<value>'{0}': an entry point cannot be marked with the 'async' modifier</value>
</data>
<data name="ERR_BadAwaitInQuery" xml:space="preserve">
<value>The 'await' operator may only be used in a query expression within the first collection expression of the initial 'from' clause or within the collection expression of a 'join' clause</value>
</data>
Expand Down Expand Up @@ -5074,4 +5071,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_VoidInTuple" xml:space="preserve">
<value>A tuple may not contain a value of type 'void'.</value>
</data>
<data name="ERR_NonTaskMainCantBeAsync" xml:space="preserve">
<value>A void or int returning entry point cannot be async</value>
</data>
</root>
126 changes: 95 additions & 31 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1453,42 +1453,103 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
}
}

DiagnosticBag warnings = DiagnosticBag.GetInstance();
var viableEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
foreach (var candidate in entryPointCandidates)
// Validity and diagnostics are also tracked because they must be conditionally handled
// if there are not any "traditional" entrypoints found.
var taskEntryPoints = ArrayBuilder<(bool IsValid, MethodSymbol Candidate, DiagnosticBag SpecificDiagnostics)>.GetInstance();
Copy link
Member

@cston cston May 2, 2017

Choose a reason for hiding this comment

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

intOrVoidEntryPoints and taskEntryPoints are not freed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// These diagnostics (warning only) are added to the compilation only if
// there were not any main methods found.
DiagnosticBag noMainFoundDiagnostics = DiagnosticBag.GetInstance();

bool CheckValid(MethodSymbol candidate, bool isCandidate, DiagnosticBag specificDiagnostics)
{
if (!HasEntryPointSignature(candidate, warnings))
if (!isCandidate)
{
// a single error for partial methods
warnings.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
continue;
noMainFoundDiagnostics.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
noMainFoundDiagnostics.AddRange(specificDiagnostics);
return false;
}

if (candidate.IsGenericMethod || candidate.ContainingType.IsGenericType)
{
// a single error for partial methods:
warnings.Add(ErrorCode.WRN_MainCantBeGeneric, candidate.Locations.First(), candidate);
continue;
noMainFoundDiagnostics.Add(ErrorCode.WRN_MainCantBeGeneric, candidate.Locations.First(), candidate);
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps check IsTaskLike first. That would simplify the IsCandidate check and perCandidateBag could be freed in one place rather than two.

if (IsTaskLike)
{
    taskEntryPoints.Add(...);
}
else
{
    if (IsCandidate)
    {
        intOrVoidEntryPoints.Add(candidate);
    }
    else
    {
        noMainFoundDiagnostics.Add(...);
    }
    perCandidatesBag.Free();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good!

Done

return true;
}

var viableEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();

foreach (var candidate in entryPointCandidates)
{
var perCandidateBag = DiagnosticBag.GetInstance();
var (IsCandidate, IsTaskLike) = HasEntryPointSignature(candidate, perCandidateBag);

// PROTOTYPE(async-main): CheckFeatureAvailability should be called on non-async methods
// that return Task or Task<T>
if (candidate.IsAsync)
if (IsTaskLike)
{
// PROTOTYPE(async-main): Get the diagnostic to point to a smaller syntax piece.
// PROTOTYPE(async-main): Switch diagnostics around if the type is not Task or Task<T>
CheckFeatureAvailability(candidate.GetNonNullSyntaxNode(), MessageID.IDS_FeatureAsyncMain, diagnostics);
taskEntryPoints.Add((IsCandidate, candidate, perCandidateBag));
}
else
{
if (CheckValid(candidate, IsCandidate, perCandidateBag))
{
if (candidate.IsAsync)
{
diagnostics.Add(ErrorCode.ERR_NonTaskMainCantBeAsync, candidate.Locations.First(), candidate);
}
else
{
diagnostics.AddRange(perCandidateBag);
viableEntryPoints.Add(candidate);
Copy link
Member

@cston cston May 4, 2017

Choose a reason for hiding this comment

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

Assert perCandidateBag is empty. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively, add diagnostics.AddRange(perCandidateBag);.

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 like diagnostics.AddRange(perCandidateBag);, I'm going with that one.

}
}
perCandidateBag.Free();
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2017

Choose a reason for hiding this comment

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

perCandidateBag [](start = 24, length = 15)

Please assert the bag is empty. #Closed

}
}

viableEntryPoints.Add(candidate);
if (viableEntryPoints.Count == 0)
{
foreach (var (IsValid, Candidate, SpecificDiagnostics) in taskEntryPoints)
{
// PROTOTYPE(async-main): Get the diagnostic to point to a smaller syntax piece.
if (CheckValid(Candidate, IsValid, SpecificDiagnostics) &&
CheckFeatureAvailability(Candidate.GetNonNullSyntaxNode(), MessageID.IDS_FeatureAsyncMain, diagnostics))
{
diagnostics.AddRange(SpecificDiagnostics);
viableEntryPoints.Add(Candidate);
}
}
}

if ((object)mainType == null || viableEntryPoints.Count == 0)
foreach (var (_, _, SpecificDiagnostics) in taskEntryPoints)
{
diagnostics.AddRange(warnings);
SpecificDiagnostics.Free();
}

warnings.Free();
if (viableEntryPoints.Count == 0)
{
diagnostics.AddRange(noMainFoundDiagnostics);
}
else if ((object)mainType == null)
{
// Turns error diagnostics into warnings. The reason that Error diagnostics can end up in `noMainFoundDiagnostics` is when
// HasEntryPointSignature yields some Error Diagnostics when people implement Task or Task<T> incorrectly.
//
// We can't add those Errors to the general diagnostics bag because it would break previously-working programs.
// The fact that these warnings are not added when csc is invoked with /main is possibly a bug, and is tracked at
// https://github.com/dotnet/roslyn/issues/18964
foreach (var diagnostic in noMainFoundDiagnostics.AsEnumerable())
{
if (diagnostic.Severity == DiagnosticSeverity.Error) {
diagnostics.Add(diagnostic.WithSeverity(DiagnosticSeverity.Warning));
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2017

Choose a reason for hiding this comment

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

diagnostics.Add(diagnostic.WithSeverity(DiagnosticSeverity.Warning)); [](start = 28, length = 69)

I don't think changing severity on the diagnostics is the right thing to do. Also, why, for example, would we want to report an obsolete diagnostics associated with GetAwaiter for an entry point that isn't going to be used? Please add a test for this scenario. I still think that we should be reporting only specific warnings, which are reported in CheckValid. #Closed

}
else
{
diagnostics.Add(diagnostic);
}
}
}

MethodSymbol entryPoint = null;
if (viableEntryPoints.Count == 0)
Expand Down Expand Up @@ -1518,7 +1579,9 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
entryPoint = viableEntryPoints[0];
}

taskEntryPoints.Free();
viableEntryPoints.Free();
noMainFoundDiagnostics.Free();
return entryPoint;
}
finally
Expand Down Expand Up @@ -1561,15 +1624,16 @@ internal bool ReturnsAwaitableToVoidOrInt(MethodSymbol method, DiagnosticBag dia

/// <summary>
/// Checks if the method has an entry point compatible signature, i.e.
/// - the return type is either void, int, <see cref="System.Threading.Tasks.Task" />,
/// or <see cref="System.Threading.Tasks.Task{T}" /> where T is an int.
/// - the return type is either void, int, or returns a <see cref="System.Threading.Tasks.Task" />,
/// or <see cref="System.Threading.Tasks.Task{T}" /> where the return type of GetAwaiter().GetResult()
/// is either void or int.
/// - has either no parameter or a single parameter of type string[]
/// </summary>
private bool HasEntryPointSignature(MethodSymbol method, DiagnosticBag bag)
private (bool IsCandidate, bool IsTaskLike) HasEntryPointSignature(MethodSymbol method, DiagnosticBag bag)
{
if (method.IsVararg)
{
return false;
return (false, false);
}

TypeSymbol returnType = method.ReturnType;
Expand All @@ -1580,45 +1644,45 @@ private bool HasEntryPointSignature(MethodSymbol method, DiagnosticBag bag)
returnsTaskOrTaskOfInt = ReturnsAwaitableToVoidOrInt(method, bag);
if (!returnsTaskOrTaskOfInt)
{
return false;
return (false, false);
}
}

// Prior to 7.1, async methods were considered to "have the entrypoint signature".
// In order to keep back-compat, we need to let these through if compiling using a previous language version.
if (!returnsTaskOrTaskOfInt && method.IsAsync && this.LanguageVersion >= LanguageVersion.CSharp7_1)
{
return false;
return (false, returnsTaskOrTaskOfInt);
}

if (method.RefKind != RefKind.None)
{
return false;
return (false, returnsTaskOrTaskOfInt);
}

if (method.Parameters.Length == 0)
{
return true;
return (true, returnsTaskOrTaskOfInt);
}

if (method.Parameters.Length > 1)
{
return false;
return (false, returnsTaskOrTaskOfInt);
}

if (!method.ParameterRefKinds.IsDefault)
{
return false;
return (false, returnsTaskOrTaskOfInt);
}

var firstType = method.Parameters[0].Type;
if (firstType.TypeKind != TypeKind.Array)
{
return false;
return (false, returnsTaskOrTaskOfInt);
}

var array = (ArrayTypeSymbol)firstType;
return array.IsSZArray && array.ElementType.SpecialType == SpecialType.System_String;
return (array.IsSZArray && array.ElementType.SpecialType == SpecialType.System_String, returnsTaskOrTaskOfInt);
}

internal override bool IsUnreferencedAssemblyIdentityDiagnosticCode(int code)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ internal enum ErrorCode
ERR_VarargsAsync = 4006,
ERR_ByRefTypeAndAwait = 4007,
ERR_BadAwaitArgVoidCall = 4008,
// ERR_MainCantBeAsync = 4009,
Copy link
Member

Choose a reason for hiding this comment

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

// ERR_MainCantBeAsync = 400 [](start = 6, length = 30)

Why comment out instead of delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seemed like the convention. Should it be removed?

Copy link
Member

Choose a reason for hiding this comment

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

The convention, at least as well as I can surmise, is to comment out when there is a gap in the ordering. For instance ERR_BadAwaitWithoutAsyncAnonMeth (20-ish lines down). It was removed but the error slot 4035 still exists. In this case we're replacing an error hence 4009 is not a gap anymore.

This convention makes it easy to find places to put new errors. Vs having to scan through te file looking for subtle changes in the number.


In reply to: 114816031 [](ancestors = 114816031)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (as discussed in person)

ERR_NonTaskMainCantBeAsync = 4009,
ERR_CantConvAsyncAnonFuncReturns = 4010,
ERR_BadAwaiterPattern = 4011,
ERR_BadSpecialByRefLocal = 4012,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,6 @@ internal override BoundBlock CreateBody()
{
var syntax = _userMainReturnTypeSyntax;


if (ReturnsVoid)
{
return new BoundBlock(
Expand Down
Loading