Skip to content

Add support for static lambdas.#39121

Merged
jcouv merged 38 commits intodotnet:features/static-lambdasfrom
CyrusNajmabadi:staticLambdas2
Nov 11, 2019
Merged

Add support for static lambdas.#39121
jcouv merged 38 commits intodotnet:features/static-lambdasfrom
CyrusNajmabadi:staticLambdas2

Conversation

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 7, 2019

Adds impl support for dotnet/csharplang#275.

This should be reviewed with whitespace diffs off.

Followup to #39118

Test plan: #39606

@CyrusNajmabadi
Copy link
Contributor Author

Tagging @jcouv for eyes. Note: these are draft PRs.

@CyrusNajmabadi
Copy link
Contributor Author

@jcouv this is ready for review.

public override bool HasExplicitlyTypedParameterList { get { return false; } }
public override int ParameterCount { get { return _parameters.Length; } }
public override bool IsAsync { get { return false; } }
public override bool IsStatic => false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the lambda created for a query is never a static lambda.

ImmutableArray<TypeWithAnnotations> types,
ImmutableArray<string> names,
bool isAsync,
bool hasErrors = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: there appear to be no callers that provide this value. so it's always false. @jcouv would you like me to remove it entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the parameter, thanks.


In reply to: 332342232 [](ancestors = 332342232)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have removed it. Thanks!

@CyrusNajmabadi CyrusNajmabadi changed the base branch from master to features/static-lambdas October 8, 2019 18:37
@jinujoseph jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Oct 8, 2019
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Extra blank lines, here and in other tests.

Fixed.

@CyrusNajmabadi
Copy link
Contributor Author

Consider using parseOptions: TestOptions.RegularPreview for all tests that aren't specifically testing ERR_FeatureInPreview.

Done!

@CyrusNajmabadi
Copy link
Contributor Author

@cston Feedback responded to!

@cston
Copy link
Contributor

cston commented Oct 17, 2019

@CyrusNajmabadi

point me to the specification for static-local-functions

A proposal is at dotnet/csharplang#2891.

@jcouv
Copy link
Member

jcouv commented Oct 21, 2019

Opened a PR for speclet: dotnet/csharplang#2899

@jcouv jcouv mentioned this pull request Oct 31, 2019
34 tasks
@jcouv
Copy link
Member

jcouv commented Oct 31, 2019

FYI, opened test plan issue: #39606
Will need to update feature status

}

Location getLocationForDiagnostics(SyntaxNode node)
private static Location GetLocationForDiagnostics(SyntaxNode node)
Copy link
Member

Choose a reason for hiding this comment

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

GetLocationForDiagnostics [](start = 32, length = 25)

looks like this is still only used in one method. Could still be a local function

@RikkiGibson
Copy link
Member

It looks like we are just blocked on the file conflicts?

namespace Microsoft.CodeAnalysis.CSharp.UnitTests
{
[WorkItem(275, "https://github.com/dotnet/csharplang/issues/275")]
[CompilerTrait(CompilerFeature.AnonymousFunctions)]
Copy link
Member

@jcouv jcouv Oct 31, 2019

Choose a reason for hiding this comment

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

AnonymousFunctions [](start = 35, length = 18)

consider adding a new feature Consider renaming the new feature to StaticLambdas or something. I was confused by "AnonymousFunctions" ;-)

Func<int> f = static () => a;
}
}";
CreateCompilation(source).VerifyDiagnostics(
Copy link
Member

Choose a reason for hiding this comment

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

CreateCompilation [](start = 12, length = 17)

explicit language version would be good here (Regular8) so that we don't have to update this test again later

using Roslyn.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests
Copy link
Member

Choose a reason for hiding this comment

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

UnitTests [](start = 40, length = 9)

consider adding some tests that execute the code (CompileAndVerify with expectedOutput). Also, please include a test with static async or async static.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 37) with some test suggestions. Those could be added to test plan that I just created instead...

@jcouv jcouv self-assigned this Oct 31, 2019
@jcouv jcouv added this to the Compiler.Next milestone Oct 31, 2019
@CyrusNajmabadi
Copy link
Contributor Author

Those could be added to test plan that I just created instead...

I would appreciate that :)

@CyrusNajmabadi
Copy link
Contributor Author

i'll fix the current merge conflicts.

@jcouv jcouv merged commit 3ba5446 into dotnet:features/static-lambdas Nov 11, 2019
@jcouv
Copy link
Member

jcouv commented Nov 11, 2019

Merged. Thanks!

@CyrusNajmabadi
Copy link
Contributor Author

Yaay. Thanks!

@CyrusNajmabadi CyrusNajmabadi deleted the staticLambdas2 branch November 11, 2019 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants