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.

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

Support modifiers with simple lambda parameters. #75400

Merged
merged 126 commits into from
Jan 16, 2025

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Oct 4, 2024

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 4, 2024
if (p.Default != null)
{
if (firstDefault == -1)
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic moved from teh caller. no need for it to do a secondary pass to report errors when it can just do that here.

@@ -350,27 +365,6 @@ private UnboundLambda BindAnonymousFunction(AnonymousFunctionExpressionSyntax sy

var lambda = AnalyzeAnonymousFunction(syntax, diagnostics);
var data = lambda.Data;
if (data.HasExplicitlyTypedParameterList)
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic moved into AnalyzeAnonymousFunction. In particular, we want check parameters unilaterally even if there are types or no types (since we want to check modifier usage in the latter case). so the HasExplicitlyTypedParameterList had to go.

if (lambdaParameterType.IsErrorType())
{
continue;
}
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Oct 5, 2024

Choose a reason for hiding this comment

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

this waas already checked above on line 2107. no need to check again.

{
void M()
{
D d = (scoped {{(escaped ? "@" : "")}}scoped) => { };
Copy link
Member

Choose a reason for hiding this comment

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

(scoped {{(escaped ? "@" : "")}}scoped) => { }

I'm surprised the second scoped is treated as a modifier in (scoped scoped x) => { } but as an identifier in (scoped scoped) => { }.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's less about this being a context (like query-expressions) where a word just unilaterally becomes a keyword. Instead, it's more like: in the case where it could multiple things (like a modifier vs the type), then it's only a modifier (even though that breaks from prior interpretations).

The 'identifier' part is always mandatory, and never ambiguous. So it's just an identifier there.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner January 15, 2025 21:54
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) January 15, 2025 21:57
Assert.Equal(MethodKind.LambdaMethod, symbol.MethodKind);
Assert.Equal(RefKind.RefReadOnlyParameter, symbol.Parameters.Single().RefKind);
Assert.Equal(SpecialType.System_Int32, symbol.Parameters.Single().Type.SpecialType);
}
Copy link
Member

Choose a reason for hiding this comment

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

}

Consider testing:

delegate void D(ref readonly int x);

D d = (ref x) => { };

@@ -122,6 +122,7 @@
<Compile Include="$(MSBuildThisFileDirectory)RemoveUnusedMembers\CSharpRemoveUnusedMembersDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)RemoveUnusedParametersAndValues\CSharpRemoveUnusedParametersAndValuesDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionInitializer\CSharpUseCollectionInitializerDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseImplicitlyTypedLambdaExpression\UseImplicitlyTypedLambdaExpressionDiagnosticAnalyzer.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

UseImplicitlyTypedLambdaExpressionDiagnosticAnalyzer.cs

Is this file included in the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

no. this was for anotehr branch. removing.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) January 15, 2025 22:36
@CyrusNajmabadi CyrusNajmabadi merged commit 8c05cc8 into dotnet:main Jan 16, 2025
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jan 16, 2025
@CyrusNajmabadi CyrusNajmabadi deleted the refLambdas branch January 16, 2025 01:39
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 16, 2025
* upstream/main: (172 commits)
  Move impl of ILspWorkpace to EditorTestWorkspace/LspTestWorkspace (dotnet#76753)
  Field-backed properties: report diagnostic for variable named field declared in accessor (dotnet#76671)
  Add more tests
  Update 'use nameof instead of typeof' to support generic types
  Update dependencies from https://github.com/dotnet/arcade build 20250115.2
  Add docs
  invert
  Update src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems
  Update options
  Fixup tests
  fixup options
  Update tests
  Add test
  Fix trivia
  Handle params
  Support modifiers with simple lambda parameters. (dotnet#75400)
  Handle params
  Use helper
  Add fixes
  Flesh out
  ...
@dibarbet dibarbet modified the milestones: Next, 17.14 P1 Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants