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
223 changes: 156 additions & 67 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1041,102 +1041,190 @@ private bool IsValidFixedVariableInitializer(TypeSymbol declType, SourceLocalSym
Debug.Assert(!ReferenceEquals(declType, null));
Debug.Assert(declType.IsPointerType());

if (ReferenceEquals(initializerOpt, null))
if (initializerOpt?.HasAnyErrors != false)
{
return false;
Copy link
Member

@jcouv jcouv Feb 12, 2018

Choose a reason for hiding this comment

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

Does a test cover this path? #Closed

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 a test


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

}

TypeSymbol initializerType = initializerOpt.Type;
SyntaxNode initializerSyntax = initializerOpt.Syntax;

if (ReferenceEquals(initializerType, null))
if ((object)initializerType == null)
Copy link
Member

@agocke agocke Feb 12, 2018

Choose a reason for hiding this comment

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

Maybe is null now? #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

== null is used throughout the file. I think we might want to fix that globally, but one change would just look odd.


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

{
initializerOpt = GenerateConversionForAssignment(declType, initializerOpt, diagnostics);
if (!initializerOpt.HasAnyErrors)
{
// Dev10 just reports the assignment conversion error, which must occur, except for these cases:
Debug.Assert(
initializerOpt is BoundConvertedStackAllocExpression ||
initializerOpt is BoundConversion conversion && (conversion.Operand.IsLiteralNull() || conversion.Operand.Kind == BoundKind.DefaultExpression),
"All other typeless expressions should have conversion errors");
Error(diagnostics, ErrorCode.ERR_ExprCannotBeFixed, initializerSyntax);
return false;
}

// CONSIDER: this is a very confusing error message, but it's what Dev10 reports.
Error(diagnostics, ErrorCode.ERR_FixedNotNeeded, initializerSyntax);
}
TypeSymbol elementType;
bool hasErrors = false;
MethodSymbol getPinnableMethod = null;

switch (initializerOpt.Kind)
{
case BoundKind.AddressOfOperator:
elementType = ((BoundAddressOfOperator)initializerOpt).Operand.Type;
break;

case BoundKind.FieldAccess:
var fa = (BoundFieldAccess)initializerOpt;
if (fa.FieldSymbol.IsFixed)
{
elementType = ((PointerTypeSymbol)fa.Type).PointedAtType;
break;
}

goto default;

default:
// fixed (T* variable = <expr>) ...

// check for arrays
if (initializerType.IsArray())
{
// See ExpressionBinder::BindPtrToArray (though most of that functionality is now in LocalRewriter).
elementType = ((ArrayTypeSymbol)initializerType).ElementType;
break;
}

// check for a special ref-returning method
var additionalDiagnostics = DiagnosticBag.GetInstance();
Copy link
Member

@jcouv jcouv Feb 20, 2018

Choose a reason for hiding this comment

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

What was the motivation for removing the try/catch for freeing this instance? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Andy's comment on this PR. I was undecided which way is better, so if someone also thinks that try/finally makes this more complex than needed, I've removed it


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

getPinnableMethod = GetDangerousGetPinnableReferenceMethodOpt(initializerOpt, additionalDiagnostics);

// check for String
// NOTE: We will allow DangerousGetPinnableReferenceMethod to take precendence, but only if it is a member of System.String
if (initializerType.SpecialType == SpecialType.System_String &&
((object)getPinnableMethod == null || getPinnableMethod.ContainingType.SpecialType != SpecialType.System_String))
{
elementType = this.GetSpecialType(SpecialType.System_Char, diagnostics, initializerSyntax);
additionalDiagnostics.Free();
break;
}

if ((object)getPinnableMethod != null)
{
elementType = getPinnableMethod.ReturnType;
CheckFeatureAvailability(initializerOpt.Syntax, MessageID.IDS_FeatureExtensibleFixedStatement, diagnostics);
additionalDiagnostics.Free();
break;
}
else
{
// if the feature was enabled but something went wrong with the method, report that
bool extensibleFixedEnabled = ((CSharpParseOptions)initializerOpt.SyntaxTree.Options)?.IsFeatureEnabled(MessageID.IDS_FeatureExtensibleFixedStatement) != false;
if (extensibleFixedEnabled && additionalDiagnostics != null)
{
diagnostics.AddRange(additionalDiagnostics);
}
Error(diagnostics, ErrorCode.ERR_ExprCannotBeFixed, initializerSyntax);
additionalDiagnostics.Free();
return false;
}
}
else

if (elementType.IsManagedType)
{
TypeSymbol elementType;
bool hasErrors = false;
Error(diagnostics, ErrorCode.ERR_ManagedAddr, initializerSyntax, elementType);
hasErrors = true;
}

if (initializerType.SpecialType == SpecialType.System_String)
initializerOpt = GetFixedLocalCollectionInitializer(initializerOpt, elementType, declType, getPinnableMethod, hasErrors, diagnostics);
return true;
}

