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

Annotate more of the binder #40946

Open
wants to merge 4 commits into
base: master
from
Open

Conversation

@333fred
Copy link
Member

333fred commented Jan 14, 2020

No description provided.

@333fred 333fred requested a review from dotnet/roslyn-compiler Jan 14, 2020
@@ -450,14 +450,14 @@ private BoundExpression CreateTupleLiteralConversion(SyntaxNode syntax, BoundTup
else
{
var tupleSyntax = (TupleExpressionSyntax)sourceTuple.Syntax;
var locationBuilder = ArrayBuilder<Location>.GetInstance();
var locationBuilder = ArrayBuilder<Location?>.GetInstance();

foreach (var argument in tupleSyntax.Arguments)
{
locationBuilder.Add(argument.NameColon?.Name.Location);

This comment has been minimized.

Copy link
@333fred

333fred Jan 14, 2020

Author Member

This is what is forcing all the ImmutableArray<Location?> annotation changes. #ByDesign

Copy link
Member

jaredpar left a comment

:shipit:

@333fred

This comment has been minimized.

Copy link
Member Author

333fred commented Jan 15, 2020

@dotnet/roslyn-compiler for a second review.

@@ -3167,7 +3167,7 @@ protected override IPointerTypeSymbol CommonCreatePointerTypeSymbol(ITypeSymbol
return NamedTypeSymbol.CreateTuple(
locationOpt: null, // no location for the type declaration
elementTypesWithAnnotations: typesBuilder.ToImmutableAndFree(),
elementLocations: elementLocations,
elementLocations: elementLocations!,

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

elementLocations [](start = 34, length = 16)

Should the type of elementLocations be adjusted in public API then, so we avoid the suppression? #Resolved

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

I don't think so. If you're creating this from a public context, you should pass real locations as more of a matter of principle.


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

@@ -102,7 +102,7 @@ static NamedTypeSymbol getTupleUnderlyingType(ImmutableArray<TypeWithAnnotations
NamedTypeSymbol tupleCompatibleType,
ImmutableArray<string?> elementNames = default,
ImmutableArray<bool> errorPositions = default,
ImmutableArray<Location> elementLocations = default,
ImmutableArray<Location?> elementLocations = default,

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

ImmutableArray<Location?> elementLocations [](start = 12, length = 42)

what prompted that change?
Do we need a test for Compilation.CreateTupleTypeSymbol passing null locations? (I couldn't find one) #Closed

This comment has been minimized.

Copy link
@333fred

333fred Jan 16, 2020

Author Member
@@ -332,13 +333,13 @@ protected bool IsAttributeConditionallyOmitted(NamedTypeSymbol attributeType, Sy

// Named argument
// TODO: use fully qualified identifier name for boundNamedArgumentsSet
string argumentName = argument.NameEquals.Name.Identifier.ValueText;
string argumentName = argument.NameEquals.Name.Identifier.ValueText!;
if (boundNamedArgumentsBuilder == null)

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

boundNamedArgumentsBuilder == null [](start = 28, length = 34)

consider changing this test to check boundNamedArgumentsSet instead, since the two locals are always initialized together (or not), then the suppression can be removed. #WontFix

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

Our approach for nullable has been to not change any semantic behavior, just to annotate what we are currently doing. I'm going to keep this going forward.


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

@@ -396,9 +397,9 @@ private BoundExpression BindNamedAttributeArgument(AttributeArgumentSyntax named

// TODO: should we create an entry even if there are binding errors?
var fieldSymbol = namedArgumentNameSymbol as FieldSymbol;
IdentifierNameSyntax nameSyntax = namedArgument.NameEquals.Name;
IdentifierNameSyntax nameSyntax = namedArgument.NameEquals!.Name;

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

namedArgument.NameEquals [](start = 46, length = 24)

consider using an assertion instead #Resolved

@@ -425,10 +426,10 @@ private BoundExpression BindNamedAttributeArgument(AttributeArgumentSyntax named

private Symbol BindNamedAttributeArgumentName(AttributeArgumentSyntax namedArgument, NamedTypeSymbol attributeType, DiagnosticBag diagnostics, out bool wasError, out LookupResultKind resultKind)
{
var identifierName = namedArgument.NameEquals.Name;
var identifierName = namedArgument.NameEquals!.Name;

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

NameEquals [](start = 47, length = 10)

consider using an assertion here too (and at least one more place below) #Resolved


// Bind each clause and add to the results.
foreach (var clause in clauses)
{
var name = clause.Name.Identifier.ValueText;
int ordinal;
if (names.TryGetValue(name, out ordinal))
if (names.TryGetValue(name!, out ordinal))

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

name [](start = 38, length = 4)

consider asserting instead #Resolved

@@ -98,7 +99,7 @@ internal partial class Binder

TypeParameterConstraintClause.AdjustConstraintTypes(containingSymbol, typeParameters, results, ref isValueTypeOverride);

RemoveInvalidConstraints(typeParameters, results, syntaxNodes, diagnostics);
RemoveInvalidConstraints(typeParameters, results!, syntaxNodes, diagnostics);

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

results [](start = 53, length = 7)

why this suppression? #ByDesign

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

RemoveInvalidConstraints takes a non-null arraybuilder. We started with some nulls in the array, but just went through the whole thing to remove it.


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

@@ -74,7 +76,7 @@ internal partial class Binder

// We need to preserve any conversion that changes the type (even identity conversions, like object->dynamic),
// or that was explicitly written in code (so that GetSemanticInfo can find the syntax in the bound tree).
if (!isCast && source.Type.Equals(destination, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
if (!isCast && source.Type!.Equals(destination, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

Type [](start = 38, length = 4)

how do we know source.Type isn't null from surrounding code?
If we know because of some remote code, then assertion seems preferable.

Note: a few lines below we do a null-test on source.Type #Resolved

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

Note: a few lines below we do a null-test on source.Type

BindToNaturalType should take care of that, but I'll add an assert.


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

@@ -700,7 +700,7 @@ private bool MemberGroupFinalValidationAccessibilityChecks(BoundExpression recei
}
else if (WasImplicitReceiver(receiverOpt))
{
if (InFieldInitializer && !ContainingType.IsScriptClass || InConstructorInitializer || InAttributeArgument)
if (InFieldInitializer && !ContainingType!.IsScriptClass || InConstructorInitializer || InAttributeArgument)

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

ContainingType [](start = 43, length = 14)

Is the suppression okay because InFieldInitializer is true at this point? #Resolved

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

Correct.


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

@@ -1067,14 +1067,14 @@ internal bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, b
TypeSymbol destination,
DiagnosticBag diagnostics)
{
Debug.Assert(sourceValue != null);
RoslynDebug.Assert(sourceValue != null);
Debug.Assert(!sourceValue.IsBad);

SpecialType destinationType;
if ((object)destination != null && destination.IsEnumType())

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

destination [](start = 24, length = 11)

Should destination be declared as nullable? #Resolved

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

The only callers pass non-null.


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

@@ -1093,7 +1093,7 @@ internal bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, b
if (!CheckConstantBounds(destinationType, sourceValue))
{
// NOTE: Dev10 puts a suffix, "M", on the constant value.
Error(diagnostics, ErrorCode.ERR_ConstOutOfRange, syntax, sourceValue.Value + "M", destination);
Error(diagnostics, ErrorCode.ERR_ConstOutOfRange, syntax, sourceValue.Value + "M", destination!);

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 16, 2020

Member

destination [](start = 103, length = 11)

Can't destination be null here? Seems dangerous #Resolved

This comment has been minimized.

Copy link
@333fred

333fred Jan 23, 2020

Author Member

The null check above is actually superfluous.


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

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Jan 16, 2020

        CSharpAttributeData[] attributesBuilder,

Should this hold nullable elements instead? We test for null elements below #Resolved


Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs:56 in 69941f7. [](commit_id = 69941f7, deletion_comment = False)

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Jan 16, 2020

            ConstantValue constantValue = node.ConstantValue;

Should this be nullable? we test for null below #Resolved


Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs:1043 in 69941f7. [](commit_id = 69941f7, deletion_comment = False)

Copy link
Member

jcouv left a comment

Done with review pass (iteration 3)

@jcouv jcouv self-assigned this Jan 16, 2020
@333fred

This comment has been minimized.

Copy link
Member Author

333fred commented Jan 23, 2020

@jcouv addressed feedback. I haven't merged from master in a while, so I'll wait for your feedback and if you sign off, rebase and fix any new changes.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.