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

Inject NonNullTypesAttribute #29180

Merged
merged 20 commits into from
Sep 10, 2018

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 9, 2018

If no NonNullTypesAttribute type is available in source, we make one up.

  • Remaining test failures (ExpressionEvaluator, IDE)
  • check we really need to track completion on injected symbol
  • Switch away from ThreeState?
  • Detect all usages (not just as an attribute)
  • Diagnostics story
  • Remove magic marker logic for handling poisoned attribute (ie. test attribute defined in metadata is ignored)
  • Missing AttributeUsage
  • Injecting hand-crafted symbols
  • Some machinery to make parameter optional.
  • Problem with ctor body being empty.

@jcouv jcouv added this to the 16.0 milestone Aug 9, 2018
@jcouv jcouv self-assigned this Aug 9, 2018
@jcouv jcouv requested a review from a team as a code owner August 9, 2018 04:09
@@ -1093,14 +1093,20 @@ internal new NamespaceSymbol GlobalNamespace
var result = MergedNamespaceSymbol.Create(
new NamespaceExtent(this),
null,
modules.SelectDistinct(m => m.GlobalNamespace));
modules.SelectDistinct(m => m.GlobalNamespace),
injectNamespaceOpt: injectNamespace);
Copy link
Member

@cston cston Aug 9, 2018

Choose a reason for hiding this comment

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

Just curious: Why pass a delegate to a local function rather than inlining the body of the local function as a lambda? #Resolved

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 can't see what code this comment was for, and I couldn't find injectNamespaceOpt: injectNamespace);.
Is this comment still alive?


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

}
}

internal static ImmutableArray<MethodSymbol> makeConstructors(CSharpCompilation compilation, NamedTypeSymbol containingType, DiagnosticBag diagnostics)
Copy link
Member

@cston cston Aug 9, 2018

Choose a reason for hiding this comment

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

private … Make...

@jcouv
Copy link
Member Author

jcouv commented Aug 9, 2018

This is meant for design discussion, not for detailed review yet ;-) Thanks #Closed

@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 10, 2018
@jcouv jcouv force-pushed the inject-nnt branch 9 times, most recently from 98eb2c4 to 07d7944 Compare August 27, 2018 21:48
Binder.ReportUseSiteDiagnostics(ctor, Diagnostics, Location.None);

NamedTypeSymbol attributeTargets = DeclaringCompilation.GetWellKnownType(WellKnownType.System_AttributeTargets);
Binder.ReportUseSiteDiagnostics(attributeTargets, Diagnostics, Location.None);
Copy link
Member Author

@jcouv jcouv Aug 27, 2018

Choose a reason for hiding this comment

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

ReportUseSiteDiagnostics [](start = 19, length = 24)

ReportUseSiteDiagnostics [](start = 19, length = 24)

📝 I wasn't able to find scenarios for use-site diagnostics on AttributeTargets or the constructor of AttributeUsage, but I'm including those checks anyways. #Closed

@jcouv jcouv force-pushed the inject-nnt branch 4 times, most recently from e4455c1 to 39614a0 Compare August 28, 2018 06:24
@jcouv jcouv requested a review from a team as a code owner August 28, 2018 06:24
@jcouv jcouv force-pushed the inject-nnt branch 5 times, most recently from b18dba6 to 6407e6d Compare August 29, 2018 17:16
@jcouv jcouv requested a review from a team as a code owner August 29, 2018 21:44
var obj2 = new TestReference.TestType2();
var obj3 = new TestReference.TestType3();
}
}";
Copy link
Member

@cston cston Sep 7, 2018

Choose a reason for hiding this comment

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

Remove if not needed. #Resolved

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Sep 7, 2018

Consider splitting referenced assembly and added module cases. Will simply adding a module declaring the type cause an error, or it is reported only if the attribute is used? #Closed


