Support `scoped` keyword in declaration expressions. #64380

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.

Sign up for GitHub

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

Jump to bottom

Merged

AlekseyTs merged 6 commits into dotnet:main from AlekseyTs:Issue62039_06

Oct 13, 2022

Contributor

AlekseyTs commented Sep 29, 2022

Related to #62039.
https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md#syntax

Also fixes #64259.


          Support scoped keyword in declaration expressions.

4df2321

Related to dotnet#62039.
https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md#syntax

Also fixes dotnet#64259.

AlekseyTs added Area-Compilers Feature - Ref Fields labels

AlekseyTs requested review from RikkiGibson and cston

September 29, 2022 23:04

AlekseyTs requested a review from a team as a code owner

September 29, 2022 23:04

Contributor Author

AlekseyTs commented Sep 30, 2022

@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review.

runfoapp bot mentioned this pull request

WorkCoordinatorTests.AdditionalDocumentOpenedClosedEvents is flaky #64047

Closed

Contributor Author

AlekseyTs commented Oct 3, 2022

@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review.

1 similar comment

Contributor Author

AlekseyTs commented Oct 4, 2022

@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review.

Contributor

cston commented Oct 4, 2022 •

edited by AlekseyTs

Loading

";

Consider testing:

(scoped _, var a) = r;
(scoped scoped _, var b) = r;

In reply to: 1267291775

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:11206 in 4df2321. [](commit_id = 4df2321, deletion_comment = False)

Contributor

cston commented Oct 4, 2022 •

edited by AlekseyTs

Loading

}";

Consider testing:

M1(out scoped _);
M1(out scoped scoped _);

In reply to: 1267317245

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:10647 in 4df2321. [](commit_id = 4df2321, deletion_comment = False)

Contributor

cston commented Oct 4, 2022 •

edited

Loading

            Diagnostic(ErrorCode.ERR_RefReturnParameter, "r0").WithArguments("r0").WithLocation(11, 30)

These errors look unnecessary. This might be #64448, if not already fixed in #64318.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:11540 in 4df2321. [](commit_id = 4df2321, deletion_comment = False)

Member

RikkiGibson commented Oct 4, 2022

Is this change being considered for 17.4?

Contributor Author

AlekseyTs commented Oct 4, 2022

Is this change being considered for 17.4?

Not at the moment. Do you think it should be?

Contributor

cston commented Oct 4, 2022

            Diagnostic(ErrorCode.ERR_EscapeVariable, "s1").WithArguments("s1").WithLocation(7, 15)

This looks like #64448.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:11896 in 4df2321. [](commit_id = 4df2321, deletion_comment = False)

Member

RikkiGibson commented Oct 4, 2022

Is this change being considered for 17.4?

Not at the moment. Do you think it should be?

My preference would be to do it in 17.5.

Contributor

cston commented Oct 4, 2022

";

Consider testing:

{
    int i1 = 1;
    (new S(ref i1)).M(out scoped S1);
    s0 = s1;
}
{
    int i1 = 1;
    (new S(ref i1)).M(out var S1);
    s0 = s1;
}
{
    int i1 = 1;
    (new S(ref i1)).M(out scoped var S1);
    s0 = s1;
}

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:12289 in 4df2321. [](commit_id = 4df2321, deletion_comment = False)

cston approved these changes

View reviewed changes

Contributor

cston left a comment

LGTM, with a few test suggestions.

Contributor Author

AlekseyTs commented Oct 5, 2022

@RikkiGibson, @dotnet/roslyn-compiler For the second review.

2 similar comments

Contributor Author

AlekseyTs commented Oct 6, 2022

@RikkiGibson, @dotnet/roslyn-compiler For the second review.

Contributor Author

AlekseyTs commented Oct 7, 2022

@RikkiGibson, @dotnet/roslyn-compiler For the second review.

RikkiGibson self-assigned this

Contributor Author

AlekseyTs commented Oct 10, 2022

@RikkiGibson, @dotnet/roslyn-compiler For the second review.

1 similar comment

Contributor Author

AlekseyTs commented Oct 11, 2022

@RikkiGibson, @dotnet/roslyn-compiler For the second review.

AlekseyTs added 3 commits

October 11, 2022 15:39


          Merge remote-tracking branch 'dotnet/main' into Issue62039_06

66a1623


          Follow up on merge from main

fa8ff1f


          PR feedback

59194e5

Contributor

cston commented Oct 12, 2022 •

edited

Loading

            //             (new S(ref i1)).M(out S s1);

Is the error because of out S s1? Is s1 being inferred as scoped but not scoped to the nested block?

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:12693 in 66a1623. [](commit_id = 66a1623, deletion_comment = False)

cston approved these changes

View reviewed changes

Contributor Author

AlekseyTs commented Oct 12, 2022

Is the error because of out S s1? Is s1 being inferred as scoped but not scoped to the nested block?

I am having a hard time mapping this question to a specific location in source. Could you repost it relative to the current state of the branch?

Contributor

cston commented Oct 12, 2022

I am having a hard time mapping this question to a specific location in source. Could you repost it relative to the current state of the branch?

The comment was referring to the error reported for (new S(ref i1)).M(out S s1); in LocalScope_12_OutVar.

Contributor Author

AlekseyTs commented Oct 12, 2022

I cannot comment about reasons why the errors are reported, simply didn't dig into that. The behavior is consistent with behavior when out var declaration isn't used:

        int i0 = 0;
        S s0 = new S(ref i0);
        {
            int i1 = 1;
            S s1;
            (new S(ref i1)).M(out s1);
            s0 = s1;
        }


          Merge remote-tracking branch 'dotnet/main' into Issue62039_06

2a95577

RikkiGibson approved these changes

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/SyntaxFacts.cs

    
                      {

                          parent = node.Parent as DeclarationExpressionSyntax;

                          return node == parent?.Type;

                          parent = node.ModifyingScopedOrRefTypeOrSelf().Parent as DeclarationExpressionSyntax;

Member

RikkiGibson Oct 10, 2022 •

edited by AlekseyTs

Loading

What is the difference between node.ModifyingScopedOrRefTypeOrSelf() and node.SkipScoped(out _).SkipRef(out _)? #Resolved

Contributor Author

AlekseyTs Oct 12, 2022

What is the difference between node.ModifyingScopedOrRefTypeOrSelf() and node.SkipScoped(out _).SkipRef(out _)?

ModifyingScopedOrRefTypeOrSelf goes up (to the parent), "skip" helpers go down (to the child). They are, effectively, opposite operations.

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

    
                              var discard = tree.GetRoot().DescendantNodes().OfType<DiscardDesignationSyntax>().ToArray();

                              Assert.Equal(8, discard.Length);

                              for (int i = 0; i < 6; i++)

Member

RikkiGibson Oct 12, 2022 •

edited by AlekseyTs

Loading

I didn't understand why the last 2 discard elements are not checked (since length was asserted to be 8 above). #Resolved

Contributor Author

AlekseyTs Oct 12, 2022

I didn't understand why the last 2 discard elements are not checked (since length was asserted to be 8 above).

The last two discards refer to an undeclared type, do not align with the rest, and are not really interesting for the purpose of the test.


          Follow up on merge from main

271486a

Closes dotnet#64556.

cston approved these changes

View reviewed changes

AlekseyTs merged commit f020a37 into dotnet:main

ghost added this to the Next milestone

allisonchou mentioned this pull request

[Automated] PRs inserted in VS build main-33014.230 #64748

Closed

ryzngard mentioned this pull request

Fix Color Contrast For STE Close Button #64834

Merged

RikkiGibson modified the milestones: Next, 17.5 P1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Ref Fields