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

Handle [UnscopedRef] scoped in source and metadata #63134

Merged
merged 6 commits into from
Aug 22, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 2, 2022

Fixes #63070 (metadata scenario)
Fixes #63057 (source scenario)

Relates to test plan #59194

@jcouv jcouv force-pushed the explicitly-scoped branch from 66fbae6 to 939ea72 Compare August 5, 2022 16:42
@jcouv jcouv marked this pull request as ready for review August 5, 2022 16:42
@jcouv jcouv requested a review from a team as a code owner August 5, 2022 16:42
@jcouv jcouv requested a review from cston August 5, 2022 16:42
}

[Fact]
public void UnscopedScoped()
Copy link
Member

@cston cston Aug 5, 2022

Choose a reason for hiding this comment

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

UnscopedScoped

This test looks like a duplicate of the previous test. If so, let's remove it. #Resolved

var comp = CreateCompilation(new[] { sourceA, UnscopedRefAttributeDefinition }, runtimeFeature: RuntimeFlag.ByRefFields);
comp.VerifyEmitDiagnostics();
var verifier = CompileAndVerify(comp, verify: Verification.Skipped);
var dump = verifier.Dump();
Copy link
Member

@cston cston Aug 5, 2022

Choose a reason for hiding this comment

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

var dump = verifier.Dump();

Is this needed? #Resolved

{
throw null;
}
}";
Copy link
Member

@cston cston Aug 5, 2022

Choose a reason for hiding this comment

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

}

Consider including F4([UnscopedRef] scoped R<T> r) as well. #Resolved

}
public class A<T>
{
public ref T F4A([UnscopedRef] scoped ref R<T> r4) // 1
Copy link
Member

@cston cston Aug 5, 2022

Choose a reason for hiding this comment

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

F4A

Minor: Consider renaming these F1, F2, F3, (or something similarly consistent). #Resolved

@RikkiGibson RikkiGibson self-assigned this Aug 9, 2022
@@ -202,7 +202,8 @@ internal sealed override DeclarationScope EffectiveScope
{
get
{
var scope = DeclaredScope;
var scope = (RefKind == RefKind.Out && DeclaredScope == DeclarationScope.Unscoped) ? DeclarationScope.RefScoped : DeclaredScope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it change behavior to move this logic from ParameterHelpers, etc. into this accessor? Or is it just a refactoring?

Copy link
Member

Choose a reason for hiding this comment

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

The behavior change is described in ParameterSymbol. Basically, DeclaredScope is now just the declared scope, without the implicit scope for various cases. EffectiveScope is still the combined scope.

@cston cston requested a review from a team August 16, 2022 21:39
@RikkiGibson RikkiGibson self-requested a review August 18, 2022 20:04
@cston
Copy link
Member

cston commented Aug 22, 2022

dotnet/roslyn-compiler, please review.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 5), with one test nit.

@cston cston merged commit 2552d53 into dotnet:main Aug 22, 2022
@ghost ghost added this to the Next milestone Aug 22, 2022
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
@jcouv jcouv deleted the explicitly-scoped branch September 12, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants