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

Add option to emit nullable metadata for public members only #36398

Merged
merged 16 commits into from
Jun 14, 2019
191 changes: 191 additions & 0 deletions docs/features/nullable-metadata.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
Nullable Metadata
Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

nit: FWIW, I'd prefer if we merged this doc into the main design doc. #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.

I'd prefer to see this as a separate document because the metadata spec is now longer and can stand alone.


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

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 14, 2019

Choose a reason for hiding this comment

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

In this document, consider reflecting actual behavior of the compiler in this PR, excluding any information about possible future changes in this space. This should help to avoid confusion about expectations from this PR. #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.

I agree, the state is currently confusing. But I'd rather avoid making extra changes here that will be reverted. Perhaps I could follow up and correct this document if we don't make all the changes in the near future.


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

=========
The following describes the representation of nullable annotations in metadata.

## NullableAttribute
Type references are annotated in metadata with a `NullableAttribute`.

```C#
namespace System.Runtime.CompilerServices
{
[AttributeUsage(
AttributeTargets.Class |
AttributeTargets.Event |
AttributeTargets.Field |
AttributeTargets.GenericParameter |
AttributeTargets.Parameter |
AttributeTargets.Property |
AttributeTargets.ReturnValue,
AllowMultiple = false)]
public sealed class NullableAttribute : Attribute
{
public readonly byte[] Flags;
public NullableAttribute(byte flag)
{
Flags = new byte[] { flag };
}
public NullableAttribute(byte[] flags)
{
Flags = flags;
}
}
}
```

The `NullableAttribute` type is for compiler use only - it is not permitted in source.
The type declaration is synthesized by the compiler if not already included in the compilation.

Each type reference in metadata may have an associated `NullableAttribute` with a `byte[]` where each `byte`
represents nullability: 0 for oblivious, 1 for not annotated, and 2 for annotated.

The `byte[]` is constructed as follows:
- Reference type: the nullability (0, 1, or 2), followed by the representation of the type arguments in order including containing types
- Nullable value type: the representation of the type argument only
- Value type: the representation of the type arguments in order including containing types
- Array: the nullability (0, 1, or 2), followed by the representation of the element type
- Tuple: the representation of the tuple elements in order
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 14, 2019

Choose a reason for hiding this comment

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

Is this accurate? I thought we simply annotated the underlying type. #Closed

- Type parameter reference: the nullability (0, 1, or 2, with 0 for unconstrained type parameter)

Note that certain type references are represented by an empty `byte[]`:
- Non-generic value types, and
- Nullable value types and tuples where all elements are represented by an empty `byte[]`.

### Optimizations
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we no longer omitting NullableAttribute(0)?


If the `byte[]` is empty, the `NullableAttribute` is omitted.

If all values in the `byte[]` are the same, the `NullableAttribute` is constructed with that single `byte` value. (For instance, `NullableAttribute(1)` rather than `NullableAttribute(new byte[] { 1, 1 }))`.)

### Type parameters
Each type parameter definition may have an associated `NullableAttribute` with a single `byte`:

1. `notnull` constraint: `NullableAttribute(1)`
2. `class` constraint in `#nullable disable` context: `NullableAttribute(0)`
3. `class` constraint in `#nullable enable` context: `NullableAttribute(1)`
4. `class?` constraint: `NullableAttribute(2)`
5. No `notnull`, `class`, `struct`, `unmanaged`, or type constraints in `#nullable disable` context: `NullableAttribute(0)`
6. No `notnull`, `class`, `struct`, `unmanaged`, or type constraints in `#nullable enable` context
(equivalent to an `object?` constraint): `NullableAttribute(2)`

## NullableContextAttribute
`NullableContextAttribute` can be used to indicate the nullability of type references that have no `NullableAttribute` annotations.

```C#
namespace System.Runtime.CompilerServices
{
[System.AttributeUsage(
AttributeTargets.Module |
AttributeTargets.Class |
AttributeTargets.Delegate |
Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

Delegate [](start = 25, length = 8)

Why allow on Delegate? #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Why allow on Delegate?

I think, with respect to this attribute, delegates should be valid targets as other classes


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

AttributeTargets.Interface |
AttributeTargets.Method |
AttributeTargets.Struct,
AllowMultiple = false,
Inherited = false)]
public sealed class NullableContextAttribute : Attribute
{
public readonly byte Flag;
public NullableContextAttribute(byte flag)
{
Flag = flag;
}
}
}
```

