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 1 commit
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.

59 changes: 31 additions & 28 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema

<!--
Microsoft ResX Schema
Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -5074,4 +5074,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>
</root>
<data name="ERR_NonTaskMainCantBeAsync" xml:space="preserve">
<value>Async Main methods must return Task or Task&lt;int&gt;</value>
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

Task<int> [](start = 50, length = 15)

Does it have to be Task<int> though? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not technically, but I think this is fine for an error message.

I'd be willing to change it, but "Async Main methods must return either Task or Task whereby the return type of GetAwaiter().GetResult() is either void or int" seems a bit verbose for an error message.

Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

I think we can simply say that int or void returning Main cannot be async. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

+1 for "void returning Main cannot be async"


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.


In reply to: 114619543 [](ancestors = 114619543,114608904)

</data>
</root>
107 changes: 80 additions & 27 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1453,42 +1453,94 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
}
}

DiagnosticBag warnings = DiagnosticBag.GetInstance();
var viableEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
var intOrVoidEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
// 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();

foreach (var candidate in entryPointCandidates)
{
if (!HasEntryPointSignature(candidate, warnings))
var perCandidateBag = DiagnosticBag.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.

perCandidateBag is not freed. #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.

Done.

var (IsCandidate, IsTaskLike) = HasEntryPointSignature(candidate, perCandidateBag);

if (IsCandidate && !IsTaskLike)
{
// a single error for partial methods
warnings.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
continue;
intOrVoidEntryPoints.Add(candidate);
}
else if (IsTaskLike)
{
taskEntryPoints.Add((IsCandidate, candidate, perCandidateBag));
}
else
{
noMainFoundDiagnostics.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
noMainFoundDiagnostics.AddRange(perCandidateBag);
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.

perCandidateBag is not freed, in if or else.

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.

}
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

}

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

foreach (var candidate in intOrVoidEntryPoints)
{
if (candidate.IsGenericMethod || candidate.ContainingType.IsGenericType)
{
// a single error for partial methods:
warnings.Add(ErrorCode.WRN_MainCantBeGeneric, candidate.Locations.First(), candidate);
noMainFoundDiagnostics.Add(ErrorCode.WRN_MainCantBeGeneric, candidate.Locations.First(), candidate);
continue;
}

// PROTOTYPE(async-main): CheckFeatureAvailability should be called on non-async methods
// that return Task or Task<T>
if (candidate.IsAsync)
{
// 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);
diagnostics.Add(ErrorCode.ERR_NonTaskMainCantBeAsync, candidate.Locations.First(), candidate);
continue;
}

viableEntryPoints.Add(candidate);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could the body of this foreach be inlined in the previous foreach, when CheckValid() returns true? Then, intOrVoidEntryPoints can be removed.

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.


if ((object)mainType == null || viableEntryPoints.Count == 0)
if (viableEntryPoints.IsEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

Does IsEmpty require an IEnumerable? If so, consider using .Count == 0 instead.

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.

{
diagnostics.AddRange(warnings);
foreach (var (IsValid, Candidate, SpecificDiagnostics) in taskEntryPoints)
{
if (Candidate.IsGenericMethod || Candidate.ContainingType.IsGenericType)
{
// a single error for partial methods:
noMainFoundDiagnostics.Add(ErrorCode.WRN_MainCantBeGeneric, Candidate.Locations.First(), Candidate);
continue;
}

if (!IsValid)
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

if (!IsValid) [](start = 24, length = 13)

It feels like this if should be first in this loop (for consistency with int/void entry points). #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Can these two if blocks be extracted to a helper method used in both the non-task-like and task-like candidates? (It would mean performing both checks for non-task-like candidates in the same loop, perhaps the first foreach, so the candidate is never added to intOrVoidEntryPoints.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlekseyTs: Yep, that makes sense. Done.

@cston: Unless I'm misunderstanding you, I don't think that there would be much to gain from pulling this out into a local function and performing the check earlier would break the diagnostic guarantees

{
noMainFoundDiagnostics.Add(ErrorCode.WRN_InvalidMainSig, Candidate.Locations.First(), Candidate);
noMainFoundDiagnostics.AddRange(SpecificDiagnostics);
continue;
}

// PROTOTYPE(async-main): Get the diagnostic to point to a smaller syntax piece.
if (CheckFeatureAvailability(Candidate.GetNonNullSyntaxNode(), MessageID.IDS_FeatureAsyncMain, diagnostics))
{
diagnostics.AddRange(SpecificDiagnostics);
viableEntryPoints.Add(Candidate);
}
}
}

warnings.Free();
if (viableEntryPoints.Count == 0)
{
diagnostics.AddRange(noMainFoundDiagnostics);
}
else if ((object)mainType == null)
{
// Only add warning diagnostics. 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
diagnostics.AddRange(noMainFoundDiagnostics.AsEnumerable().Where(d => d.Severity == DiagnosticSeverity.Warning));
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

noMainFoundDiagnostics.AsEnumerable().Where(d => d.Severity == DiagnosticSeverity.Warning) [](start = 41, length = 90)

Linq is banned in compilers. Also, if the point of this code is to preserve old behavior, then we should probably be more precise and add only WRN_MainCantBeGeneric and WRN_InvalidMainSig warnings. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linq is banned in compilers.

Fixed.

Also, if the point of this code is to preserve old behavior

Nah, the point is the emit errors where there were errors before, and emit warnings where there were warnings before. I don't care about emitting the same ones because it's possible to add more helpful diagnostics now.

}

MethodSymbol entryPoint = null;
if (viableEntryPoints.Count == 0)
Expand Down Expand Up @@ -1519,6 +1571,7 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
}

viableEntryPoints.Free();
noMainFoundDiagnostics.Free();
return entryPoint;
}
finally
Expand Down Expand Up @@ -1565,11 +1618,11 @@ internal bool ReturnsAwaitableToVoidOrInt(MethodSymbol method, DiagnosticBag dia
/// or <see cref="System.Threading.Tasks.Task{T}" /> where T is an int.
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

where T is an int [](start = 61, length = 17)

It looks like this comment is no longer accurate. #Closed

/// - 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 +1633,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
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 @@ -1479,5 +1479,6 @@ internal enum ErrorCode
ERR_BadDynamicMethodArgDefaultLiteral = 9000,
ERR_DefaultLiteralNotValid = 9001,
WRN_DefaultInSwitch = 9002,
ERR_NonTaskMainCantBeAsync = 9003,
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

ERR_NonTaskMainCantBeAsync [](start = 8, length = 26)

Can we simply reuse ERR_MainCantBeAsync slot? #Closed

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 wasn't sure about this. Are error numbers relevant to back-compat?

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'm reusing the old number, but with a new name and message.

}
}
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