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

Generate readonlyattribute if it does not exist #18715

Merged
merged 23 commits into from
May 18, 2017
Merged
Show file tree
Hide file tree
Changes from 17 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
7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1116,7 +1116,12 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio
? ((AliasSymbol)symbol).GetAliasTarget(basesBeingResolved)
: symbol;

if (WrongArity(symbol, arity, diagnose, options, out diagInfo))
// Check for symbols marked with 'Microsoft.CodeAnalysis.Embedded' attribute
if (!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) && unwrappedSymbol.IsHiddenByCodeAnalysisEmbeddedAttribute())
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 26, 2017

Choose a reason for hiding this comment

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

!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) [](start = 16, length = 71)

Do we have tests covering significance of this condition? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Curious - why "external" part makes a difference? I thought the embedded symbols are not bindable regardless of where defined.


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

Copy link
Contributor Author

@OmarTawfik OmarTawfik Apr 28, 2017

Choose a reason for hiding this comment

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

We have tests that cover one condition. Adding more tests now.
edit: @VSadov I'm correcting this now. Lookup would fail if on embedded attributes defined somewhere else. If you define your own embedded attribute we would ignore it. UNLESS the compiler needs to generate one, then we will produce an error.


In reply to: 113549567 [](ancestors = 113549567,113533730)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you point me to a test that demonstrates significance of the first condition?


In reply to: 114024707 [](ancestors = 114024707,113549567,113533730)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check AttributeTests_Embedded.cs:
ReferencingEmbeddedAttributesFromADifferentAssemblyFails_Internal
ReferencingEmbeddedAttributesFromADifferentAssemblyFails_Public
and
EmbeddedAttributeInSourceShouldTriggerAnErrorIfCompilerNeedsToGenerateOne


In reply to: 115050349 [](ancestors = 115050349,114024707,113549567,113533730)

{
return LookupResult.Empty();
}
else if (WrongArity(symbol, arity, diagnose, options, out diagInfo))
{
return LookupResult.WrongArity(symbol, diagInfo);
}
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/LookupResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ internal static SingleLookupResult WrongArity(Symbol symbol, DiagnosticInfo erro
return new SingleLookupResult(LookupResultKind.WrongArity, symbol, error);
}

internal static SingleLookupResult Empty()
{
return new SingleLookupResult(LookupResultKind.Empty, null, null);
}

internal static SingleLookupResult NotReferencable(Symbol symbol, DiagnosticInfo error)
{
return new SingleLookupResult(LookupResultKind.NotReferencable, symbol, error);
Expand Down
21 changes: 19 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,17 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
cacheKey.ParameterTypes,
cacheKey.ParameterRefKinds,
refKind,
returnType);
returnType,
diagnostics);
lambdaBodyBinder = new ExecutableCodeBinder(_unboundLambda.Syntax, lambdaSymbol, ParameterBinder(lambdaSymbol, binder));

if (lambdaSymbol.RefKind == CodeAnalysis.RefKind.RefReadOnly)
{
binder.Compilation.EnsureIsReadOnlyAttributeExists(diagnostics, lambdaSymbol.DiagnosticLocation, modifyCompilationForRefReadOnly: false);
}

ParameterHelpers.EnsureIsReadOnlyAttributeExists(lambdaSymbol.Parameters, diagnostics, modifyCompilationForRefReadOnly: false);
Copy link
Member

@cston cston May 11, 2017

Choose a reason for hiding this comment

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

Aren't we calling EnsureIsReadOnlyAttributeExists for the lambda and parameters in LocalRewriter? #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

If these checks are necessary, is there a reason to have the checks in the caller rather than in LambdaSymbol..ctor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For lambdas and local functions, we do that two times:

  1. Here, where we want to generate diagnostics for each symbol, but not actually modify the compilation (because they might not belong to the current one). That has to be done here rather than the constructor because of circular lookups. The compiler can crash with a SOE trying to bing all types.
  2. In the local rewriter, where we will modify the current module builder, and note the need for the attribute, but we will not generate errors, because they're are already generated here.

Copy link
Member