Refers to: docs/compilers/CSharp/Compiler Breaking Changes - VS2019.md:11 in f4a2147. [](commit_id = f4a2147, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Also, I think the term "referenced" is somewhat confusing in this context. Would it be clearer to say something like: "we allowed to refer to a System.Runtime.CompilerServices.NonNullTypesAttribute type declared in a referenced assembly or an added module." BTW, I think one can still refer to a System.Runtime.CompilerServices.NonNullTypesAttribute type declared in a referenced assembly by using an extern alias. Adding a test for this scenario would be good.


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


Refers to: docs/compilers/CSharp/Compiler Breaking Changes - VS2019.md:11 in bba7cd2. [](commit_id = bba7cd293a3deacbf276165a0f93fae80ff9426a, deletion_comment = False)

*Breaks are formatted with a monotonically increasing numbered list to allow them to referenced via shorthand (i.e., "known break #1").
Each entry should include a short description of the break, followed by either a link to the issue describing the full details of the break or the full details of the break inline.*

1. Previously, we allowed the `Microsoft.CodeAnalysis.EmbeddedAttribute` to be referenced from a source module.
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

to be referenced from [](start = 73, length = 21)

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

No, it can still be declared in source. The scenario that changed is with a module added that declares that type.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it can still be declared in source. The scenario that changed is with a module added that declares that type.

So this is not about a source module, this is a bout an added module. Also consider the other comment about using "referenced" term.


In reply to: 216041844 [](ancestors = 216041844,216033962)

=> new AttributeUsageInfo(validTargets: AttributeTargets.All, allowMultiple: false, inherited: false);

internal override DiagnosticBag GetDiagnostics()
=> _diagnostics;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

=> _diagnostics [](start = 12, length = 15)

Consider avoiding exposing "private" DiagnosticBag to external consumers. That can be achieved by adding a DiagnosticBag parameter where the diagnostics should be added. #Closed


class InjectedNamespaceSymbol : NamespaceSymbol
{
private ModuleSymbol _module;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

private ModuleSymbol _module; [](start = 16, length = 29)

I think this field is not necessary. It looks like it is used only for Extent and I think we can safely return extent for _containingNamespace #Closed

class InjectedNamespaceSymbol : NamespaceSymbol
{
private ModuleSymbol _module;
private NamespaceSymbol _containingNamespace;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

private NamespaceSymbol _containingNamespace; [](start = 16, length = 45)

readonly? #Closed

internal sealed class InjectedNonNullTypesAttributeSymbol : InjectedAttributeSymbol
{
private ImmutableArray<CSharpAttributeData> _lazyCustomAttributes;
private SymbolCompletionState _state;
Copy link
Member Author

@jcouv jcouv Sep 7, 2018

Choose a reason for hiding this comment

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

_state [](start = 38, length = 6)

I will remove the completion state (and left-over logic that relates to it) #Resolved

public override string Name
=> _name;

public override AssemblySymbol ContainingAssembly
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

public override AssemblySymbol ContainingAssembly [](start = 16, length = 49)

I think we can rely on implementation in base class. #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.

There is no implementation (just the abstract declaration).


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

: ImmutableArray<NamedTypeSymbol>.Empty;
}

internal override LexicalSortKey GetLexicalSortKey()
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

internal override LexicalSortKey GetLexicalSortKey() [](start = 16, length = 52)

I think we can rely on implementation in base class. #Closed

internal override NamespaceExtent Extent
=> new NamespaceExtent(_module);

internal override ImmutableArray<Symbol> GetMembersUnordered()
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

internal override ImmutableArray GetMembersUnordered() [](start = 16, length = 62)

I think we can rely on implementation in base class. #Closed

return _nameToTypeMembersMap;
}

internal override ImmutableArray<NamedTypeSymbol> GetTypeMembersUnordered()
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

internal override ImmutableArray GetTypeMembersUnordered() [](start = 16, length = 75)

I think we can rely on implementation in base class. #Closed

@@ -55,6 +55,9 @@ internal enum CompletionPart

NamedTypeSymbolAll = NamedTypeSymbolWithLocationAll | MembersCompleted,

// For injected symbols
InjectedSymbolAll = Attributes,
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

InjectedSymbolAll = Attributes, [](start = 8, length = 31)

Is this still used? Consider reverting the file otherwise. #Closed

_diagnostics = diagnostics;
}

public override ImmutableArray<Location> Locations
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

public override ImmutableArray Locations [](start = 7, length = 51)

Can we rely on implementation from base? #Closed

#else
return result;
#endif
return GetMembers().ConditionallyDeOrder();
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

return GetMembers().ConditionallyDeOrder(); [](start = 12, length = 43)

Is base implementation different? #Closed

return ImmutableArray<CSharpAttributeData>.Empty;
}

