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.

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

Out as return only #64318

Merged
merged 28 commits into from
Sep 29, 2022
Merged

Out as return only #64318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7522f0d
Out change
jaredpar Sep 27, 2022
39ac635
More
jaredpar Sep 27, 2022
7e64550
more
jaredpar Sep 27, 2022
e2ef7a3
Fixed out
jaredpar Sep 27, 2022
1312bbb
finishing up
jaredpar Sep 27, 2022
2e18e89
feedback
jaredpar Sep 27, 2022
fadfefd
more
jaredpar Sep 27, 2022
d90757e
Clean up
jaredpar Sep 27, 2022
eee84e7
More
jaredpar Sep 27, 2022
4cee632
more
jaredpar Sep 27, 2022
ff1f999
PR feedback
jaredpar Sep 27, 2022
9f2605b
PR feedback
jaredpar Sep 27, 2022
d9608af
PR feedback
jaredpar Sep 27, 2022
1503cfc
PR feedback
jaredpar Sep 28, 2022
f4b2ce5
Refactor the EscapeLevel anaylsis
jaredpar Sep 28, 2022
6c70c9b
PR feedback
jaredpar Sep 28, 2022
a2007a4
PR feedback
jaredpar Sep 28, 2022
e11141d
PR feedback
jaredpar Sep 28, 2022
f68618c
In person feedback
jaredpar Sep 28, 2022
7d2605b
More
jaredpar Sep 28, 2022
28dd184
RSTE of all ref is return-only
jaredpar Sep 28, 2022
dd92bf2
Apply suggestions from code review
jaredpar Sep 28, 2022
99cc78a
more tests
jaredpar Sep 28, 2022
b663cb5
PR feedback
jaredpar Sep 28, 2022
3a4577d
PR feedback
jaredpar Sep 28, 2022
6e231a4
More
jaredpar Sep 29, 2022
5536fd1
Tests
jaredpar Sep 29, 2022
e20541e
formatting
jaredpar Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
543 changes: 361 additions & 182 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13817,6 +13817,9 @@ public ref struct S1

comp = CreateCompilation(new[] { code, InterpolatedStringHandlerAttribute, InterpolatedStringHandlerArgumentAttribute }, parseOptions: TestOptions.Regular11, targetFramework: TargetFramework.NetCoreApp);
comp.VerifyDiagnostics(
// (10,100): error CS8352: Cannot use variable 'out CustomHandler' in this context because it may expose referenced variables outside of their declaration scope
// public CustomHandler(int literalLength, int formattedCount, ref S1 s1) : this() { s1.Handler = this; }
Diagnostic(ErrorCode.ERR_EscapeVariable, "this").WithArguments("out CustomHandler").WithLocation(10, 100),
// (15,9): error CS8350: This combination of arguments to 'CustomHandler.M2(ref S1, CustomHandler)' is disallowed because it may expose variables referenced by parameter 'handler' outside of their declaration scope
// M2(ref s1, $"{s2}");
Diagnostic(ErrorCode.ERR_CallArgMixing, @"M2(ref s1, $""{s2}"")").WithArguments("CustomHandler.M2(ref S1, CustomHandler)", "handler").WithLocation(15, 9),
Expand Down Expand Up @@ -13862,6 +13865,9 @@ static void M(ref S s, [InterpolatedStringHandlerArgument(""s"")] CustomHandler

comp = CreateCompilation(new[] { code, InterpolatedStringHandlerAttribute, InterpolatedStringHandlerArgumentAttribute }, parseOptions: TestOptions.Regular11, targetFramework: TargetFramework.NetCoreApp);
comp.VerifyDiagnostics(
// (5,97): error CS8352: Cannot use variable 'out CustomHandler' in this context because it may expose referenced variables outside of their declaration scope
// public CustomHandler(int literalLength, int formattedCount, ref S s) : this() { s.Handler = this; }
Diagnostic(ErrorCode.ERR_EscapeVariable, "this").WithArguments("out CustomHandler").WithLocation(5, 97),
// (17,15): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference
// M(ref s, $"{1}");
Diagnostic(ErrorCode.ERR_RefReturnLvalueExpected, "s").WithLocation(17, 15),
Expand Down
8 changes: 7 additions & 1 deletion src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11278,6 +11278,9 @@ public ref struct S1

var comp = CreateCompilation(new[] { code, InterpolatedStringHandlerAttribute, InterpolatedStringHandlerArgumentAttribute }, targetFramework: TargetFramework.NetCoreApp);
comp.VerifyDiagnostics(
// (10,107): error CS8352: Cannot use variable 'out CustomHandler' in this context because it may expose referenced variables outside of their declaration scope
// public CustomHandler(int literalLength, int formattedCount, scoped ref S1 s1) : this() { s1.Handler = this; }
Diagnostic(ErrorCode.ERR_EscapeVariable, "this").WithArguments("out CustomHandler").WithLocation(10, 107),
// (15,9): error CS8350: This combination of arguments to 'CustomHandler.M2(ref S1, CustomHandler)' is disallowed because it may expose variables referenced by parameter 'handler' outside of their declaration scope
// M2(ref s1, $"""{s2}""");
Diagnostic(ErrorCode.ERR_CallArgMixing, @"M2(ref s1, $""""""{s2}"""""")").WithArguments("CustomHandler.M2(ref S1, CustomHandler)", "handler").WithLocation(15, 9),
Expand Down Expand Up @@ -11312,7 +11315,10 @@ static void M(ref S s, [InterpolatedStringHandlerArgument(""s"")] CustomHandler
}";

var comp = CreateCompilation(new[] { code, InterpolatedStringHandlerAttribute, InterpolatedStringHandlerArgumentAttribute }, targetFramework: TargetFramework.NetCoreApp);
comp.VerifyDiagnostics();
comp.VerifyDiagnostics(
// (5,104): error CS8352: Cannot use variable 'out CustomHandler' in this context because it may expose referenced variables outside of their declaration scope
Copy link
Contributor

Choose a reason for hiding this comment

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

would this scenario work if [UnscopedRef] were applied to the method?

Copy link
Member Author

@jaredpar jaredpar Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No. The [UnscopedRef] would only apply to the ref portion of this. That doesn't come into play here though as the violation is the following:

s.Handler = this;

In this case s has a STE of containing method and this has STE of return only as we model this as an out.

Also note that we specifically disallow [UnscopedRef] on a struct constructor because it would create some really awkward interactions. At that point the STE and RSTE would be equivalent and the ctor could capture refs to itself. So cyclic self assignment in a ctor. I think we could now model that correctly but have it disallowed because it creates really awkward scenarios and I can't see a scenario in which it would be justified. If one was provided I'd be interested in allowing but it won't help this particular issue.

Copy link
Contributor

@RikkiGibson RikkiGibson Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, of course. STE of this is not customizable and neither is STE of a scoped ref parameter. Thanks!

Though I guess if it were possible to say ref scoped S, you would be able to value-assign this to it. I wonder if that fact is related to why ref scoped was cut? I wasn't following as closely when that happened.

Copy link
Member Author

@jaredpar jaredpar Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if that fact is related to why ref scoped was cut? I wasn't following as closely when that happened.

The initial design only had essentially two scopes: calling method and containing method. A parameter which was ref scoped had it's STS set to containing method. That decision though was wrong and it allowed for easy exploitation as follows:

void M(ref scoped Span<int> parameter)
{
  Span<int> local = stackalloc int[42];
  parameter = local; // uh oh 
}

This works because the STS of both values was containing method so the assignment just falls out. At the time we realized the mistake here we still were also looking at other unintended design issues we'd created and cut this on a focus of

Get back to the original intent of just allowing ref fields in current use with minimal additions to language

Now that we've gotten comfortable representing multiple scopes in the design it is possible to model ref scoped. For example it could be done effectively as:

Span<int> M(Span<int> p1, ref scoped Span<int> p2, scoped Span<int> p3)
// Becomes
'a Span<int> M('a Span<inT>, 'b ref 'c Span<int> p2, 'd Span<int> p3) 
  lifetime 'a >= 'b
  lifetime 'c >= 'd

The relationship between 'd and 'c means that you can assign these values to scoped locals but not the other way around. There is very deliberately no relationship because that requires thinking through a bit more.

This also introduces the other problem that we have to deal with: variance. Essentially we have to make sure that in the above design 'b ref 'c Span<int> cannot be assigned to 'b ref 'd Span<int> as that just recreates the original problem. Yet 'c Span<int> can be assigned to 'd Span<int>. That is just assigning a Span<int> with some value to scoped Span<int> which is always safe.

Copy link
Contributor

@RikkiGibson RikkiGibson Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

right..with a 2-scope solution, where STE of ref scoped parameters is current method, it's sort of unsafe by construction. The caller is essentially saying to the callee, "somehow, I am passing you references to your own local variables".

// public CustomHandler(int literalLength, int formattedCount, scoped ref S s) : this() { s.Handler = this; }
Diagnostic(ErrorCode.ERR_EscapeVariable, "this").WithArguments("out CustomHandler").WithLocation(5, 104));
}

[Theory, WorkItem(54703, "https://github.com/dotnet/roslyn/issues/54703")]
Expand Down
122 changes: 82 additions & 40 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,15 +1703,8 @@ ref struct S1
Diagnostic(ErrorCode.ERR_CallArgMixing, "MayAssign1(__arglist(ref inner, ref rOuter))").WithArguments("Program.MayAssign1(__arglist)", "__arglist").WithLocation(23, 9)
);

// Breaking change in C#11: A ref to ref struct argument is considered
// an unscoped reference when passed to an __arglist.
// Same errors modulo the scoped
CreateCompilationWithMscorlibAndSpan(text).VerifyDiagnostics(
// (17,34): error CS8168: Cannot return local 'rOuter' by reference because it is not a ref local
// MayAssign2(__arglist(ref rOuter, ref rOuter));
Diagnostic(ErrorCode.ERR_RefReturnLocal, "rOuter").WithArguments("rOuter").WithLocation(17, 34),
// (17,9): error CS8350: This combination of arguments to 'Program.MayAssign2(__arglist)' is disallowed because it may expose variables referenced by parameter '__arglist' outside of their declaration scope
// MayAssign2(__arglist(ref rOuter, ref rOuter));
Diagnostic(ErrorCode.ERR_CallArgMixing, "MayAssign2(__arglist(ref rOuter, ref rOuter))").WithArguments("Program.MayAssign2(__arglist)", "__arglist").WithLocation(17, 9),
// (20,46): error CS8352: Cannot use variable 'rInner' in this context because it may expose referenced variables outside of their declaration scope
// MayAssign2(__arglist(ref rOuter, ref rInner));
Diagnostic(ErrorCode.ERR_EscapeVariable, "rInner").WithArguments("rInner").WithLocation(20, 46),
Expand Down Expand Up @@ -2878,13 +2871,13 @@ ref struct S1
";
var comp = CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion));
comp.VerifyDiagnostics(
// (16,30): error CS8526: Cannot use variable 'local' in this context because it may expose referenced variables outside of their declaration scope
// (16,30): error CS8352: Cannot use variable 'local' in this context because it may expose referenced variables outside of their declaration scope
// sp = MayWrap(ref local);
Diagnostic(ErrorCode.ERR_EscapeVariable, "local").WithArguments("local").WithLocation(16, 30),
// (16,18): error CS8521: Cannot use a result of 'Program.MayWrap(ref Span<int>)' in this context because it may expose variables referenced by parameter 'arg' outside of their declaration scope
// (16,18): error CS8347: Cannot use a result of 'Program.MayWrap(ref Span<int>)' in this context because it may expose variables referenced by parameter 'arg' outside of their declaration scope
// sp = MayWrap(ref local);
Diagnostic(ErrorCode.ERR_EscapeCall, "MayWrap(ref local)").WithArguments("Program.MayWrap(ref System.Span<int>)", "arg").WithLocation(16, 18),
// (22,20): error CS8526: Cannot use variable 'sp1' in this context because it may expose referenced variables outside of their declaration scope
// (22,20): error CS8352: Cannot use variable 'sp1' in this context because it may expose referenced variables outside of their declaration scope
// return sp1;
Diagnostic(ErrorCode.ERR_EscapeVariable, "sp1").WithArguments("sp1").WithLocation(22, 20)
);
Expand Down Expand Up @@ -2949,6 +2942,7 @@ public void MemberOfReadonlyRefLikeEscape(LanguageVersion languageVersion)
{
var text = @"
using System;
using System.Diagnostics.CodeAnalysis;
public static class Program
{
public static void Main()
Expand All @@ -2957,8 +2951,13 @@ public static void Main()
Span<int> value1 = stackalloc int[1];
new SR().TryGet(out value1);

// error, TryGet can write into the instance
// Ok, the new value can be copied into SW but not the
// ref to the value
new SW().TryGet(out value1);

// Error as the ref of this can escape into value2
Span<int> value2 = default;
new SW().TryGet2(out value2);
}
}

Expand All @@ -2976,16 +2975,37 @@ public void TryGet(out Span<int> result)
{
result = default;
}

[UnscopedRef]
public void TryGet2(out Span<int> result)
{
result = default;
}
}
";
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (12,33): error CS8526: Cannot use variable 'value1' in this context because it may expose referenced variables outside of their declaration scope
// new SW().TryGet(out value1);
Diagnostic(ErrorCode.ERR_EscapeVariable, "value1").WithArguments("value1").WithLocation(12, 33),
// (12,13): error CS8524: This combination of arguments to 'SW.TryGet(out Span<int>)' is disallowed because it may expose variables referenced by parameter 'result' outside of their declaration scope
// new SW().TryGet(out value1);
Diagnostic(ErrorCode.ERR_CallArgMixing, "new SW().TryGet(out value1)").WithArguments("SW.TryGet(out System.Span<int>)", "result").WithLocation(12, 13)
);
if (languageVersion == LanguageVersion.CSharp10)
{
CreateCompilationWithMscorlibAndSpan(new[] { text, UnscopedRefAttributeDefinition }, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (14,13): error CS8350: This combination of arguments to 'SW.TryGet(out Span<int>)' is disallowed because it may expose variables referenced by parameter 'result' outside of their declaration scope
// new SW().TryGet(out value1);
Diagnostic(ErrorCode.ERR_CallArgMixing, "new SW().TryGet(out value1)").WithArguments("SW.TryGet(out System.Span<int>)", "result").WithLocation(14, 13),
// (14,33): error CS8352: Cannot use variable 'value1' in this context because it may expose referenced variables outside of their declaration scope
// new SW().TryGet(out value1);
Diagnostic(ErrorCode.ERR_EscapeVariable, "value1").WithArguments("value1").WithLocation(14, 33)
);

}
else
{
CreateCompilationWithMscorlibAndSpan(new[] { text, UnscopedRefAttributeDefinition }, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (18,13): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference
// new SW().TryGet2(out value2);
Diagnostic(ErrorCode.ERR_RefReturnLvalueExpected, "new SW()").WithLocation(18, 13),
// (18,13): error CS8350: This combination of arguments to 'SW.TryGet2(out Span<int>)' is disallowed because it may expose variables referenced by parameter 'this' outside of their declaration scope
// new SW().TryGet2(out value2);
Diagnostic(ErrorCode.ERR_CallArgMixing, "new SW().TryGet2(out value2)").WithArguments("SW.TryGet2(out System.Span<int>)", "this").WithLocation(18, 13)
);
}
}

[WorkItem(21911, "https://github.com/dotnet/roslyn/issues/21911")]
Expand Down Expand Up @@ -3503,17 +3523,28 @@ public static class Extensions
public static void Deconstruct(ref this Span<int> self, out Span<int> x, out Span<int> y) => throw null;
}
";
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (8,9): error CS1510: A ref or out value must be an assignable variable
// (global, global) = global;
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "(global, global) = global").WithLocation(8, 9),
// (8,9): error CS8352: Cannot use variable '(global, global) = global' in this context because it may expose referenced variables outside of their declaration scope
// (global, global) = global;
Diagnostic(ErrorCode.ERR_EscapeVariable, "(global, global) = global").WithArguments("(global, global) = global").WithLocation(8, 9),
// (8,28): error CS8350: This combination of arguments to 'Extensions.Deconstruct(ref Span<int>, out Span<int>, out Span<int>)' is disallowed because it may expose variables referenced by parameter 'x' outside of their declaration scope
// (global, global) = global;
Diagnostic(ErrorCode.ERR_CallArgMixing, "global").WithArguments("Extensions.Deconstruct(ref System.Span<int>, out System.Span<int>, out System.Span<int>)", "x").WithLocation(8, 28)
);
if (languageVersion == LanguageVersion.CSharp10)
{
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (8,9): error CS1510: A ref or out value must be an assignable variable
// (global, global) = global;
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "(global, global) = global").WithLocation(8, 9),
// (8,9): error CS8352: Cannot use variable '(global, global) = global' in this context because it may expose referenced variables outside of their declaration scope
// (global, global) = global;
Diagnostic(ErrorCode.ERR_EscapeVariable, "(global, global) = global").WithArguments("(global, global) = global").WithLocation(8, 9),
// (8,28): error CS8350: This combination of arguments to 'Extensions.Deconstruct(ref Span<int>, out Span<int>, out Span<int>)' is disallowed because it may expose variables referenced by parameter 'x' outside of their declaration scope
// (global, global) = global;
Diagnostic(ErrorCode.ERR_CallArgMixing, "global").WithArguments("Extensions.Deconstruct(ref System.Span<int>, out System.Span<int>, out System.Span<int>)", "x").WithLocation(8, 28)
);
}
else
{
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (8,9): error CS1510: A ref or out value must be an assignable variable
// (global, global) = global;
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "(global, global) = global").WithLocation(8, 9)
);
}
}

[Fact]
Expand Down Expand Up @@ -3550,10 +3581,7 @@ public static class Extensions
CreateCompilationWithMscorlibAndSpan(text, options: TestOptions.UnsafeDebugDll, parseOptions: TestOptions.Regular11).VerifyDiagnostics(
// (8,9): error CS1510: A ref or out value must be an assignable variable
// (global, global) = global;
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "(global, global) = global").WithLocation(8, 9),
// (8,9): warning CS9077: Use of variable '(global, global) = global' in this context may expose referenced variables outside of their declaration scope
// (global, global) = global;
Diagnostic(ErrorCode.WRN_EscapeVariable, "(global, global) = global").WithArguments("(global, global) = global").WithLocation(8, 9)
Diagnostic(ErrorCode.ERR_RefLvalueExpected, "(global, global) = global").WithLocation(8, 9)
);
}

Expand Down Expand Up @@ -4080,11 +4108,25 @@ public ref struct S
";
// Tracking issue: https://github.com/dotnet/roslyn/issues/22361

CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (9,9): error CS8352: Cannot use variable 'local1' in this context because it may expose referenced variables outside of their declaration scope
// local1.M(out S local2);
Diagnostic(ErrorCode.ERR_EscapeVariable, "local1").WithArguments("local1").WithLocation(9, 9)
);
if (languageVersion == LanguageVersion.CSharp10)
{
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (9,9): error CS8352: Cannot use variable 'local1' in this context because it may expose referenced variables outside of their declaration scope
// local1.M(out S local2);
Diagnostic(ErrorCode.ERR_EscapeVariable, "local1").WithArguments("local1").WithLocation(9, 9)
);
}
else
{
CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular.WithLanguageVersion(languageVersion)).VerifyDiagnostics(
// (9,9): error CS8352: Cannot use variable 'local1' in this context because it may expose referenced variables outside of their declaration scope
// local1.M(out S local2); // we'd want this to succeed, but determine the safe-to-escape scope for local2 based on the invocation that declared it
Diagnostic(ErrorCode.ERR_EscapeVariable, "local1").WithArguments("local1").WithLocation(9, 9),
// (9,9): error CS8350: This combination of arguments to 'S.M(out S)' is disallowed because it may expose variables referenced by parameter 'this' outside of their declaration scope
// local1.M(out S local2); // we'd want this to succeed, but determine the safe-to-escape scope for local2 based on the invocation that declared it
Diagnostic(ErrorCode.ERR_CallArgMixing, "local1.M(out S local2)").WithArguments("S.M(out S)", "this").WithLocation(9, 9)
);
}
}

[Theory]
Expand Down
Loading