The `NullableContextAttribute` type is for compiler use only - it is not permitted in source.
The type declaration is synthesized by the compiler if not already included in the compilation.

The `NullableContextAttribute` is optional - nullable annotations can be represented in metadata with full fidelity using `NullableAttribute` only.

`NullableContextAttribute` is valid in metadata at the module level and at type and method declarations.
The `byte` value represents the implicit `NullableAttribute` value for type references within that scope
that do not have an explicit `NullableAttribute` and would not otherwise be represented by an empty `byte[]`.
The nearest `NullableContextAttribute` in the metadata hierarchy applies.
If there are no `NullableContextAttribute` attributes in the hierarchy,
missing `NullableAttribute` attributes are treated as `NullableAttribute(0)`.

The attribute is not inherited.

The C#8 compiler uses the following algorithm to determine which `NullableAttribute` and
`NullableContextAttribute` attributes to emit.
First, `NullableAttribute` attributes are generated at each type reference and type parameter definition by:
calculating the `byte[]`, skipping empty `byte[]`, and collapsing `byte[]` to single `byte` where possible.
Then at each level in metadata hierarchy starting at methods:
The compiler finds the most common single `byte` value across all the `NullableAttribute` attributes at that level
and any `NullableContextAttribute` attributes on immediate children.
If there are no single `byte` values, there are no changes.
Otherwise, a `NullableContext(value)` attribute is created at that level where `value` is most common
value (preferring `0` over `1` and preferring `1` over `2`), and all `NullableAttribute` and `NullableContextAttribute` attributes with that value are removed.
That iterative process continues up to the module level.
If the common value at the module level is a value other than `0` (the default), a module level `NullableContext(value)` attribute is emitted.

Note that an assembly compiled with C#8 where all reference types are oblivious will have no
`NullableContextAttribute` and no `NullableAttribute` attributes emitted.
That is equivalent to a legacy assembly.

### Examples
```C#
// C# representation of metadata
[NullableContext(2)]
class Program
{
string s; // string?
[Nullable({ 2, 1, 2 }] Dictionary<string, object> d; // Dictionary<string!, object?>?
[Nullable(1)] int[] a; // int[]!
int[] b; // int[]?
[Nullable({ 0, 2 })] object[] c; // object?[]~
}
```

## NullableMembersAttribute

`NullableMembersAttribute` can be used to indicate whether nullable attributes were included for `private` or `internal` members.

```C#
namespace System.Runtime.CompilerServices
{
public enum NullableMembers
Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

NullableMembers [](start = 16, length = 15)

Out-of-date from our discussion today #Closed

{
Public = 0, // public and protected only
Internal = 1, // public, protected, internal
All = 2,
}
[System.AttributeUsage(AttributeTargets.Module, AllowMultiple = false)]
public sealed class NullableMembersAttribute : Attribute
{
public readonly NullableMembers Members;
public NullableMembersAttribute(NullableMembers members)
{
Members = members;
}
}
}
```

The `NullableMembersAttribute` type is for compiler use only - it is not permitted in source.
The type declaration is synthesized by the compiler if not already included in the compilation.

The `NullableMembersAttribute` must be emitted if nullable attributes are dropped for
`private` or `internal` members to allow tools to correctly interpret the nullability of members
without explicit `NullableAttribute` attributes.

To reduce the size of metadata, the C#8 compiler will not emit attributes for `private` members,
and the compiler will only emit attributes for `internal` members if the assembly contains
Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

assembly [](start = 73, length = 8)

module? #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.

InternalsVisibleToAttribute is an assembly-level attribute.


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

`InternalsVisibleToAttribute` attributes.
The compiler will emit a `[module: NullableMembers(...)]` attribute to indicate which members were included.

_If necessary, a compiler option could be added in a future release to override the default behavior and
explicitly emit or drop nullable attributes for `private` or `internal` members._

Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