Choose a reason for hiding this comment

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

Are there diagnostics generated here that we cannot generate in the local rewriter? If not, can we drop the checks from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Diagnostics generated here are needed even though we are not emitting anything. We would still want to display them to the API users.


block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);

((ExecutableCodeBinder)lambdaBodyBinder).ValidateIteratorMethods(diagnostics);
Expand Down Expand Up @@ -525,7 +534,15 @@ private void ValidateUnsafeParameters(DiagnosticBag diagnostics, ImmutableArray<
private BoundLambda ReallyInferReturnType(NamedTypeSymbol delegateType, ImmutableArray<TypeSymbol> parameterTypes, ImmutableArray<RefKind> parameterRefKinds)
{
var diagnostics = DiagnosticBag.GetInstance();
var lambdaSymbol = new LambdaSymbol(binder.Compilation, binder.ContainingMemberOrLambda, _unboundLambda, parameterTypes, parameterRefKinds, refKind: Microsoft.CodeAnalysis.RefKind.None, returnType: null);
var lambdaSymbol = new LambdaSymbol(
binder.Compilation,
binder.ContainingMemberOrLambda,
_unboundLambda,
parameterTypes,
parameterRefKinds,
refKind: CodeAnalysis.RefKind.None,
returnType: null,
diagnostics: diagnostics);
Binder lambdaBodyBinder = new ExecutableCodeBinder(_unboundLambda.Syntax, lambdaSymbol, ParameterBinder(lambdaSymbol, binder));
var block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpCodeAnalysis.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@
<Compile Include="Symbols\DiscardSymbol.cs" />
<Compile Include="Symbols\Metadata\PE\TupleTypeDecoder.cs" />
<Compile Include="Symbols\Source\GlobalExpressionVariable.cs" />
<Compile Include="Symbols\Synthesized\SynthesizedEmbeddedAttributeSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleEventSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleFieldSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleMethodSymbol.cs" />
Expand Down Expand Up @@ -871,4 +872,4 @@
</ItemGroup>
<Import Project="..\CSharpAnalyzerDriver\CSharpAnalyzerDriver.projitems" Label="Shared" />
<Import Project="..\..\..\..\build\Targets\Imports.targets" />
</Project>
</Project>
11 changes: 10 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5069,6 +5069,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>Compiler version: '{0}'. Language version: {1}.</value>
</data>
<data name="ERR_ExplicitReadOnlyAttr" xml:space="preserve">
<value>Do not use 'System.Runtime.CompilerServices.ReadOnlyAttribute'. This is reserved for compiler usage.</value>
<value>Do not use 'System.Runtime.CompilerServices.IsReadOnlyAttribute'. This is reserved for compiler usage.</value>
</data>
<data name="ERR_TypeReserved" xml:space="preserve">
<value>The type name '{0}' is reserved to be used by the compiler.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -1837,6 +1837,10 @@ internal DiagnosticBag DeclarationDiagnostics
private IEnumerable<Diagnostic> FreezeDeclarationDiagnostics()
{
_declarationDiagnosticsFrozen = true;

// Also freeze that flag, as symbols bound after getting the declaration diagnostics shouldn't need to modify it
_needsGeneratedIsReadOnlyAttribute_IsFrozen = true;

var result = _lazyDeclarationDiagnostics?.AsEnumerable() ?? Enumerable.Empty<Diagnostic>();
return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
int statementIndex = 0;

// explicit base constructor call
BoundExpression call = MethodCompiler.GenerateObjectConstructorInitializer(this, diagnostics);
Debug.Assert(ContainingType.BaseTypeNoUseSiteDiagnostics.SpecialType == SpecialType.System_Object);
BoundExpression call = MethodCompiler.GenerateBaseParameterlessConstructorInitializer(this, diagnostics);
if (call == null)
{
// This may happen if Object..ctor is not found or is unaccessible
Expand Down
37 changes: 21 additions & 16 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public static void CompileMethodBodies(
// compile additional and anonymous types if any
if (moduleBeingBuiltOpt != null)
{
var additionalTypes = moduleBeingBuiltOpt.GetAdditionalTopLevelTypes();
var additionalTypes = moduleBeingBuiltOpt.GetAdditionalTopLevelTypes(diagnostics);
if (!additionalTypes.IsEmpty)
{
methodCompiler.CompileSynthesizedMethods(additionalTypes, diagnostics);
Expand Down Expand Up @@ -1638,7 +1638,7 @@ internal static BoundExpression BindConstructorInitializer(
{
if (baseType.SpecialType == SpecialType.System_Object)
{
return GenerateObjectConstructorInitializer(constructor, diagnostics);
return GenerateBaseParameterlessConstructorInitializer(constructor, diagnostics);
}
else if (baseType.IsErrorType() || baseType.IsStatic)
{
Expand Down Expand Up @@ -1739,42 +1739,47 @@ private static SyntaxToken GetImplicitConstructorBodyToken(CSharpSyntaxNode cont
}
}

internal static BoundCall GenerateObjectConstructorInitializer(MethodSymbol constructor, DiagnosticBag diagnostics)
internal static BoundCall GenerateBaseParameterlessConstructorInitializer(MethodSymbol constructor, DiagnosticBag diagnostics)
{
NamedTypeSymbol objectType = constructor.ContainingType.BaseTypeNoUseSiteDiagnostics;
Debug.Assert(objectType.SpecialType == SpecialType.System_Object);
MethodSymbol objectConstructor = null;
NamedTypeSymbol baseType = constructor.ContainingType.BaseTypeNoUseSiteDiagnostics;
MethodSymbol baseConstructor = null;
LookupResultKind resultKind = LookupResultKind.Viable;
Location diagnosticsLocation = constructor.Locations.IsEmpty ? NoLocation.Singleton : constructor.Locations[0];

foreach (MethodSymbol objectCtor in objectType.InstanceConstructors)
foreach (MethodSymbol ctor in baseType.InstanceConstructors)
{
if (objectCtor.ParameterCount == 0)
if (ctor.ParameterCount == 0)
{
objectConstructor = objectCtor;
baseConstructor = ctor;
break;
}
}

// UNDONE: If this happens then something is deeply wrong. Should we give a better error?
if ((object)objectConstructor == null)
if ((object)baseConstructor == null)
{
diagnostics.Add(ErrorCode.ERR_BadCtorArgCount, diagnosticsLocation, baseType, /*desired param count*/ 0);
return null;
}

if (Binder.ReportUseSiteDiagnostics(baseConstructor, diagnostics, diagnosticsLocation))
{
diagnostics.Add(ErrorCode.ERR_BadCtorArgCount, constructor.Locations[0], objectType, /*desired param count*/ 0);
return null;
}

// UNDONE: If this happens then something is deeply wrong. Should we give a better error?
bool hasErrors = false;
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
if (!AccessCheck.IsSymbolAccessible(objectConstructor, constructor.ContainingType, ref useSiteDiagnostics))
if (!AccessCheck.IsSymbolAccessible(baseConstructor, constructor.ContainingType, ref useSiteDiagnostics))
{
diagnostics.Add(ErrorCode.ERR_BadAccess, constructor.Locations[0], objectConstructor);
diagnostics.Add(ErrorCode.ERR_BadAccess, diagnosticsLocation, baseConstructor);
resultKind = LookupResultKind.Inaccessible;
hasErrors = true;
}

if (!useSiteDiagnostics.IsNullOrEmpty())
{
diagnostics.Add(constructor.Locations.IsEmpty ? NoLocation.Singleton : constructor.Locations[0], useSiteDiagnostics);
diagnostics.Add(diagnosticsLocation, useSiteDiagnostics);
}

CSharpSyntaxNode syntax = constructor.GetNonNullSyntaxNode();
Expand All @@ -1783,7 +1788,7 @@ internal static BoundCall GenerateObjectConstructorInitializer(MethodSymbol cons
return new BoundCall(
syntax: syntax,
receiverOpt: receiver,
method: objectConstructor,
method: baseConstructor,
arguments: ImmutableArray<BoundExpression>.Empty,
argumentNamesOpt: ImmutableArray<string>.Empty,
argumentRefKindsOpt: ImmutableArray<RefKind>.Empty,
Expand All @@ -1792,7 +1797,7 @@ internal static BoundCall GenerateObjectConstructorInitializer(MethodSymbol cons
invokedAsExtensionMethod: false,
argsToParamsOpt: ImmutableArray<int>.Empty,
resultKind: resultKind,
type: objectType,
type: baseType,
hasErrors: hasErrors)
{ WasCompilerGenerated = true };
}
Expand Down
12 changes: 2 additions & 10 deletions src/Compilers/CSharp/Portable/Emitter/Model/MethodSymbolAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -570,23 +570,15 @@ bool Cci.IMethodDefinition.RequiresSecurityObject
}
}

IEnumerable<Cci.ICustomAttribute> Cci.IMethodDefinition.ReturnValueAttributes
{
get
{
return GetReturnValueCustomAttributesToEmit();
}
}

private IEnumerable<CSharpAttributeData> GetReturnValueCustomAttributesToEmit()
IEnumerable<Cci.ICustomAttribute> Cci.IMethodDefinition.GetReturnValueAttributes(EmitContext context)
{
CheckDefinitionInvariant();

ImmutableArray<CSharpAttributeData> userDefined;
ArrayBuilder<SynthesizedAttributeData> synthesized = null;

userDefined = this.GetReturnTypeAttributes();
this.AddSynthesizedReturnTypeAttributes(ref synthesized);
this.AddSynthesizedReturnTypeAttributes((PEModuleBuilder)context.Module, ref synthesized);

// Note that callers of this method (CCI and ReflectionEmitter) have to enumerate
// all items of the returned iterator, otherwise the synthesized ArrayBuilder may leak.
Expand Down
92 changes: 90 additions & 2 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Reflection;
using System.Threading;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.Emit;
using Roslyn.Utilities;
Expand All @@ -17,6 +20,10 @@ internal abstract class PEAssemblyBuilderBase : PEModuleBuilder, Cci.IAssemblyRe
private readonly ImmutableArray<NamedTypeSymbol> _additionalTypes;
private ImmutableArray<Cci.IFileReference> _lazyFiles;

private SynthesizedEmbeddedAttributeSymbol _lazyEmbeddedAttribute;
private SynthesizedEmbeddedAttributeSymbol _lazyIsReadOnlyAttribute;
private ImmutableArray<Diagnostic> _lazyEmbeddedAttributesDiagnostics;

/// <summary>
/// The behavior of the C# command-line compiler is as follows:
/// 1) If the /out switch is specified, then the explicit assembly name is used.
Expand Down Expand Up @@ -56,9 +63,24 @@ public PEAssemblyBuilderBase(

public override ISourceAssemblySymbolInternal SourceAssemblyOpt => _sourceAssembly;

internal override ImmutableArray<NamedTypeSymbol> GetAdditionalTopLevelTypes()
internal override ImmutableArray<NamedTypeSymbol> GetAdditionalTopLevelTypes(DiagnosticBag diagnostics)
{
return _additionalTypes;
var builder = ArrayBuilder<NamedTypeSymbol>.GetInstance();
builder.AddRange(_additionalTypes);

CreateEmbeddedAttributesIfNeeded();
diagnostics.AddRange(_lazyEmbeddedAttributesDiagnostics);
Copy link
Contributor

Choose a reason for hiding this comment

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

diagnostics.AddRange(_lazyEmbeddedAttributesDiagnostics); [](start = 12, length = 57)

This is likely to cause duplication of errors. We should avoid that, we specifically avoid reporting duplicate errors in PEModuleBuilder, see _reportedErrorTypesMap for example. Please revert to the previous approach.


if ((object)_lazyEmbeddedAttribute != null)
{
builder.Add(_lazyEmbeddedAttribute);
}
if ((object)_lazyIsReadOnlyAttribute != null)
{
builder.Add(_lazyIsReadOnlyAttribute);
}

return builder.ToImmutableAndFree();
}

public sealed override IEnumerable<Cci.IFileReference> GetFiles(EmitContext context)
Expand Down Expand Up @@ -131,6 +153,72 @@ protected override void AddEmbeddedResourcesFromAddedModules(ArrayBuilder<Cci.Ma
public override string Name => _metadataName;
public AssemblyIdentity Identity => _sourceAssembly.Identity;
public Version AssemblyVersionPattern => _sourceAssembly.AssemblyVersionPattern;

internal override SynthesizedAttributeData SynthesizeEmbeddedAttribute()
{
if ((object)_lazyEmbeddedAttribute != null)
{
return new SynthesizedAttributeData(
_lazyEmbeddedAttribute.Constructor,
ImmutableArray<TypedConstant>.Empty,
ImmutableArray<KeyValuePair<string, TypedConstant>>.Empty);
}

return base.SynthesizeEmbeddedAttribute();
}

protected override SynthesizedAttributeData TrySynthesizeIsReadOnlyAttribute()
{
if ((object)_lazyIsReadOnlyAttribute != null)
{
return new SynthesizedAttributeData(
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2017

Choose a reason for hiding this comment

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

return new SynthesizedAttributeData( [](start = 16, length = 36)

I would expect us to return null if symbol is not from our source module. #Closed

_lazyIsReadOnlyAttribute.Constructor,
ImmutableArray<TypedConstant>.Empty,
ImmutableArray<KeyValuePair<string, TypedConstant>>.Empty);
}

return base.TrySynthesizeIsReadOnlyAttribute();
}

private void CreateEmbeddedAttributesIfNeeded()
{
if (_lazyEmbeddedAttributesDiagnostics.IsDefault)
{
var diagnostics = DiagnosticBag.GetInstance();

if (this.NeedsGeneratedIsReadOnlyAttribute)
{
CreateEmbeddedAttributeIfNeeded(
ref _lazyEmbeddedAttribute,
diagnostics,
AttributeDescription.CodeAnalysisEmbeddedAttribute);

CreateEmbeddedAttributeIfNeeded(
ref _lazyIsReadOnlyAttribute,
diagnostics,
AttributeDescription.IsReadOnlyAttribute);
}

_lazyEmbeddedAttributesDiagnostics = diagnostics.ToReadOnlyAndFree();
}
}

private void CreateEmbeddedAttributeIfNeeded(ref SynthesizedEmbeddedAttributeSymbol symbol, DiagnosticBag diagnostics, AttributeDescription description)
{
if ((object)symbol == null)
Copy link
Member

Choose a reason for hiding this comment

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

Is there still a case where symbol can be null? If not, remove the null check and perhaps return symbol rather than using a ref parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is possibly going to be rewritten in Vlad's next PR, as there are more attributes to be generated. His feature will possibly need to generate some attributes that are generated in this PR, so the check will be necessary.
I see the future of embedded attributes going in one of two directions:

  1. keeping the current design, having each feature calling CreateEmbeddedAttributeIfNeeded on the attributes it needs (possibly overlapping), and thus the current design protects against that.
  2. If the features that need embedded attributes grow in numbers, there might be a need for an independent type manager to hold/manage them.

Copy link
Member

Choose a reason for hiding this comment

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

For now we are adding only IsByRefLike, so there is no need for #2 just yet.
The current code seems ok to me for now.

{
var attributeMetadataName = MetadataTypeName.FromFullName(description.FullName);
var userDefinedAttribute = _sourceAssembly.SourceModule.LookupTopLevelMetadataType(ref attributeMetadataName);
Debug.Assert((object)userDefinedAttribute.ContainingModule == _sourceAssembly.SourceModule);

if (!(userDefinedAttribute is MissingMetadataTypeSymbol))
{
diagnostics.Add(ErrorCode.ERR_TypeReserved, userDefinedAttribute.Locations[0], description.FullName);
}

symbol = new SynthesizedEmbeddedAttributeSymbol(description, _sourceAssembly.DeclaringCompilation, diagnostics);
}
}
}

internal sealed class PEAssemblyBuilder : PEAssemblyBuilderBase
Expand Down
Loading