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

Adjust the way nullable annotations are represented in metadata #31212

Merged
merged 7 commits into from
Nov 19, 2018
Merged
Show file tree
Hide file tree
Changes from 4 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
71 changes: 30 additions & 41 deletions docs/features/nullable-reference-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ In source, nullable reference types are annotated with `?`.
string? OptString; // may be null
Dictionary<string, object?>? OptDictionaryOptValues; // dictionary may be null, values may be null
```
A warning is reported when annotating a reference type or unconstrained generic type with `?` outside a `NonNullTypes(true)` context.
A warning is reported when annotating a reference type or unconstrained generic type with `?` outside a `#nullable` context.
Copy link
Member

@gafter gafter Nov 17, 2018

Choose a reason for hiding this comment

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

I thought it was an error to annotate an unconstrained generic type. #Resolved


In metadata, nullable reference types are annotated with a `[Nullable]` attribute.
```c#
Expand All @@ -22,54 +22,43 @@ namespace System.Runtime.CompilerServices
AllowMultiple = false)]
public sealed class NullableAttribute : Attribute
Copy link
Member

@jcouv jcouv Nov 16, 2018

Choose a reason for hiding this comment

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

NullableAttribute [](start = 24, length = 17)

Not blocking this PR: we'll need to think about how the values of the parameters will be stored in the fields of NullableAttribute. Jared mentioned we need to store those values so that they are accessible from reflection.

Tracked by #30143 #Resolved

{
public NullableAttribute() { }
public NullableAttribute(bool[] b) { }
public NullableAttribute(byte b) { }
public NullableAttribute(byte[] b) { }
}
}
```
The parameter-less constructor is emitted for simple type references with top-level nullability and for type parameter definitions that have a `class?` constraint;
the constructor with `bool[]` parameter is emitted for type references with nested types and nullability.

Each type reference is accompanied by a NullableAttribute with an array of bytes, where 0 is Oblivious, 1 is NotAnnotated and 2 is Annotated.

To optimize trivial cases the attribute can be omitted, or instead can be replaced with an attribute that takes a single byte value rather than an array.

Trivial/optimized cases:
1) All parts are NotAnnotated � a NullableAttribute with a single value 1 (rather than an array of 1s)
2) All parts are Annotated - a NullableAttribute with a single value 2 (rather than an array of 2s)
3) All parts are Oblivious � the attribute is omitted, this matches how we interpret the lack of an attribute in legacy assemblies.
For completeness, we would also recognize a NullableAttribute with a single value 0 (rather than an array of 0s),
but compiler will never emit an attribute like this.

NullableAttribute(1) should be placed on a type parameter definition that has a `class!` constraint.
NullableAttribute(2) should be placed on a type parameter definition that has a `class?` constraint.
Other forms of NullableAttribute ar not emitted on type parameter definitions and are not specially recognized on them.
Copy link
Member

@jcouv jcouv Nov 16, 2018

Choose a reason for hiding this comment

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

ar [](start = 33, length = 2)

typo: are #Resolved


The `NullableAttribute` type declaration is synthesized by the compiler if it is not included in the compilation, but is needed to produce the output.

```c#
// C# representation of metadata
Copy link
Member

@jcouv jcouv Nov 19, 2018

Choose a reason for hiding this comment

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

It would be good to expand this section with an example involving value types (int and Nullable<int>). #Resolved

[Nullable]
[Nullable(2)]
string OptString; // string?
[Nullable(new[] { true, false, true })]
[Nullable(new[] { 2, 1, 2 })]
Dictionary<string, object> OptDictionaryOptValues; // Dictionary<string!, object?>?
Copy link
Member

@jcouv jcouv Nov 16, 2018

Choose a reason for hiding this comment

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

object> [](start = 19, length = 7)

Should this be Dictionary<..., object?>? in source as well? Never mind. The type here is from PE. #Closed

string[] Oblivious1; // string~[]~
[Nullable(0)] string[] Oblivious2; // string~[]~
[Nullable(new[] { 0, 0 })] string[] Oblivious3; // string~[]~
[Nullable(1)] string[] NotNull1; // string![]!
[Nullable(new[] { 1, 1 })] string[] NotNull2; // string![]!
[Nullable(new[] { 0, 2 })] string[] ObliviousMaybeNull; // string?[]~
[Nullable(new[] { 1, 2 })] string[] NotNullMaybeNull; // string?[]!
```
The `NullableAttribute` type declaration is synthesized by the compiler if it is not included in the compilation.

