Add NullableContextAttribute #36152

cston · 2019-06-04T16:24:39Z

Emit a NullableContextAttribute at each type and method where the byte value is the most common NullableAttribute (and NullableContextAttribute) value within that scope. The redundant NullableAttribute (and NullableContextAttribute) attributes within the scope are dropped.

This is the third part of three changes to address #35888.

AlekseyTs · 2019-06-04T18:46:06Z

docs/features/nullable-metadata.md

+
+The first case, where all parts are oblivious in an oblivious context, matches the representation in legacy assemblies where there are no `NullaleAttribute` annotations.
+
+`NullableAttribute(1)` should be placed on a type parameter definition that has a `class!` constraint.


Consider adding an entry for notnull #Closed

AlekseyTs · 2019-06-04T18:49:09Z

docs/features/nullable-metadata.md

+
+The first case, where all parts are oblivious in an oblivious context, matches the representation in legacy assemblies where there are no `NullaleAttribute` annotations.
+
+`NullableAttribute(1)` should be placed on a type parameter definition that has a `class!` constraint.


Are attributes in this list subject to NullableContextAttribute optimizations? If yes, we might need to emit NullableAttribute(0) within some contexts #Closed

AlekseyTs · 2019-06-04T18:59:57Z

docs/features/nullable-metadata.md

+
+`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 type constraints, `class`, `struct` and `unmanaged` constraints and


Consider adjusting this for notnull #Closed

AlekseyTs · 2019-06-04T19:00:44Z

docs/features/nullable-metadata.md

+`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 type constraints, `class`, `struct` and `unmanaged` constraints and
+is declared in a context where nullable type annotations are allowed, that is eqivalent to having an `object?` constraint.


Typo "eqivalent". #Closed

AlekseyTs · 2019-06-04T19:02:30Z

docs/features/nullable-metadata.md

