Bind NonNullTypes for types#28491
Bind NonNullTypes for types#28491jcouv merged 18 commits intodotnet:features/NullableReferenceTypesfrom
Conversation
There was a problem hiding this comment.
// Note: we set the BinderFlags.NonNullTypesTrue to break a cycle [](start = 12, length = 65)
Perhaps move the comment to the base() call.
There was a problem hiding this comment.
Consider removing, or add a placeholder for new parameter.
There was a problem hiding this comment.
= false [](start = 220, length = 8)
Does not need to be optional.
There was a problem hiding this comment.
` [](start = 12, length = 1)
` [](start = 12, length = 1)
Consider using <code> tags instead of markdown. #ByDesign
There was a problem hiding this comment.
` [](start = 12, length = 1)
Consider using <code> tags instead of markdown.
|
Done review pass (commit 1) #Resolved |
|
@cston Bootstrapping build caught a few more cycles for this PR. I'll stop by to discuss the last one (we bind some types in the |
|
I'm going to reset this PR with a simpler change on top of the lazy NonNullTypes. I'm scrapping this PR a second time. The PR is now built on top of the test changes to related to the default context (#28769). |
There was a problem hiding this comment.
if (includeNullability && …)? #Resolved
There was a problem hiding this comment.
Thanks. Hold off on detailed review. I'm going to rebase this shortly. Also still working on some failing tests.
I'll stop by tomorrow to sync on this PR (solved problems, remaining problems). #Closed
There was a problem hiding this comment.
if (includeNullability && ...)? #Resolved
|
@cston I'm down to 3 compiler tests and a few IDE tests (for some reason). This PR is ready for review :-) #Resolved |
|
FYI, I updated OP's description of the various cycles and fixes included in this PR. #Resolved |
| if (typeSymbol.TypeKind == TypeKind.Array && isNullable.HasValue) | ||
|
|
||
|
|
||
| if (typeSymbol.TypeKind == TypeKind.Array) |
There was a problem hiding this comment.
Extra blank line. #Resolved
| } | ||
|
|
||
|
|
||
| var arrayType = symbol; |
There was a problem hiding this comment.
Extra blank line. #Resolved
| } | ||
|
|
||
| private void VisitArrayType(IArrayTypeSymbol symbol, bool? isNullable) | ||
| private void VisitArrayType(IArrayTypeSymbol symbol, TypeSymbolWithAnnotations isNullable) |
There was a problem hiding this comment.
isNullable [](start = 87, length = 10)
Consider renaming here and in method below. Perhaps typeOpt. #Resolved
| private void AddNullableAnnotations(TypeSymbolWithAnnotations isNullable) | ||
| { | ||
| if (format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier) && | ||
| !ReferenceEquals(isNullable, null) && |
There was a problem hiding this comment.
ReferenceEquals(isNullable, null) [](start = 17, length = 33)
Perhaps return early if arg is null. #Resolved
| /// <summary> | ||
| /// Returns a constructed type given its type arguments. | ||
| /// </summary> | ||
| /// <param name="nonNullTypesContext">This context indicates how to interprete un-annotated types.</param> |
There was a problem hiding this comment.
interprete [](start = 76, length = 10)
Typo. #Resolved
| /// </summary> | ||
| [WorkItem(26626, "https://github.com/dotnet/roslyn/issues/26626")] | ||
| [Fact] | ||
| [Fact(Skip = "PROTOTYPE(NullalbeReferenceTypes): null check on default value temporarily skipped")] |
There was a problem hiding this comment.
Nullalbe [](start = 32, length = 8)
Typo, here and in other tests below. #Resolved
| //Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C").WithArguments("field", "F1").WithLocation(8, 12)); | ||
| } | ||
|
|
||
| private const string NonNullTypesAttributesDefinition = @" |
There was a problem hiding this comment.
private const [](start = 8, length = 13)
Please move these constants to the top. #Resolved
| "; | ||
|
|
||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Extra blank line. #Resolved
|
@333fred This is ready for another review when you get a chance. Thanks #Resolved |
| IgnoreDynamicAndTupleNames = IgnoreDynamic | IgnoreTupleNames, | ||
| // PROTOTYPE(NullableReferenceTypes): Consider renaming the Nullable options, | ||
| // perhaps CompareNullableAnnotations and IgnoreUnknownNullableAnnotations. | ||
| // Note: comparisons with nullability-related options pull on NonNullTypes, which can cause cycles. |
There was a problem hiding this comment.
Consider making this a doc comment #Closed
| /// <summary> | ||
| /// Append '!' to non-nullable reference types. | ||
| /// Note this causes SymbolDisplay to pull on IsNullable and therefore NonNullTypes, | ||
| /// so don't use this option in binding, in order to avoid cycles. |
There was a problem hiding this comment.
Can we add an assert in the binder to verify this? #Resolved
There was a problem hiding this comment.
I'm not sure how we could prevent this flag from being used in binder code. #Resolved
| var typeSymbol = DynamicTypeDecoder.TransformType(originalEventType, targetSymbolCustomModifierCount, handle, moduleSymbol); | ||
| var type = TypeSymbolWithAnnotations.Create(typeSymbol); | ||
|
|
||
| // We start without annotations |
There was a problem hiding this comment.
Might be good to elaborate a bit as to why we start without annotations #Closed
There was a problem hiding this comment.
| Debug.Assert(resultType.Equals(destinationType, | ||
| TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | // Same object/dynamic and tuple names as destination type. | ||
| TypeCompareKind.CompareNullableModifiersForReferenceTypes)); // Same nullability as destination type. | ||
| TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds)); // Same object/dynamic and tuple names as destination type. |
There was a problem hiding this comment.
[](start = 12, length = 3)
Indentation is off. #Resolved
| if (!_underlying.TypeSymbol.IsValueType) | ||
| { | ||
| Debug.Assert(_underlying.IsNullable == false); | ||
| //Debug.Assert(_underlying.IsNullable == false); |
There was a problem hiding this comment.
Are these causing a cycle? If so, consider adding a prototype comment so we don't forget about them #Closed
There was a problem hiding this comment.
Yes. We shouldn't call IsNullable (which pulls on NonNullTypes) during binding, which this method is.
I'll remove the commented out code. #Closed
| public class NullableReferenceTypesTests : CSharpTestBase | ||
| { | ||
| private const string NullableAttributeDefinition = @" | ||
| private const string NullableAttributeDefinition = @" |
There was a problem hiding this comment.
[](start = 8, length = 1)
Indentation is off #Closed
|
Done review pass (commit 17) #Resolved |
In a previous PR on
[NonNullTypes]I used a syntax-based check to recognize the attribute, rather than use proper binding. This allowed for avoiding a number of cycles.The current PR removes the use of syntax-based check for deciding if a source named type symbol has
NonNullTypes. Other symbols will be address in follow-up PRs.Even with the NonNullTypesContext design, there were a cycles left to fix, because we're pulling on NonNullTypes too early.
CheckNullableReferenceTypeMismatchOnImplementingMember:
This was fixed by calling CheckNullableReferenceTypeMismatchOnImplementingMember later.
SetUnknownNullabilityForReferenceTypesIfNecessary cycle via SourceMemberFieldSymbol.GetFieldType:
Fixed by removing the call to SetUnknownNullabilityForReferenceTypesIfNecessary. But caused other issues to become visible. Still working on those.
Cycle with SymbolDisplay:
Fixed by making symbol display as lazy as possible.
I also had to change the display options used for debugger display, so that the debugger would not pull on NonNullTypes accidentally. It's better to only see
?and blank, rather than no be able to see the type at all, while debugging.MakeDefaultExpression:
I temporarily temporarily commented out this check. We should execute this check later.
The cycles below are no longer relevant. They were all solved by the NonNullTypesContext design :-)
For context, here are the stack traces for the various cycles:
Cycle 11
Cycle 12 (hit in bootstrap build, fixed in
GetNextBaseTypeNoUseSiteDiagnosticspassing theignoreNonNullTypesAttributefor the enum case as well):Cycle 13 (and 14 is very closely related):
Cycle 15 is when there was in
MakeAcyclicBaseType, which would callFindBaseRefSyntax(which callsBindType) to report use-site diagnostics on bases.Cycle 16 isn't solved yet:
There was another cycle, but I lost my notes for it. I'll gather the information and add here.