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

Rename LifetimeAnnotationAttribute to ScopedRefAttribute and simplify #63009

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jul 28, 2022

Fixes #62838

@jcouv jcouv added this to the 17.4 P1 milestone Jul 28, 2022
@jcouv jcouv self-assigned this Jul 28, 2022
@jcouv jcouv force-pushed the scoped-attribute branch from 7834724 to 35ff572 Compare July 28, 2022 06:09
@jcouv jcouv marked this pull request as ready for review July 28, 2022 16:16
@jcouv jcouv requested a review from a team as a code owner July 28, 2022 16:16
@@ -334,7 +334,7 @@ static AttributeDescription()
private static readonly byte[][] s_signaturesOfNullableAttribute = { s_signature_HasThis_Void_Byte, s_signature_HasThis_Void_SzArray_Byte };
private static readonly byte[][] s_signaturesOfNullableContextAttribute = { s_signature_HasThis_Void_Byte };
private static readonly byte[][] s_signaturesOfNativeIntegerAttribute = { s_signature_HasThis_Void, s_signature_HasThis_Void_SzArray_Boolean };
private static readonly byte[][] s_signaturesOfLifetimeAnnotationAttribute = { s_signature_HasThis_Void_Boolean_Boolean };
private static readonly byte[][] s_signaturesOfScopedRefAttribute = { s_signature_HasThis_Void };
Copy link
Member

@cston cston Jul 28, 2022

Choose a reason for hiding this comment

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

s_signaturesOfScopedRefAttribute

Consider removing and replacing use with s_signatures_HasThis_Void_Only. #Closed

}
else
{
scope = scopeOpt.GetValueOrDefault();
if (typeWithAnnotations.Type.IsRefLikeType)
Copy link
Member

@cston cston Jul 28, 2022

Choose a reason for hiding this comment

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

Minor: Consider collapsing to else if. #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 was intentional, but I don't feel very strongly about it ;-)

@cston
Copy link
Member

cston commented Jul 28, 2022

using Microsoft.CodeAnalysis.PooledObjects;

Consider removing if not needed.


In reply to: 1198403939


Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEmbeddedLifetimeAnnotationAttributeSymbol.cs:9 in 35ff572. [](commit_id = 35ff572, deletion_comment = False)

});
}

[Fact]
public void ExplicitAttribute_MissingConstructor()
Copy link
Member

@cston cston Jul 28, 2022

Choose a reason for hiding this comment

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

ExplicitAttribute_MissingConstructor

We should keep a missing constructor test but change the constructor to one with parameters. #Closed

var parameter = method.Parameters[0];
Assert.Equal(DeclarationScope.ValueScoped, parameter.Scope);

method = comp.GetMember<MethodSymbol>("A.F3");
Copy link
Member

@cston cston Jul 28, 2022

Choose a reason for hiding this comment

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

comp.GetMember("A.F3");

Include "A.F2" as well? #Closed

@cston
Copy link
Member

cston commented Jul 28, 2022

Consider renaming files as well:

  • SynthesizedEmbeddedLifetimeAnnotationAttributeSymbol.cs
  • AttributeTests_LifetimeAnnotation.cs
  • LifetimeAnnotationAttributesVisitor.cs

In reply to: 1198411628

@jcouv
Copy link
Member Author

jcouv commented Jul 28, 2022

I intentionally didn't rename them yet to minimize risk of conflicts. We can do that later


In reply to: 1198411628

Diagnostic(ErrorCode.WRN_UnreferencedEvent, "E").WithArguments("Program.E").WithLocation(7, 58));
// (3,10): error CS0592: Attribute 'ScopedRef' is not valid on this declaration type. It is only valid on 'parameter' declarations.
// [module: ScopedRef]
Diagnostic(ErrorCode.ERR_AttributeOnBadSymbolType, "ScopedRef").WithArguments("ScopedRef", "parameter").WithLocation(3, 10),
Copy link
Member

@cston cston Jul 28, 2022

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.ERR_AttributeOnBadSymbolType, "ScopedRef").WithArguments("ScopedRef", "parameter").WithLocation(3, 10),

Ideally, this test should use a definition of ScopedRefAttributes with AttributeTargets.All rather than AttributeTargets.Parameter, so we can ensure we're reporting errors for the use in source (when #62124 is fixed) rather than for incorrect target.

It's not blocking though. #Pending

SynthesizedParameterSymbol.Create(m, TypeWithAnnotations.Create(boolType), 1, RefKind.None)),
(f, s, p) => GenerateConstructorBody(f, s, p)));
getParameters: m => ImmutableArray<ParameterSymbol>.Empty,
getConstructorBody: (f, s, p) => { }));
Copy link
Contributor

@chsienki chsienki Jul 28, 2022

Choose a reason for hiding this comment

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

Nit: discards? #Pending

@jcouv jcouv enabled auto-merge (squash) July 28, 2022 21:21
@jcouv jcouv merged commit 7915cb7 into dotnet:release/dev17.4 Jul 28, 2022
@jcouv jcouv deleted the scoped-attribute branch July 28, 2022 23:26
cston added a commit to cston/roslyn that referenced this pull request Aug 3, 2022
JoeRobich added a commit that referenced this pull request Aug 4, 2022
Revert "Rename `LifetimeAnnotationAttribute` to `ScopedRefAttribute` and simplify (#63009)"
dibarbet added a commit that referenced this pull request Aug 4, 2022
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