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

Conversation

cston
Copy link
Member

@cston cston commented Jun 13, 2019

Relates to #35816 (work items for annotations and metadata compaction)

@cston cston requested a review from a team as a code owner June 13, 2019 06:04
@cston cston force-pushed the private-members branch from 6f3496e to ce358a7 Compare June 13, 2019 15:08
`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 `private` members,
Copy link
Member

@chsienki chsienki Jun 13, 2019

Choose a reason for hiding this comment

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

attributes for private #Resolved

this.HasNotNullConstraint ||
(!this.HasReferenceTypeConstraint && !this.HasValueTypeConstraint &&
this.ConstraintTypesNoUseSiteDiagnostics.IsEmpty && this.IsNotNullableIfReferenceType == false);
if (!DeclaringCompilation.ShouldEmitNullableAttributes(ContainingSymbol))
Copy link
Member

@chsienki chsienki Jun 13, 2019

Choose a reason for hiding this comment

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

You might consider putting a link the nullable attribute spec here, so it's easier to understand the logic #Resolved

@@ -0,0 +1,10 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
Copy link
Member

Choose a reason for hiding this comment

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

Keep this file?

@chsienki
Copy link
Member

LGTM, Modulo the failing tests for expression evaluation

@cston cston force-pushed the private-members branch from ceb8115 to 5be3827 Compare June 13, 2019 19:36
@@ -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)

[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)

```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

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)


_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

_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._

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

@@ -538,6 +539,7 @@ static AttributeDescription()
internal static readonly AttributeDescription AssemblyAlgorithmIdAttribute = new AttributeDescription("System.Reflection", "AssemblyAlgorithmIdAttribute", s_signaturesOfAssemblyAlgorithmIdAttribute);
internal static readonly AttributeDescription DeprecatedAttribute = new AttributeDescription("Windows.Foundation.Metadata", "DeprecatedAttribute", s_signaturesOfDeprecatedAttribute);
internal static readonly AttributeDescription NullableAttribute = new AttributeDescription("System.Runtime.CompilerServices", "NullableAttribute", s_signaturesOfNullableAttribute);
internal static readonly AttributeDescription NullableMembersAttribute = new AttributeDescription("System.Runtime.CompilerServices", "NullableMembersAttribute", s_signaturesOfNullableMembersAttribute);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 13, 2019

Choose a reason for hiding this comment

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

NullableMembersAttribute [](start = 54, length = 24)

It doesn't looks like we pay attention to this attribute on import. Are we using this description anywhere? Perhaps we should disallow an explicit attribute application in source. #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.

Logged #36457.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

Logged #36457.

Would it make sense to revert changes in this file if they are not used/tested.


In reply to: 293911855 [](ancestors = 293911855,293557364)

Copy link
Member Author

Choose a reason for hiding this comment

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

This AttributeDescription is used by PEAssemblyBuilder in determining whether to synthesize the type.


In reply to: 293917589 [](ancestors = 293917589,293911855,293557364)

/// (public, protected, and if any [InternalsVisibleTo] attributes, internal members).
/// If false, attributes are emitted for all members regardless of visibility.
/// </summary>
internal bool EmitPublicNullableMetadataOnly { get; private set; }
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.

EmitPublicNullableMetadataOnly [](start = 22, length = 30)

From discussion today, we'll use a feature flag for now. #Resolved

@@ -57,6 +56,52 @@ internal static class AccessCheck
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

// Note: we don't need to warn on annotations used without NonNullTypes context for local functions, as this is handled in binding already
var compilation = DeclaringCompilation;
ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, parameters, diagnostics, modifyCompilation: false);
ParameterHelpers.EnsureNullableAttributeExists(compilation, this, parameters, diagnostics, modifyCompilation: false);
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.

EnsureNullableAttributeExists [](start = 29, length = 29)

Should we guard this call to EnsureNullableAttributeExists with an accessibility check? Never mind, ParameterHelpers does that #Closed