Out-of-date? #Closed

## Compatibility
Copy link
Member

Choose a reason for hiding this comment

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

Compatibility [](start = 3, length = 13)

Not related to this PR: when making the compaction change, let's add an entry to the breaking changes doc, even though this is only a break from previous previews.


The nullable metadata does not include an explicit version number.
Where possible, the compiler will silently ignore attribute forms that are unexpected.

The metadata format described here is incompatible with the format used by earlier C#8 previews:
1. Concrete non-generic value types are no longer included in the `byte[]`, and
2. `NullableContextAttribute` attributes are used in place of explicit `NullableAttribute` attributes.

Those differences mean that assemblies compiled with earlier previews may be read incorrectly by later previews,
and assemblies compiled with later previews may be read incorrectly by earlier previews.
64 changes: 1 addition & 63 deletions docs/features/nullable-reference-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,70 +22,8 @@ Dictionary<string, object?>? OptDictionaryOptValues; // dictionary may be null,
```
A warning is reported when annotating a reference type with `?` outside a `#nullable` context.

In metadata, nullable reference types are annotated with a `[Nullable]` attribute.
```c#
namespace System.Runtime.CompilerServices
{
[AttributeUsage(
AttributeTargets.Class |
AttributeTargets.GenericParameter |
AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Property |
AttributeTargets.Parameter | AttributeTargets.ReturnValue,
AllowMultiple = false)]
public sealed class NullableAttribute : Attribute
{
public readonly byte[] NullableFlags;

public NullableAttribute(byte b)
{
NullableFlags = new byte[] { b };
}

public NullableAttribute(byte[] b)
{
NullableFlags = b;
}
}
}
```

