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

Fix overload check #20

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Fix overload check #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da5bd17
fixed correct overload check
jkurdek Apr 9, 2024
355bf34
disable test
jkurdek Apr 9, 2024
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
23 changes: 9 additions & 14 deletions src/Controls/src/BindingSourceGen/BindingSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ static BindingDiagnosticsWrapper GetBindingForGeneration(GeneratorSyntaxContext
method.Name.GetLocation().GetLineSpan().StartLinePosition.Character + 1
);

var overloadDiagnostics = VerifyCorrectOverload(method, context, t);
var overloadDiagnostics = VerifyCorrectOverload(invocation, context, t);

if (overloadDiagnostics.Length > 0)
{
Expand Down Expand Up @@ -107,18 +107,18 @@ static BindingDiagnosticsWrapper GetBindingForGeneration(GeneratorSyntaxContext
return new BindingDiagnosticsWrapper(codeWriterBinding, diagnostics.ToArray());
}

private static Diagnostic[] VerifyCorrectOverload(SyntaxNode method, GeneratorSyntaxContext context, CancellationToken t)
private static Diagnostic[] VerifyCorrectOverload(InvocationExpressionSyntax invocation, GeneratorSyntaxContext context, CancellationToken t)
{
var methodSymbolInfo = context.SemanticModel.GetSymbolInfo(method, cancellationToken: t);

if (methodSymbolInfo.Symbol is not IMethodSymbol methodSymbol) //TODO: Do we need this check?
var argumentList = invocation.ArgumentList.Arguments;
if (argumentList.Count < 2)
{
return [DiagnosticsFactory.UnableToResolvePath(method.GetLocation())];
return [DiagnosticsFactory.SuboptimalSetBindingOverload(invocation.GetLocation())];
}

if (methodSymbol.Parameters.Length < 2 || methodSymbol.Parameters[1].Type.Name != "Func")
var getter = argumentList[1].Expression;
if (getter is not LambdaExpressionSyntax)
{
return [DiagnosticsFactory.SuboptimalSetBindingOverload(method.GetLocation())];
return [DiagnosticsFactory.SuboptimalSetBindingOverload(getter.GetLocation())];
}

return Array.Empty<Diagnostic>();
Expand All @@ -127,12 +127,7 @@ private static Diagnostic[] VerifyCorrectOverload(SyntaxNode method, GeneratorSy
private static (ExpressionSyntax? lambdaBodyExpression, IMethodSymbol? lambdaSymbol, Diagnostic[] diagnostics) GetLambda(InvocationExpressionSyntax invocation, SemanticModel semanticModel)
{
var argumentList = invocation.ArgumentList.Arguments;
var getter = argumentList[1].Expression;

if (getter is not LambdaExpressionSyntax lambda)
{
return (null, null, [DiagnosticsFactory.GetterIsNotLambda(getter.GetLocation())]);
}
var lambda = (LambdaExpressionSyntax)argumentList[1].Expression;

if (lambda.Body is not ExpressionSyntax lambdaBody)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Controls/tests/BindingSourceGen.UnitTests/DiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace BindingSourceGen.UnitTests;

public class DiagnosticsTests
{
[Fact]
[Fact(Skip = "Improve detecting overloads")]
Copy link
Owner

Choose a reason for hiding this comment

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

What's wrong with the test? Does it report wrong diagnostic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we based our overload detection on checking the type of the arguments now it will report that we are using a wrong overload as the second argument is not lambda.

Copy link
Owner

Choose a reason for hiding this comment

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

OK. It's acceptable that it reports BSG0004 for now. Let's deal with this later and merge it as is.

public void ReportsErrorWhenGetterIsNotLambda()
{
var source = """
Expand Down
Loading