ParameterHelpers.EnsureNullableAttributeExists(Parameters, diagnostics, modifyCompilation: true);
var compilation = DeclaringCompilation;
ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, Parameters, diagnostics, modifyCompilation: true);
ParameterHelpers.EnsureNullableAttributeExists(compilation, this, Parameters, diagnostics, modifyCompilation: true);
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.

EnsureNullableAttributeExists [](start = 29, length = 29)

Same question here (should we guard with an access check?) Never mind, ParameterHelpers does that #Closed

@jcouv jcouv added this to the 16.2.P4 milestone Jun 13, 2019

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SynthesizedEmbeddedNullableMembersAttributeSymbol : SynthesizedEmbeddedAttributeSymbolBase
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.

SynthesizedEmbeddedNullableMembersAttributeSymbol [](start = 26, length = 49)

I'll skip reviewing this file in this iteration, since changes are planned #Closed

@@ -44,6 +44,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
base.AddSynthesizedAttributes(moduleBuilder, ref attributes);

CSharpCompilation compilation = this.DeclaringCompilation;
var type = this.TypeWithAnnotations;
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.

type [](start = 16, length = 4)

nit: typeWithAnnotations would be better name to avoid confusion with this.Type below. Consider declaring a local var type = typeWithAnnotations.Type to replace this.Type. #Closed

@@ -51,6 +51,26 @@ public NullableAttribute(byte[] transformFlags)
}
";

protected const string NullableMembersAttributeDefinition = @"
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.

NullableMembersAttributeDefinition [](start = 31, length = 34)

seems unused at the moment #Closed

VisitList(@namespace.GetMembers());
}

public override void VisitNamedType(NamedTypeSymbol type)
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.

VisitNamedType [](start = 29, length = 14)

Should we visit constraints? (same comment for VisitMethod) #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.

Visiting constraints doesn't fit easily into this model since we're assuming all symbols are definitions.


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

return new string(' ', level * 4);
}

private static readonly SymbolDisplayFormat _displayFormat =
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.

_displayFormat [](start = 52, length = 14)

Assuming this is a variant of an existing format, could it be expressed as ExistingFormat.With(change) instead? I think that is easier to maintain when we add new options. #Closed


private void ReportSymbol(Symbol symbol, bool includeAlways = false)
{
var attributes = (symbol.Kind == SymbolKind.Method) ? ((MethodSymbol)symbol).GetReturnTypeAttributes() : symbol.GetAttributes();
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.

(symbol.Kind == SymbolKind.Method) [](start = 29, length = 34)

nit: consider a type test symbol is MethodSymbol method ? method. ... #Closed

return;
}
ReportContainingSymbols(symbol.ContainingSymbol);
ReportSymbol(symbol, includeAlways: true);
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.

, includeAlways: true [](start = 31, length = 21)

I didn't understand this part. We always want to report containing symbols? #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.

When we need to report a specific symbol, we first report all ancestors, from the outermost type down.


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


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

/// Returns true if the symbol is effectively public or internal based on
/// the declared accessibility of the symbol and any containing symbols.
/// </summary>
internal static bool IsPublicOrInternal(Symbol symbol, out bool isInternal)
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.IsDefinition);

var sourceAssembly = SourceAssembly;
if (symbol.ContainingAssembly != sourceAssembly)
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.

if (symbol.ContainingAssembly != sourceAssembly) [](start = 12, length = 48)

Is this check for just in case, or do we have a scenario when it is significant? Consider switching to SourceModule comparison instead. #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.

We hit this in the EE.


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

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.

We hit this in the EE.

Then comparing the module is the right check instead, I think.


In reply to: 293924981 [](ancestors = 293924981,293924012)


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)

foreach (var parameter in parameters)
{
if (parameter.TypeWithAnnotations.NeedsNullableAttribute())
if (compilation.ShouldEmitNullableAttributes(container) && parameter.TypeWithAnnotations.NeedsNullableAttribute())
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.

compilation.ShouldEmitNullableAttributes(container) [](start = 20, length = 51)

It feel like this condition can be taken out of the loop. If we don't want to evaluate this value for an empty parameters array, we could check that too. #Closed


if ((needsAttributes & EmbeddedAttributes.NullablePublicOnlyAttribute) != 0)
{
CreateEmbeddedAttributeItselfIfNeeded(diagnostics);
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.

CreateEmbeddedAttributeItselfIfNeeded(diagnostics) [](start = 16, length = 50)

Minor, I guess now we can take this out of all the '''ifs''' and call this once before them if needsAttributes is not 0. #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.

Good catch, thanks.


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

@@ -490,90 +448,85 @@ internal SynthesizedAttributeData SynthesizeDebuggerStepThroughAttribute()
return TrySynthesizeAttribute(WellKnownMember.System_Diagnostics_DebuggerStepThroughAttribute__ctor);
}

internal void EnsureIsReadOnlyAttributeExists(DiagnosticBag diagnostics, Location location, bool modifyCompilation)
private void EnsureEmbeddedAttributeExists(EmbeddedAttributes attribute, DiagnosticBag diagnostics, Location location, bool modifyCompilation)
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.

EnsureEmbeddedAttributeExists [](start = 21, length = 29)

The name is confusing:

  • We have an EmbeddedAttribute
  • The attributes are not always embedded.

Consider renaming to something like "EnsureEmbeddableAttributeExists" #Closed

namespace Microsoft.CodeAnalysis.CSharp
{
[Flags]
internal enum EmbeddedAttributes
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.

EmbeddedAttributes [](start = 18, length = 18)

"EmbeddableAttributes"? #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.

That's a better name, thanks.


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

@@ -1553,52 +1536,40 @@ protected virtual SynthesizedAttributeData TrySynthesizeIsByRefLikeAttribute()
return Compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_IsByRefLikeAttribute__ctor);
}

internal void EnsureIsReadOnlyAttributeExists()
private void EnsureEmbeddedAttributeExists(EmbeddedAttributes attribute)
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.

EnsureEmbeddedAttributeExists [](start = 21, length = 29)

"EnsureEmbeddableAttributeExists"? #Closed

if (typeParameters.Any(typeParameter => typeParameter.HasUnmanagedTypeConstraint))
{
_factory.CompilationState.ModuleBuilderOpt?.EnsureIsUnmanagedAttributeExists();
}

bool constraintsNeedNullableAttribute = typeParameters.Any(
typeParameter => ((SourceTypeParameterSymbolBase)typeParameter).ConstraintsNeedNullableAttribute());
if (_factory.CompilationState.Compilation.ShouldEmitNullableAttributes(localFunction))
Copy link
Contributor

Choose a reason for hiding this comment

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

if (_factory.CompilationState.Compilation.ShouldEmitNullableAttributes(localFunction)) [](start = 12, length = 86)

Similar suggestion as for lambdas.

}

if (returnType.NeedsNullableAttribute())
if (compilation?.ShouldEmitNullableAttributes(this) == true &&
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.

compilation? [](start = 16, length = 12)

Can compilation ever be null and why other places do not guard? #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.

Yes, compilation may be null from the EE.


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

{
var compilation = DeclaringCompilation;
return compilation.EmitNullablePublicOnly &&
compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes);
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.

compilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes) [](start = 20, length = 73)

Having this property on the Module symbol as well feels confusing, especially that it has extra conditions. Effectively, there is an inconsistency between other users of compilation.EmitNullablePublicOnly and this code. Consider incorporating the feature check into the compilation's property and getting rid of this property. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, consider renaming this property to EmitNullablePublicOnlyAttribute


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

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 13)