Unannotated reference types are non-nullable or null-oblivious depending on whether the containing scope includes `[NonNullTypes]`.
```c#
namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Class |
AttributeTargets.Constructor |
AttributeTargets.Delegate |
AttributeTargets.Enum |
AttributeTargets.Event |
AttributeTargets.Field |
AttributeTargets.Interface |
AttributeTargets.Method |
AttributeTargets.Module |
AttributeTargets.Property |
AttributeTargets.Struct,
AllowMultiple = false)]
internal sealed class NonNullTypesAttribute : Attribute
{
public NonNullTypesAttribute(bool enabled = true) { }
}
}
```
If there is no `[NonNullTypes]` attribute at any containing scope, including the module, reference types are null-oblivious.
```c#
[NonNullTypes(false)] string[] Oblivious; // string~[]~
[NonNullTypes(true)] string[] NotNull; // string![]!
[NonNullTypes(false), Nullable(new[] { false, true })] string[] ObliviousMaybeNull; // string?[]~
[NonNullTypes(true), Nullable(new[] { false, true })] string[] NotNullMaybeNull; // string?[]!
```
The `NonNullTypesAttribute` is always implicitly included in a compilation, but is only emitted if it is referenced in source. It cannot be referenced from metadata (assembly or module).
`NonNullTypesAttribute` can only be used in C# 8.0 compilations (or above).

## Declaration warnings
_Describe warnings reported for declarations in initial binding._

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ internal virtual Symbol ContainingMemberOrLambda
/// <summary>
/// Are we in a context where un-annotated types should be interpreted as non-null?
/// </summary>
internal Symbol NonNullTypesContext => ContainingMember().OriginalDefinition;
internal INonNullTypesContext NonNullTypesContext => (Flags & BinderFlags.InEEMethodBinder) == 0 ? ContainingMember().OriginalDefinition : NonNullTypesTrueContext.Instance;

/// <summary>
/// Is the contained code within a member method body?
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/BinderFlags.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ internal enum BinderFlags : uint
/// </summary>
InContextualAttributeBinder = 1 << 29,

/// <summary>
/// Are we binding for the purpose of an Expression Evaluator
/// </summary>
InEEMethodBinder = 1 << 30,

// Groups

AllClearedAtExecutableCodeBoundary = InLockBody | InCatchBlock | InCatchFilter | InFinallyBlock | InTryBlockOfTryCatch | InNestedFinallyBlock,
Expand Down
30 changes: 12 additions & 18 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1494,28 +1494,22 @@ internal Symbol ResultSymbol(
var srcSymbol = symbols[best.Index];
var mdSymbol = symbols[secondBest.Index];

object arg0;

if (best.IsFromSourceModule)
{
arg0 = srcSymbol.Locations.First().SourceTree.FilePath;
}
else
{
Debug.Assert(best.IsFromAddedModule);
arg0 = srcSymbol.ContainingModule;
}

//if names match, arities match, and containing symbols match (recursively), ...
if (srcSymbol.ToDisplayString(SymbolDisplayFormat.QualifiedNameArityFormat) ==
mdSymbol.ToDisplayString(SymbolDisplayFormat.QualifiedNameArityFormat))
{
if (srcSymbol.Equals(Compilation.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_NonNullTypesAttribute)))
{
// Silently prefer the injected symbol
return originalSymbols[best.Index];
}

object arg0;
if (best.IsFromSourceModule)
{
SyntaxTree tree = srcSymbol.Locations.FirstOrNone().SourceTree;
arg0 = tree != null ? (object)tree.FilePath : MessageID.IDS_InjectedDeclaration.Localize();
}
else
{
Debug.Assert(best.IsFromAddedModule);
arg0 = srcSymbol.ContainingModule;
}

if (srcSymbol.Kind == SymbolKind.Namespace && mdSymbol.Kind == SymbolKind.NamedType)
{
// ErrorCode.WRN_SameFullNameThisNsAgg: The namespace '{1}' in '{0}' conflicts with the imported type '{3}' in '{2}'. Using the namespace defined in '{0}'.
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)

if (!returnType.IsNull)
{
if (returnType.ContainsNullableReferenceTypes())
if (returnType.NeedsNullableAttribute())
{
binder.Compilation.EnsureNullableAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilation: false);
// Note: we don't need to warn on annotations used without NonNullTypes context for lambdas, as this is handled in binding already
Expand Down
20 changes: 1 addition & 19 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.

6 changes: 0 additions & 6 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -4284,9 +4284,6 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_Text" xml:space="preserve">
<value>&lt;text&gt;</value>
</data>
<data name="IDS_InjectedDeclaration" xml:space="preserve">
<value>injected declaration</value>
</data>
<data name="IDS_FeatureNullPropagatingOperator" xml:space="preserve">
<value>null propagating operator</value>
</data>
Expand Down Expand Up @@ -5628,9 +5625,6 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_BadDynamicAwaitForEach" xml:space="preserve">
<value>Cannot use a collection of dynamic type in an asynchronous foreach</value>
</data>
<data name="ERR_ExplicitNonNullTypesAttribute" xml:space="preserve">
<value>Explicit application of 'System.Runtime.CompilerServices.NonNullTypesAttribute' is not allowed.</value>
</data>
<data name="ERR_NullableDirectiveQualifierExpected" xml:space="preserve">
<value>Expected enable or disable</value>
</data>
Expand Down
Loading