Skip to content

Create default arguments during binding#49186

Merged
RikkiGibson merged 6 commits intodotnet:masterfrom
RikkiGibson:fix-47352-2
Nov 14, 2020
Merged

Create default arguments during binding#49186
RikkiGibson merged 6 commits intodotnet:masterfrom
RikkiGibson:fix-47352-2

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Nov 5, 2020 •
edited
Loading

Closes #17243
Closes #47352

This PR has been organized to try and improve reviewability.

  • The first commit moves some methods from LocalRewriter_Call to Binder_Invocation
  • The second commit modifies the implementation to make everything work.

@RikkiGibson

This comment has been minimized.

Sign in to view
@RikkiGibson RikkiGibson mentioned this pull request Nov 9, 2020
Merged
@RikkiGibson RikkiGibson force-pushed the fix-47352-2 branch 2 times, most recently from c4ad664 to b6cd75a Compare November 9, 2020 23:11
@RikkiGibson RikkiGibson marked this pull request as ready for review November 9, 2020 23:41
@RikkiGibson RikkiGibson requested a review from a team as a code owner November 9, 2020 23:41
@jcouv jcouv added the Feature - Nullable Reference Types Nullable Reference Types label Nov 9, 2020
333fred
333fred reviewed Nov 10, 2020
View reviewed changes
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.

Done review pass (commit 2)

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Nov 11, 2020

Thanks for the feedback @333fred.