+namespace System.Runtime.CompilerServices
+{
+    [System.AttributeUsage(
+        AttributeTargets.Assembly |


Should be replaced with a Module target instead #Closed

AlekseyTs · 2019-06-04T19:06:13Z

docs/features/nullable-metadata.md

+The attribute constructor takes a `bool enabled` argument:
+if `true`, reference types are interpreted as not nullable;
+if `false`, reference types are interpreted as oblivious.
+If there is no `NullableContextAttribute` on the containing member or containing types or assembly,


module vs. assembly #Closed

AlekseyTs · 2019-06-04T19:08:32Z

docs/features/nullable-metadata.md

+
+The attribute is not inherited.
+
+The C#8 compiler will recognize `NullableContextAttribute` instances on the assembly, types (top-level and nested types),


module vs. assembly #Closed

AlekseyTs · 2019-06-04T19:11:24Z

docs/features/nullable-metadata.md

+
+The C#8 compiler will recognize `NullableContextAttribute` instances on the assembly, types (top-level and nested types),
+and members.
+`NullableContextAttribute` will be ignored on other targets.


Is including other targets as valid in attribute definition safe? I think it is better to disallow the attributes on targets that we do not plan to recognize. #Closed

AlekseyTs · 2019-06-04T19:12:25Z

docs/features/nullable-metadata.md

+The C#8 compiler will recognize `NullableContextAttribute` instances on the assembly, types (top-level and nested types),
+and members.
+`NullableContextAttribute` will be ignored on other targets.
+Other forms of `NullableContextAttribute` on the expected targets will be treated as `NullableContext(false)`.


What other forms? #Closed

AlekseyTs · 2019-06-04T19:23:28Z

docs/features/nullable-metadata.md

+
+The C#8 compiler will emit `NullableContextAttribute(true)` on top-level types
+that contain any members (including members of nested types) with reference types that are not oblivious.
+No other `NullableContextAttribute` instances will be emitted on types or members,


If we don't emit it on nested types, it would be hard to test that we properly recognize the attribute on a nested type in metadata. #Closed

Same goes for other forms of the attribute. #Closed

AlekseyTs · 2019-06-04T19:25:00Z

docs/features/nullable-metadata.md

+The C#8 compiler will emit `NullableContextAttribute(true)` on top-level types
+that contain any members (including members of nested types) with reference types that are not oblivious.
+No other `NullableContextAttribute` instances will be emitted on types or members,
+and no assembly-level attribute will be emitted.


module vs. assembly #Closed

AlekseyTs · 2019-06-04T19:27:33Z

docs/features/nullable-reference-types.md

-
-```c#
-// C# representation of metadata
-[Nullable(2)]


We might want to preserve the examples, perhaps move them to the other file. #Closed

Added some examples.

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

AlekseyTs · 2019-06-04T21:43:35Z

docs/features/nullable-metadata.md

+The following `NullableAttribute` cases are recognized and emitted for type parameter definitions:
+1. `notnull` constraint in oblivious [emit context](#NullableContextAttribute): `NullableAttribute(1)`
+2. `class` constraint in `#nullable enable` and in oblivious [emit context](#NullableContextAttribute): `NullableAttribute(1)`
+2. `class` constraint in `#nullable disable` and in not annotated [emit context](#NullableContextAttribute): `NullableAttribute(0)`


[](start = 0, length = 2)

Duplicate number #Closed

AlekseyTs · 2019-06-04T21:46:55Z

docs/features/nullable-metadata.md

+2. `class` constraint in `#nullable enable` and in oblivious [emit context](#NullableContextAttribute): `NullableAttribute(1)`
+2. `class` constraint in `#nullable disable` and in not annotated [emit context](#NullableContextAttribute): `NullableAttribute(0)`
+3. `class?` constraint: `NullableAttribute(2)`
+4. No `notnull`, `class`, `struct`, `unmanaged`, or type constraints in `#nullable enable` (the equivalent of an `object?` constraint): `NullableAttribute(2)`


In 'not annotated' context:
notnull - No attribute
No constraints in #nullable enable - NullableAttribute(2)
No constraints in #nullable disable - If we do not emit anything, how do we distinguish this case from notnull?

It might be good to enumerate all scenarios per NullableContextAttribute (rather than having items corresponding to different contexts in one list) to make sure there is no conflict/ambiguity.
#Closed

AlekseyTs · 2019-06-04T21:56:37Z

docs/features/nullable-metadata.md

+```C#
+[Nullable(2)] string s;      // string?
+[Nullable({ 2, 1, 2 }] Dictionary<string, object> d; // Dictionary<string!, object?>?
+[Nullable({ 2, 1 }] int[] a; // int[]?


[Nullable({ 2, 1 }] [](start = 0, length = 19)

I think today we emit [Nullable({ 2, 0 }]. Are you proposing a change? #Closed

AlekseyTs · 2019-06-04T21:57:09Z

docs/features/nullable-metadata.md

+[Nullable(2)] string s;      // string?
+[Nullable({ 2, 1, 2 }] Dictionary<string, object> d; // Dictionary<string!, object?>?
+[Nullable({ 2, 1 }] int[] a; // int[]?
+[Nullable(1)] int[] b;       // int[]!


I think today we emit [Nullable({ 1, 0 }]. Are you proposing a change? #Closed

In a NullableContext(true) emit context, we should use 1 for value types.

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

In a NullableContext(true) emit context, we should use 1 for value types.

Why do we need to emit any attribute for this type in NullableContext(true) ?

In reply to: 290519517 [](ancestors = 290519517,290513550)

AlekseyTs · 2019-06-04T21:57:40Z

docs/features/nullable-metadata.md

+[Nullable({ 2, 1, 2 }] Dictionary<string, object> d; // Dictionary<string!, object?>?
+[Nullable({ 2, 1 }] int[] a; // int[]?
+[Nullable(1)] int[] b;       // int[]!
+[Nullable(0)] int[] c;       // int[]~


I think today we do not emit an attribute. Are you proposing a change? #Closed

We will need to emit an attribute in a NullableContext(true) emit context.

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

AlekseyTs · 2019-06-04T22:01:01Z

docs/features/nullable-metadata.md

+The attribute constructor takes a `bool enabled` argument:
+if `true`, reference types are interpreted as not nullable;
+if `false`, reference types are interpreted as oblivious.
+If there is no `NullableContextAttribute` on containing types or the module, unannotated reference types are interpreted as oblivious.


unannotated [](start = 77, length = 11)

Does this refer to the absence of the NullableAttribute? #Closed

AlekseyTs · 2019-06-04T22:02:49Z

docs/features/nullable-metadata.md

+The C#8 compiler will recognize `NullableContextAttribute(bool)` instances on the module and (top-level and nested) types.
+The only valid targets for `NullableContextAttribute` are module and type - attributes on other targets will be ignored silently.
+
+The C#8 compiler will not emit a module-level `NullableContextAttribute`.


Are users allowed to apply the attribute in source? On any level. #Closed

AlekseyTs · 2019-06-04T22:08:24Z

docs/features/nullable-reference-types.md


 Trivial/optimized cases:
-1)	All parts are NotAnnotated – a NullableAttribute with a single value 1 (rather than an array of 1s)
+1)	All parts are NotAnnotated � a NullableAttribute with a single value 1 (rather than an array of 1s)


� [](start = 30, length = 1)

Is this an intentional change? #Closed

AlekseyTs · 2019-06-04T22:08:33Z

docs/features/nullable-reference-types.md

 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.
+3)	All parts are Oblivious � the attribute is omitted, this matches how we interpret the lack of an attribute in legacy assemblies.


� [](start = 27, length = 1)

Is this an intentional change? #Closed

AlekseyTs · 2019-06-04T22:57:05Z

docs/features/nullable-metadata.md

+1. No constraints (see below) in `#nullable disable`: `NullableAttribute(0)`
+1. No constraints (see below) in `#nullable enable`: `NullableAttribute(2)`
+
+"No constraints" (see above) means no `notnull`, `class`, `struct`, `unmanaged`, or type constraints - the equivalent of an `object?` constraint.


the equivalent of an object? constraint. [](start = 103, length = 42)

When #nullable enable ? #Closed

AlekseyTs · 2019-06-04T22:59:14Z

docs/features/nullable-metadata.md

+}
+class B
+{
+   [Nullable({ 2, 1 }] int[] a; // int[]?


[Nullable({ 2, 1 }] int[] a; // int[]? [](start = 3, length = 38)

I think today we emit [Nullable({ 2, 0 }]. Are you proposing a change? #Closed

AlekseyTs · 2019-06-04T23:00:05Z

docs/features/nullable-metadata.md

+class B
+{
+   [Nullable({ 2, 1 }] int[] a; // int[]?
+   [Nullable(1)] int[] b;       // int[]!


[Nullable(1)] int[] b; // int[]! [](start = 3, length = 38)

I think today we emit [Nullable({ 1, 0 }]. Are you proposing a change? #Closed

AlekseyTs · 2019-06-04T23:03:25Z

docs/features/nullable-metadata.md

+{
+   [Nullable({ 2, 1 }] int[] a; // int[]?
+   int[] b;                     // int[]!
+   [Nullable(0)] int[] c;       // int[]~


[Nullable(0)] int[] c; // int[]~ [](start = 3, length = 38)

For consistency with C.a I would expect to see [Nullable({ 0, 1 }] here. Unless you are proposing to make encoding for value types dependent on flags for other constituent parts within the same type reference. Please clarify. #Closed

Yes that is the proposal. Added a comment.

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

AlekseyTs

LGTM (iteration 24), modulo suspicious visit for property parameters in NullableAttributesVisitor

This reverts commit f886470.

cston force-pushed the 35888 branch 3 times, most recently from 9e91a03 to 0b2f0d8 Compare June 4, 2019 18:34

AlekseyTs reviewed Jun 4, 2019

View reviewed changes

cston force-pushed the 35888 branch from 0b2f0d8 to 7c59681 Compare June 4, 2019 21:13

AlekseyTs reviewed Jun 4, 2019

View reviewed changes

cston force-pushed the 35888 branch from bd9acae to 2f8f8df Compare June 4, 2019 23:25

cston added 19 commits June 25, 2019 10:02

More tests

35b4a24

PR feedback

6eda1db

PR feedback

e9069c9

Fix test

b92ae07

Report error for explicit use of attributes

6db6043

Fix formatting

b5a3b73

PR feedback

6acd5ca

Add NullablePublicOnlyAttribute.IncludesInternals field

14a9376

Move check for NullablePublicOnlyAttribute

1858ffe

PR feedback

f610924

Invoke GetNullableContextValue lazily

510fcfb

PR feedback

a40a302

Cache full nullable context value

f83fd06

Delegate to containing symbol for accessibility

5ed7323

PR feedback

b64cf78

PR feedback

2dfe52a

Use GetInterfacesToEmit

b238d9c

Move handling of dynamic and tuple attributes

12648ee

Misc.

b40be6d

cston force-pushed the 35888 branch from 549f7e8 to b40be6d Compare June 25, 2019 17:06

Fix typo

2ca9546

AlekseyTs approved these changes Jun 25, 2019

View reviewed changes

Fix test

f28acab

cston merged commit f886470 into dotnet:master Jun 25, 2019

cston deleted the 35888 branch June 25, 2019 19:48

cston mentioned this pull request Jun 25, 2019

Reduce the impact of nullable attributes in metadata #35888

Closed

agocke added a commit to agocke/roslyn that referenced this pull request Jun 26, 2019

Revert "Add NullableContextAttribute (dotnet#36152)"

6f13393

This reverts commit f886470.

agocke added a commit to agocke/roslyn that referenced this pull request Jun 28, 2019

Revert "Add NullableContextAttribute (dotnet#36152)"

a497961

This reverts commit f886470.

cston mentioned this pull request Jul 3, 2019

Report error for explicit use of NullablePublicOnlyAttribute in source #36457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NullableContextAttribute #36152

Add NullableContextAttribute #36152

cston commented Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

cston Jun 4, 2019

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

cston Jun 4, 2019

AlekseyTs Jun 4, 2019

AlekseyTs Jun 4, 2019 •

edited

Loading

cston Jun 4, 2019

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

cston Jun 4, 2019

AlekseyTs left a comment


		The first case, where all parts are oblivious in an oblivious context, matches the representation in legacy assemblies where there are no `NullaleAttribute` annotations.

		`NullableAttribute(1)` should be placed on a type parameter definition that has a `class!` constraint.


		The attribute is not inherited.

		The C#8 compiler will recognize `NullableContextAttribute` instances on the assembly, types (top-level and nested types),

Add NullableContextAttribute #36152

Add NullableContextAttribute #36152

Conversation

cston commented Jun 4, 2019 • edited Loading

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jun 4, 2019

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jun 4, 2019

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jun 4, 2019

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jun 4, 2019

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading

AlekseyTs Jun 4, 2019 •

edited

Loading