Each type reference is accompanied by a NullableAttribute with an array of bytes, where 0 is Oblivious, 1 is NotAnnotated and 2 is Annotated.
All value types are marked with flag 0 (oblivious).

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.
[Metadata representation](nullable-metadata.md)

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 `notnull` constraint.
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.
NullableAttribute(2) should be placed on a type parameter definition that has no `notnull`, `class`, `struct`, `unmanaged` and type constraints and
is declared in a context where nullable type annotations are allowed, that is equivalent to having an `object?` constraint.
Other forms of NullableAttribute are not emitted on type parameter definitions and are not specially recognized on them.

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
[Nullable(2)]
string OptString; // string?
[Nullable(new[] { 2, 1, 2 })]
Dictionary<string, object> OptDictionaryOptValues; // Dictionary<string!, object?>?
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?[]!
int Int; // int
Nullable<int> NullableInt1; // Nullable<int>
```
## Declaration warnings
_Describe warnings reported for declarations in initial binding._

Expand Down
47 changes: 46 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/AccessCheck.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -57,6 +56,52 @@ public static bool IsSymbolAccessible(
return IsSymbolAccessibleCore(symbol, within, throughTypeOpt, out failedThroughTypeCheck, within.DeclaringCompilation, ref useSiteDiagnostics, basesBeingResolved);
}

internal static bool IsPublicOrInternal(Symbol symbol, out bool isInternal)
Copy link
Member

@jcouv jcouv Jun 13, 2019

Choose a reason for hiding this comment

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

IsPublicOrInternal [](start = 29, length = 18)

nit: xml doc would be useful here #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 14, 2019

Choose a reason for hiding this comment

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

IsPublicOrInternal [](start = 29, length = 18)

Consider including "effectively" in the name for clarity. Something like "IsEffectivelyPublicOrInternal". #Closed

{
Debug.Assert(!(symbol is null));

switch (symbol.Kind)
{
case SymbolKind.NamedType:
case SymbolKind.Event:
case SymbolKind.Field:
case SymbolKind.Method:
case SymbolKind.Property:
break;
case SymbolKind.TypeParameter:
symbol = symbol.ContainingSymbol;
break;
default:
throw ExceptionUtilities.UnexpectedValue(symbol.Kind);
}

isInternal = false;

do
{
switch (symbol.DeclaredAccessibility)
{
case Accessibility.Public:
case Accessibility.Protected:
case Accessibility.ProtectedOrInternal:
break;
case Accessibility.Internal:
case Accessibility.ProtectedAndInternal:
isInternal = true;
break;
case Accessibility.Private:
return false;
default:
throw ExceptionUtilities.UnexpectedValue(symbol.DeclaredAccessibility);
}

symbol = symbol.ContainingType;
}
while (!(symbol is null));
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 14, 2019

Choose a reason for hiding this comment

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

!(symbol is null) [](start = 19, length = 17)

symbol is object? #Closed


return true;
}

/// <summary>
/// Checks if 'symbol' is accessible from within 'within', which must be a NamedTypeSymbol
/// or an AssemblySymbol.
Expand Down
24 changes: 13 additions & 11 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
BoundBlock block;

var diagnostics = DiagnosticBag.GetInstance();
var compilation = Binder.Compilation;

// when binding for real (not for return inference), there is still
// a good chance that we could reuse a body of a lambda previously bound for
Expand All @@ -516,7 +517,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
}

lambdaSymbol = new LambdaSymbol(
Binder.Compilation,
compilation,
Binder.ContainingMemberOrLambda,
_unboundLambda,
cacheKey.ParameterTypes,
Expand All @@ -528,23 +529,24 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)

if (lambdaSymbol.RefKind == CodeAnalysis.RefKind.RefReadOnly)
{
Binder.Compilation.EnsureIsReadOnlyAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilation: false);
compilation.EnsureIsReadOnlyAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilation: false);
}

var lambdaParameters = lambdaSymbol.Parameters;
ParameterHelpers.EnsureIsReadOnlyAttributeExists(lambdaParameters, diagnostics, modifyCompilation: false);
ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, lambdaParameters, diagnostics, modifyCompilation: false);

if (returnType.HasType)
{
if (returnType.NeedsNullableAttribute())
if (compilation.ShouldEmitNullableAttributes(lambdaSymbol) &&
returnType.NeedsNullableAttribute())
Copy link
Contributor

Choose a reason for hiding this comment

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

returnType.NeedsNullableAttribute() [](start = 20, length = 35)

Should we stop emitting nullable attributes for any synthesized code not meant for direct consumption?

Copy link
Member Author

Choose a reason for hiding this comment

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

Logged #36466.


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

{
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
compilation.EnsureNullableAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilation: false);
// Note: we don't need to warn on annotations used in #nullable disable context for lambdas, as this is handled in binding already
}
}

ParameterHelpers.EnsureNullableAttributeExists(lambdaParameters, diagnostics, modifyCompilation: false);
// Note: we don't need to warn on annotations used without NonNullTypes context for lambdas, as this is handled in binding already
ParameterHelpers.EnsureNullableAttributeExists(compilation, lambdaSymbol, lambdaParameters, diagnostics, modifyCompilation: false);
// Note: we don't need to warn on annotations used in #nullable disable context for lambdas, as this is handled in binding already

block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);

Expand All @@ -553,7 +555,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)

haveLambdaBodyAndBinders:

bool reachableEndpoint = ControlFlowPass.Analyze(Binder.Compilation, lambdaSymbol, block, diagnostics);
bool reachableEndpoint = ControlFlowPass.Analyze(compilation, lambdaSymbol, block, diagnostics);
if (reachableEndpoint)
{
if (DelegateNeedsReturn(invokeMethod))
Expand All @@ -571,8 +573,8 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
{
if (returnType.HasType && // Can be null if "delegateType" is not actually a delegate type.
!returnType.IsVoidType() &&
!returnType.Type.IsNonGenericTaskType(Binder.Compilation) &&
!returnType.Type.IsGenericTaskType(Binder.Compilation))
!returnType.Type.IsNonGenericTaskType(compilation) &&
!returnType.Type.IsGenericTaskType(compilation))
{
// Cannot convert async {0} to delegate type '{1}'. An async {0} may return void, Task or Task&lt;T&gt;, none of which are convertible to '{1}'.
diagnostics.Add(ErrorCode.ERR_CantConvAsyncAnonFuncReturns, lambdaSymbol.DiagnosticLocation, lambdaSymbol.MessageID.Localize(), delegateType);
Expand Down
Loading