{
if (!_lazyEmitNullablePublicOnly.HasValue())
{
bool value = SyntaxTrees.FirstOrDefault()?.Options?.Features?.ContainsKey("nullablePublicOnly") == true;
Copy link
Member

@jcouv jcouv Jun 14, 2019

Choose a reason for hiding this comment

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

SyntaxTrees.FirstOrDefault()?.Options?.Features?.ContainsKey("nullablePublicOnly") == true; [](start = 33, length = 91)

We may already have a way of checking feature flags. I'm surprised we'd need to do this "by hand" here.

Also, should we be checking for LangVersion? #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 couldn't find an appropriate helper method for checking Features.

Regarding language version, is it necessary to add that restriction here? Presumably language version will produce the necessary warnings and errors during binding.


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

Copy link
Member

Choose a reason for hiding this comment

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

You're right, we don't have a helper. IOperations did the same...

        internal override bool IsIOperationFeatureEnabled()
        {
            var options = (CSharpParseOptions)this.SyntaxTrees.FirstOrDefault()?.Options;
            return options?.IsFeatureEnabled(MessageID.IDS_FeatureIOperation) ?? false;
        }

For LangVersion, I agree too. If you're in C# 7.3 and don't nullable annotations, then the feature flag is a no-op, so no need to report a diagnostic.


In reply to: 294017389 [](ancestors = 294017389,294014629)

{
[Fact]
public void ExplicitAttribute_FromSource()
{
Copy link
Member

@jcouv jcouv Jun 14, 2019

Choose a reason for hiding this comment

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

{ [](start = 8, length = 1)

[NullablePublicOnly] should be disallowed in source. Do we have a test for that? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

{
_needsGeneratedIsUnmanagedAttribute_Value = true;
}
internal void EnsureIsUnmanagedAttributeExists()
Copy link
Member

Choose a reason for hiding this comment

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

EnsureIsUnmanagedAttributeExists [](start = 22, length = 32)

nit: consider inlining this method (only one use as far as I can tell).
Comment may be applicable for other Ensure... methods as well.

CompileAndVerify(comp, symbolValidator: AssertNoNullablePublicOnlyAttribute);

comp = CreateCompilation(source, options: options, parseOptions: parseOptions.WithFeature("nullablePublicOnly"));
CompileAndVerify(comp, symbolValidator: AssertNullablePublicOnlyAttribute);
Copy link
Member

Choose a reason for hiding this comment

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

symbolValidator: AssertNullablePublicOnlyAttribute); [](start = 35, length = 52)

consider validating that no NullablePublicOnly attribute appears on source symbols (validator: AssertNoNullablePublicOnlyAttribute)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 16)

@cston cston merged commit 0bc5071 into dotnet:master Jun 14, 2019
@cston cston deleted the private-members branch June 14, 2019 23:29
333fred added a commit to 333fred/roslyn that referenced this pull request Jun 18, 2019
…-types

* dotnet/master: (63 commits)
  Fix stack overflow in requesting syntax directives (dotnet#36347)
  crash on ClassifyUpdate for EventFields (dotnet#35962)
  Disable move type when the options service isn't present (dotnet#36334)
  Fix crash where type inference doing method inference needs to drop nullability
  Fix parsing bug in invalid using statements (dotnet#36428)
  Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression
  Add option to emit nullable metadata for public members only (dotnet#36398)
  Added null checks on F# external access services (dotnet#36469)
  Deal with discovering extra .editorconfig files
  Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery
  Add support to VisualStudioMSBuildInstalled to support minimum versions
  Fix configuration of accessibilities in editorconfig
  Shorten a resource ID
  Revert "Extract the RDT implementation for Misc files and VS open file tracker"
  Add nullability support to use local function
  Add EditorFeatures.WPF dependency to F# ExternalAccess
  Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406)
  Replace `dynamic` with `object` when substituting constraints. (dotnet#36379)
  Add some string descriptions
  Adjust type of out var based on parameter state (dotnet#36284)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants