Skip to content

Allow adding scoped or removing [UnscopedRef] for override, interface implementation, or delegate conversion#63656

Merged
cston merged 10 commits intodotnet:mainfrom
cston:62340
Sep 15, 2022
Merged

Allow adding scoped or removing [UnscopedRef] for override, interface implementation, or delegate conversion#63656
cston merged 10 commits intodotnet:mainfrom
cston:62340

Conversation

@cston
Copy link
Contributor

@cston cston commented Aug 29, 2022

@cston cston marked this pull request as ready for review September 1, 2022 06:22
@cston cston requested a review from a team as a code owner September 1, 2022 06:22
@cston
Copy link
Contributor Author

cston commented Sep 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 2 pipeline(s).

@cston
Copy link
Contributor Author

cston commented Sep 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s).

@cston
Copy link
Contributor Author

cston commented Sep 1, 2022

This also fixes an issue introduced in #63461 that prevents moving to the latest compiler.

Specifically, the following should compile without errors:

Compile with -langversion:10

public delegate void D1<T>(out T t1);

Compile with -langversion:11

class Program
{
    static void F1(out int i1) { i1 = 0; }
    static void Main()
    {
        // error CS8986: The 'scoped' modifier of parameter 'i1' doesn't match target 'D1<int>'
        D1<int> d1 = F1;
    }
}

@cston
Copy link
Contributor Author

cston commented Sep 1, 2022

@dotnet/roslyn-compiler, please review.

@RikkiGibson RikkiGibson self-assigned this Sep 1, 2022
{
D1 dA = M2;
D2 d2 = M1;
D1 d1 = M2;
Copy link
Member

Choose a reason for hiding this comment

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

Can we please make the numbers line up with DX and MX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was numbering Dx and Mx with the same x for the matching signature. That seemed the most useful to me.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose someone will be confused, no matter what you name these.

D1 dA = M2;
D2 d2 = M1;
D1 d1 = M2;
D2 d2 = M1; // 1
Copy link
Member

Choose a reason for hiding this comment

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

it would be helpful to add these trailing comments on all the tests where the diagnostics changed. Also, it looks like the comments are incomplete in this test. I would expect comments for declarations of 'M3' and 'M5'.

Copy link
Contributor Author

@cston cston Sep 1, 2022

Choose a reason for hiding this comment

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

I would expect comments for declarations of 'M3' and 'M5'.

I thought it would be clearer to have comments for the diagnostics related to scoped mismatch only.

it would be helpful to add these trailing comments on all the tests where the diagnostics changed.

It looks like there are comments on most if not all of the ERR_ScopedMismatch* errors. Let me know if I've missed some, thanks.

@cston cston added the Blocked label Sep 2, 2022
@cston cston marked this pull request as draft September 13, 2022 19:14
@cston cston removed the Blocked label Sep 13, 2022
@cston cston marked this pull request as ready for review September 13, 2022 22:20
@cston
Copy link
Contributor Author

cston commented Sep 14, 2022

@333fred, @RikkiGibson, please review since this PR has been updated to: report diagnostics for unsafe differences only; and report warnings rather than errors for modules compiled with -langversion:10 or earlier.

@RikkiGibson RikkiGibson self-requested a review September 14, 2022 19:25
}
else if (parameters.Any(p => (p.RefKind is RefKind.Ref or RefKind.Out) && p.Type.IsRefLikeType))
{
nRefParametersRequired = 2; // including the parameter found above
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure if this will need to be modified to account for the upcoming ReturnOnlyScope change. Perhaps at that time it would be worth changing this to try an approach where we simulate the base calling into the derived and see if escape rules are satisfied.

class Base
{
    public virtual Span<byte> M(scoped ref byte input)
    {
        // if we could call Derived.M like this, would we satisfy escape rules?
        return Derived.M(ref input);
    }
}
class Derived : Base
{
    public override Span<byte> M(ref byte input)
    {
    }
}

@RikkiGibson RikkiGibson requested a review from 333fred September 14, 2022 22:17
@RikkiGibson
Copy link
Member

@dotnet/roslyn-compiler it would be great to get this in soon, as the many diagnostic changes are prone to cause conflicts with other PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: remember that we have raw strings, and you can do $$""" :).

@cston cston merged commit fb2b622 into dotnet:main Sep 15, 2022
@cston cston deleted the 62340 branch September 15, 2022 17:50
@ghost ghost added this to the Next milestone Sep 15, 2022
@cston cston modified the milestones: Next, 17.4 Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants