Skip to content

Handle [UnscopedRef] scoped in source and metadata#63134

Merged
cston merged 6 commits intodotnet:mainfrom
jcouv:explicitly-scoped
Aug 22, 2022
Merged

Handle [UnscopedRef] scoped in source and metadata#63134
cston merged 6 commits intodotnet:mainfrom
jcouv:explicitly-scoped

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 self-assigned this Aug 2, 2022
@ghost ghost added the Area-Compilers label Aug 2, 2022
@jcouv jcouv force-pushed the explicitly-scoped branch from 7a01abb to 5ed9cde Compare August 5, 2022 16:13
@jcouv jcouv force-pushed the explicitly-scoped branch 2 times, most recently from 15071ad to 66fbae6 Compare August 5, 2022 16:31
@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
Contributor

@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
Contributor

@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
Contributor

@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
Contributor

@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
get
{
var scope = DeclaredScope;
var scope = (RefKind == RefKind.Out && DeclaredScope == DeclarationScope.Unscoped) ? DeclarationScope.RefScoped : DeclaredScope;
Copy link
Member

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
Contributor

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
Contributor

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

Development

Successfully merging this pull request may close these issues.

Mark parameter from metadata with ScopedRefAttribute and UnscopedRefAttribute as invalid Report error for [UnscopedRef] scoped parameter

5 participants