cston
cston reviewed Nov 11, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated
{
case BoundKind.PropertyGroup:
expr = BindIndexedPropertyAccess((BoundPropertyGroup)expr, mustHaveAllOptionalParameters: false, diagnostics: diagnostics);
expr = BindIndexerDefaultArguments(expr, valueKind, diagnostics);
Copy link
Contributor

@cston cston Nov 11, 2020

Choose a reason for hiding this comment

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

BindIndexerDefaultArguments [](start = 27, length = 27)

Does this have any effect?

Copy link
Member Author

@RikkiGibson RikkiGibson Nov 12, 2020

Choose a reason for hiding this comment

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

Yes. In some scenarios, we can call an indexer whose parameters are all optional by omitting the element access syntax entirely. Here's a test that demonstrates this:

roslyn/src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

Lines 3080 to 3139 in 98cbb71

[WorkItem(1006447, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1006447")]
[Fact]
public void Bug1006447_1()
{
const string vbSource =
@"Imports System
Imports System.Runtime.CompilerServices
Imports System.Runtime.InteropServices
Imports System.Text
<ComImport>
<Guid(""1F9C3731-6AA1-498A-AFA0-359828FCF0CE"")>
Public Interface I
Property X(Optional i as Integer = 0, <CallerFilePath> Optional s As String = Nothing) As StringBuilder
End Interface
Public Class A
Implements I
Public Property X(Optional i as Integer = 0, Optional s As String = Nothing) As StringBuilder Implements I.X
Get
Console.WriteLine(""Get X(""""{0}"""")"", s)
Return New StringBuilder
End Get
Set(value As StringBuilder)
Console.WriteLine(""Set X(""""{0}"""")"", s)
End Set
End Property
End Class";
var vbReference = BasicCompilationUtils.CompileToMetadata(vbSource, references: new[] { MscorlibRef_v4_0_30316_17626, SystemCoreRef });
const string csSource =
@"using System;
class C
{
I P = new A();
static void Main()
{
new C().P.X = null;
new C().P.X[1] = null;
new C { P = { X = null } };
new C { P = { X = { Length = 0 } } };
}
}
";
var compilation = CreateCompilationWithMscorlib45(
new[] { SyntaxFactory.ParseSyntaxTree(csSource, path: @"C:\filename", encoding: Encoding.UTF8) },
new[] { SystemCoreRef, vbReference },
TestOptions.ReleaseExe);
CompileAndVerify(compilation, expectedOutput:
@"Set X(""C:\filename"")
Set X(""C:\filename"")
Set X(""C:\filename"")
Get X(""C:\filename"")
");
}

cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated
if (useSetAccessor)
{
parameters = parameters.RemoveAt(parameters.Length - 1);
Debug.Assert(parameters.Length == indexerAccess.Indexer.Parameters.Length);
Copy link
Contributor

@cston cston Nov 11, 2020

Choose a reason for hiding this comment

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

Debug.Assert [](start = 20, length = 12)

Consider moving assert after if so it's executed always.

cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
cston
cston reviewed Nov 11, 2020
View reviewed changes
@RikkiGibson
Copy link
Member Author

RikkiGibson commented Nov 12, 2020

@cston @333fred I believe I have addressed all your feedback. Please take another look.

333fred
333fred reviewed Nov 13, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
{
// This is only expected to occur in recursive error scenarios, for example: `object F(object param = F()) { }`
// We return a non-error expression here to ensure ERR_DefaultValueMustBeConstant (or another appropriate diagnostics) is produced by the caller.
return new BoundDefaultExpression(syntax, parameterType) { WasCompilerGenerated = true };
Copy link
Member

@333fred 333fred Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this could technically change observable behavior for nullability warnings or IOperation results, but I think that's fine.

Copy link
Member Author

@RikkiGibson RikkiGibson Nov 13, 2020

Choose a reason for hiding this comment

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

If we decide to start giving nullability warnings for default arguments then we might want to add a nullable suppression to this node, but we'll cross that bridge if we come to it

333fred
333fred approved these changes Nov 13, 2020
View reviewed changes
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.

Done review pass (commit 4). Just a couple of minor comments left.

333fred
333fred approved these changes Nov 13, 2020
View reviewed changes
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 6)

@runfoapp runfoapp bot mentioned this pull request Nov 14, 2020
Closed
@RikkiGibson RikkiGibson merged commit bd3e3a2 into dotnet:master Nov 14, 2020
@ghost ghost added this to the Next milestone Nov 14, 2020
@RikkiGibson RikkiGibson mentioned this pull request Nov 14, 2020
Merged
@RikkiGibson RikkiGibson deleted the fix-47352-2 branch November 14, 2020 21:57
jaredpar
jaredpar reviewed Nov 16, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
// that never expires, then the failure mode will be to spin wait forever.
// For debug purposes let's instead use a token which expires after a modest amount of time
// to wait for the default syntax value to be available.
var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5));
Copy link
Member

@jaredpar jaredpar Nov 16, 2020

Choose a reason for hiding this comment

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

We need to pick a much higher number here. It is more than reasonable for a thread to hang for 5 secnds in CI

Copy link
Member Author

@RikkiGibson RikkiGibson Nov 24, 2020

Choose a reason for hiding this comment

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

Looks like integration tests use 1 minute to wait for things to complete in some places. I'll go with that as a starting point

roslyn/src/VisualStudio/IntegrationTest/TestUtilities/Helper.cs

Line 20 in 4262935

public static readonly TimeSpan HangMitigatingTimeout = TimeSpan.FromMinutes(1);

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
#else
var token = CancellationToken.None;
#endif
state.SpinWaitComplete(CompletionPart.EndDefaultSyntaxValue, token);
Copy link
Member

@jaredpar jaredpar Nov 16, 2020

Choose a reason for hiding this comment

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

Feel like ForceComplete should change to assert that this part has completed. That is the pattern we usually have elsewhere.

Copy link
Member Author

@RikkiGibson RikkiGibson Nov 24, 2020

Choose a reason for hiding this comment

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

I didn't find examples of asserting that parts are completed instead of spin-waiting for them to complete when looking through the overrides of Symbol.ForceComplete. Could you provide an example of what you had in mind?

333fred added a commit to 333fred/roslyn that referenced this pull request Nov 16, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
037c07d 
* upstream/master: (148 commits)
  Remove unnecessary ArrayBuilder in MakeCallWithNoExplicitArgument (dotnet#49377)
  Revert "Skip binary for determinism checking"
  Use deterministic metadata names for emitted anonymous type fields. (dotnet#49370)
  Reuse LSP solutions if they don't need re-forking (dotnet#49305)
  Small nullability fixes (dotnet#49279)
  Create default arguments during binding (dotnet#49186)
  Clean nits
  Backport dotnet#48420 to release/dev16.8 (dotnet#49357)
  Rewrite AnalyzeFileReference.GetSupportedLanguages without LINQ (dotnet#49337)
  Use .Any extension of ImmutableArray(Of Symbol) (dotnet#48980)
  fix 'remove unnecessary cast' when doing bitwise ops on unsigned values.
  Harden, document, cross-link XunitDisposeHook
  Simplify x86 hook
  Fix split comment exporting for VB.
  Code style fix
  Overwrite xunit's app domain handling to not call AppDomain.Unload
  Revert pragma suppression
  Remove blank line
  Revert file
  Move block structure code back to Features layer
  ...
This was referenced Nov 22, 2020
Closed
Merged
RikkiGibson
RikkiGibson commented Nov 22, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
BoundExpression defaultValue;
if (callerSourceLocation is object && parameter.IsCallerLineNumber)
{
int line = GetCallerLocation(syntax).SourceTree.GetDisplayLineNumber(callerSourceLocation.SourceSpan);
Copy link
Member Author

@RikkiGibson RikkiGibson Nov 22, 2020

Choose a reason for hiding this comment

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

Should use callerSourceLocation here instead of GetCallerLocation here

@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
@RikkiGibson RikkiGibson mentioned this pull request Nov 24, 2020
Merged
@333fred 333fred mentioned this pull request Dec 30, 2020
Closed
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 31, 2020
@333fred
Respond to feedback:
f1e43f5 
* Restore default argument binding behavior to be like it was before dotnet#49186, only asserting that parameters are optional, rather than checking. Prior code steps should have verified this.
* Don't bind default arguments for error cases.
* Add additional tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar left review comments

@333fred 333fred 333fred approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers Feature - Nullable Reference Types Nullable Reference Types

Projects

None yet

Milestone

16.9.P2

6 participants

@RikkiGibson @jaredpar @333fred @cston @jcouv @allisonchou