private MethodSymbol GetDangerousGetPinnableReferenceMethodOpt(BoundExpression initializer, DiagnosticBag additionalDiagnostics)
{
if (initializer.Type.SpecialType == SpecialType.System_Void)
{
return null;
}

const string methodName = "DangerousGetPinnableReference";

DiagnosticBag bindingDiagnostics = DiagnosticBag.GetInstance();
try
{
var boundAccess = BindInstanceMemberAccess(
initializer.Syntax,
initializer.Syntax,
initializer,
methodName,
rightArity: 0,
typeArgumentsSyntax: default,
typeArguments: default,
invoked: true,
bindingDiagnostics);

if (boundAccess.Kind != BoundKind.MethodGroup)
{
elementType = this.GetSpecialType(SpecialType.System_Char, diagnostics, initializerSyntax);
Debug.Assert(!elementType.IsManagedType);
// the thing is not even a method
return null;
}
else if (initializerType.IsArray())

var analyzedArguments = AnalyzedArguments.GetInstance();
BoundExpression getPinnableReferenceCall = BindMethodGroupInvocation(
initializer.Syntax,
initializer.Syntax,
methodName,
(BoundMethodGroup)boundAccess,
analyzedArguments,
bindingDiagnostics,
queryClause: null,
allowUnexpandedForm: false);

analyzedArguments.Free();

if (getPinnableReferenceCall.Kind != BoundKind.Call)
{
// See ExpressionBinder::BindPtrToArray (though most of that functionality is now in LocalRewriter).
var arrayType = (ArrayTypeSymbol)initializerType;
elementType = arrayType.ElementType;
// did not find anything callable
return null;
}

if (elementType.IsManagedType)
{
Error(diagnostics, ErrorCode.ERR_ManagedAddr, initializerSyntax, elementType);
hasErrors = true;
}
var call = (BoundCall)getPinnableReferenceCall;
if (call.ResultKind == LookupResultKind.Empty)
{
// did not find any methods that even remotely fit
return null;
}
else if (!initializerOpt.HasAnyErrors)

var getPinnableReferenceMethod = call.Method;
if (getPinnableReferenceMethod is ErrorMethodSymbol ||
getPinnableReferenceCall.HasAnyErrors)
{
elementType = initializerOpt.Type;
switch (initializerOpt.Kind)
{
case BoundKind.AddressOfOperator:
elementType = ((BoundAddressOfOperator)initializerOpt).Operand.Type;
break;
case BoundKind.FieldAccess:
var fa = (BoundFieldAccess)initializerOpt;
if (!fa.FieldSymbol.IsFixed)
{
Error(diagnostics, ErrorCode.ERR_FixedNotNeeded, initializerSyntax);
return true;
}
else
{
elementType = ((PointerTypeSymbol)fa.Type).PointedAtType;
}
break;
case BoundKind.Conversion:
// The following assertion would not be correct because there might be an implicit conversion after (above) the explicit one.
//Debug.Assert(((BoundConversion)initializerOpt).ExplicitCastInCode, "The assignment conversion hasn't been applied yet, so this must be from source.");

// NOTE: Dev10 specifically doesn't report this error for the array or string cases.
Error(diagnostics, ErrorCode.ERR_BadCastInFixed, initializerSyntax);
return true;
default:
// CONSIDER: this is a very confusing error message, but it's what Dev10 reports.
Error(diagnostics, ErrorCode.ERR_FixedNotNeeded, initializerSyntax);
return true;
}
// we almost succeded, this is unusual and may be hard to diagnose.
// report additional errors on why we failed to bind the helper
additionalDiagnostics.AddRange(bindingDiagnostics);
return null;
}
else

if (HasOptionalOrVariableParameters(getPinnableReferenceMethod) ||
getPinnableReferenceMethod.ReturnsVoid ||
!getPinnableReferenceMethod.RefKind.IsManagedReference() ||
!(getPinnableReferenceMethod.ParameterCount == 0 || getPinnableReferenceMethod.IsStatic && getPinnableReferenceMethod.ParameterCount == 1))
{
// convert to generate any additional conversion errors
initializerOpt = GenerateConversionForAssignment(declType, initializerOpt, diagnostics);
return true;
// the method does not fit the pattern
additionalDiagnostics.Add(ErrorCode.WRN_PatternBadSignature, initializer.Syntax.Location, initializer.Type, "fixed", getPinnableReferenceMethod);
return null;
}

initializerOpt = GetFixedLocalCollectionInitializer(initializerOpt, elementType, declType, hasErrors, diagnostics);
return getPinnableReferenceMethod;
}
finally
{
bindingDiagnostics.Free();
}

return true;
}