Binder.ReportUseSiteDiagnostics(ctor, _diagnostics, Location.None);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

Binder.ReportUseSiteDiagnostics(ctor, _diagnostics, Location.None); [](start = 12, length = 67)

Doesn't Binder.GetWellKnownTypeMember take care of this? #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 didn't manage to hit this with a test, but I fixed this anyways.
The few ways I know to generate use-site diagnostics didn't work:

  • Obsolete is not reported for well-known types
  • I couldn't create a missing type because we're dealing with corlib

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

{
if (_lazyCustomAttributes.IsDefault)
{
ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, MakeAttributes());
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, MakeAttributes()); [](start = 16, length = 88)

It looks like there is a chance of duplicating diagnostics when more than one thread is executing this line at the same time. #Closed

internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
var containingType = (InjectedNonNullTypesAttributeSymbol)ContainingType;
GenerateMethodBodyCore(compilationState, containingType._diagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

GenerateMethodBodyCore(compilationState, containingType._diagnostics); [](start = 16, length = 70)

Let's assume this call produces diagnostics. Every time we try to emit, will we add it again? So, each subsequent attempt will have more and more duplicates? #Closed


void addInjectedAttribute(WellKnownType wellKnownType, bool canUseFromSource)
{
NamedTypeSymbol attribute = Compilation.GetWellKnownType(wellKnownType);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

Compilation.GetWellKnownType [](start = 44, length = 28)

Can this return a symbol from an added 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.

Added a test (crash in emit). Thanks

Filed a follow-up issue for this and also duplicate diagnostic issues: #29732


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

@@ -113,6 +178,8 @@ public static void Main()
}";

CreateCompilation(code, references: new[] { reference }, assemblyName: "Source").VerifyDiagnostics(
// error CS0101: The namespace 'Microsoft.CodeAnalysis' already contains a definition for 'EmbeddedAttribute'
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

// error CS0101: The namespace 'Microsoft.CodeAnalysis' already contains a definition for 'EmbeddedAttribute' [](start = 16, length = 109)

The breaking change description says:
"

  1. Previously, we allowed to refer to a Microsoft.CodeAnalysis.EmbeddedAttribute type declared in an added module.
    In Visual Studio 2019, this produces a collision with the injected declaration of that type.
    "

However, it doesn't look like this scenario refers to the type, yet it is still an error. Consider adjusting description of the breaking change. It isn't really about referring, it is about having this type declared in an added module. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Sep 7, 2018

Done with review pass (iteration 19). Let's discuss offline what outstanding issues can be followed up separately. #Closed


// NonNullTypesAttribute from referenced assembly is used via extern alias
var comp = CreateCompilation(code, references: new[] { reference.ToMetadataReference(aliases: ImmutableArray.Create("Reference")) });
comp.VerifyDiagnostics();
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

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

VerifyDiagnostics [](start = 17, length = 17)

Let's also check that .NonNullTypes is true on the SourceModule #Closed

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

@jcouv
Copy link
Member Author

jcouv commented Sep 10, 2018

Awesome. Thanks a lot for the review!

@jcouv jcouv merged commit 3e64c13 into dotnet:features/NullableReferenceTypes Sep 10, 2018
@jcouv jcouv deleted the inject-nnt branch September 10, 2018 19:52
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.

3 participants