Address some PROTOTYPE comments #29607
Conversation
jcouv
added
Area-Compilers
Feature - Nullable Reference Types
| @@ -1734,7 +1734,6 @@ | |||
| </Node> | |||
|
|
|||
| <!-- Node only used during nullability flow analysis to represent an expression with an updated nullability --> | |||
| <!-- PROTOTYPE(NullableReferenceTypes): Derive from BoundValuePlaceholderBase. --> | |||
There was a problem hiding this comment.
Cannot derive from BoundValuePlaceholderBase since that type requires non-null Type.
| @@ -30912,7 +30913,7 @@ static T F<T>(IEnumerable<T> e) | |||
| /// should not result in a warning at the call site. | |||
| /// </summary> | |||
| [WorkItem(26626, "https://github.com/dotnet/roslyn/issues/26626")] | |||
| [Fact(Skip = "PROTOTYPE(NullableReferenceTypes): null check on default value temporarily skipped")] | |||
| [Fact(Skip = "Null check on default value temporarily skipped")] | |||
|
@dotnet/roslyn-compiler please review. |
| @@ -159,8 +159,6 @@ internal static TypeSymbolWithAnnotations Create(INonNullTypesContext nonNullTyp | |||
| { | |||
| isAnnotated = true; | |||
| } | |||
| // PROTOTYPE(NullableReferenceTypes): this defaulting logic should be removed. There are many paths that currently don't have an explicit context at the moment. | |||
| nonNullTypesContext = nonNullTypesContext ?? NonNullTypesFalseContext.Instance; | |||
There was a problem hiding this comment.
nonNullTypesContext = nonNullTypesContext ?? NonNullTypesFalseContext.Instance; [](start = 12, length = 79)
Consider adding an assert that nonNullTypesContext is not null. #Resolved
| @@ -1401,7 +1401,8 @@ class NonNullTypesAttribute : Attribute | |||
| comp.VerifyDiagnostics(); | |||
| } | |||
|
|
|||
| [Fact(Skip = "PROTOTYPE(NullableReferenceTypes): stack overflow")] | |||
| [WorkItem(29594, "https://github.com/dotnet/roslyn/issues/29594")] | |||
| [Fact(Skip = "Stack overflow")] | |||
There was a problem hiding this comment.
Stack overflow [](start = 22, length = 14)
Here and in other similar places, should we use the link to the issue as the Skip reason?
There was a problem hiding this comment.
Having a short description avoids the need to look up the bug. Will leave as is, with link in the WorkItem and description in Skip = "...".
In reply to: 214486306 [](ancestors = 214486306)
| @@ -25557,7 +25558,7 @@ public class C<T> {} | |||
| ); | |||
| } | |||
|
|
|||
| [Fact(Skip = "NonNullTypes does not control warnings")] // PROTOTYPE(NullableReferenceTypes): Update or remove test. | |||
| [Fact(Skip = "NonNullTypes does not control warnings")] // https://github.com/dotnet/roslyn/issues/29616 | |||
There was a problem hiding this comment.
#29616 [](start = 67, length = 45)
Here and in other similar places, consider adding a WorkItem attribute. #Resolved
| @@ -37,6 +35,9 @@ public void Bug18280() | |||
| var arr = x.Type.TypeSymbol; | |||
|
|
|||
| arr.GetHashCode(); | |||
| // PROTOTYPE(NullableReferenceTypes): StackOverflowException in | |||
There was a problem hiding this comment.
// PROTOTYPE(NullableReferenceTypes): StackOverflowException in [](start = 12, length = 63)
Why leave this comment if test passes? #Closed
There was a problem hiding this comment.
The commented out line will result in a StackOverflowException when uncommented.
In reply to: 214486835 [](ancestors = 214486835)
No description provided.