/// <summary>
/// Wrap the initializer in a BoundFixedLocalCollectionInitializer so that the rewriter will have the
/// information it needs (e.g. conversions, helper methods).
/// </summary>
private BoundExpression GetFixedLocalCollectionInitializer(BoundExpression initializer, TypeSymbol elementType, TypeSymbol declType, bool hasErrors, DiagnosticBag diagnostics)
private BoundExpression GetFixedLocalCollectionInitializer(
BoundExpression initializer,
TypeSymbol elementType,
TypeSymbol declType,
MethodSymbol getPinnableMethodOpt,
bool hasErrors,
DiagnosticBag diagnostics)
{
Debug.Assert(initializer != null);

Expand All @@ -1158,6 +1246,7 @@ private BoundExpression GetFixedLocalCollectionInitializer(BoundExpression initi
pointerType,
elementConversion,
initializer,
getPinnableMethodOpt,
declType,
hasErrors);
}
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,9 @@

<!-- Wrapped expression. -->
<Field Name="Expression" Type="BoundExpression" Null="disallow"/>

<!-- optional method that returns a pinnable reference for the instance -->
<Field Name="GetPinnableOpt" Type="MethodSymbol" Null="allow"/>
</Node>

<AbstractNode Name="BoundStatement" Base="BoundNode"/>
Expand Down
7 changes: 0 additions & 7 deletions src/Compilers/CSharp/Portable/BoundTree/GenerateNodes.bat

This file was deleted.

27 changes: 27 additions & 0 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.

9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,12 @@
<data name="ERR_FixedNotNeeded" xml:space="preserve">
<value>You cannot use the fixed statement to take the address of an already fixed expression</value>
</data>
<data name="ERR_ExprCannotBeFixed" xml:space="preserve">
<value>The given expression cannot be used in a fixed statement</value>
</data>
<data name="ERR_FixableHelperDoesNotFitThePattern" xml:space="preserve">
<value>The method '{0}' does not meet necessary requirements</value>
</data>
<data name="ERR_UnsafeNeeded" xml:space="preserve">
<value>Pointers and fixed size buffers may only be used in an unsafe context</value>
</data>
Expand Down Expand Up @@ -4267,6 +4273,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureRefConditional" xml:space="preserve">
<value>ref conditional expression</value>
</data>
<data name="IDS_FeatureExtensibleFixedStatement" xml:space="preserve">
<value>extensible fixed statement</value>
</data>
<data name="CompilationC" xml:space="preserve">
<value>Compilation (C#): </value>
</data>
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,9 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces
(receiverType.IsNullableType() && expression.HasValueMethodOpt != null), "conditional receiver cannot be a struct");

var receiverConstant = receiver.ConstantValue;
if (receiverConstant != null)
if (receiverConstant?.IsNull == false)
{
// const but not default, must be a reference type
// const but not null, must be a reference type
Debug.Assert(receiverType.IsVerifierReference());
// receiver is a reference type, so addresskind does not matter, but we do not intend to write.
receiverTemp = EmitReceiverRef(receiver, AddressKind.ReadOnly);
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,8 @@ internal enum ErrorCode
WRN_AttributesOnBackingFieldsNotAvailable = 8371,
ERR_DoNotUseFixedBufferAttrOnProperty = 8372,

// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)
ERR_ExprCannotBeFixed = 9365,
ERR_FixableHelperDoesNotFitThePattern = 9366,
}
// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)
}
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ internal enum MessageID

IDS_FeatureRefConditional = MessageBase + 12731,
IDS_FeatureAttributesOnBackingFields = MessageBase + 12732,
IDS_FeatureExtensibleFixedStatement = MessageBase + 12733,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -189,6 +190,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
{
// C# 7.3 features.
case MessageID.IDS_FeatureAttributesOnBackingFields: // semantic check
case MessageID.IDS_FeatureExtensibleFixedStatement: // semantic check
return LanguageVersion.CSharp7_3;

// C# 7.2 features.
Expand Down
Loading