Skip to content

Conversation

@sotteson1
Copy link
Contributor

  • If the config option is set not to output anonymous helper functions, leave them out.
  • If we didn't find an expression in its parents args, look through the parent's args to see if any of them are an implicit cast that then contains the expression we didn't find. Without this, I get a crash because the index is -1.

sotteson1 and others added 4 commits March 17, 2021 17:05
… at each arg to see if it's an implicit cast, and then check its expression for a match

Avoid adding anonymous helper properties if the config option is set
// If we didn't find the expression, try and find it under an implicit cast
if (index == -1)
{
for (int i = 0; i < callExpr.Args.Count; i++)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to write this as a single loop:

var args = callExpr.Args;
var index = -1;

for (int i = 0; i < args.Count; i++)
{
    var arg = args[i];

    if (IsStmtAsWritten(arg, unaryExprOrTypeTraitExpr))
    {
        index = i;
        break;
    }

    if (index == -1)
    {
        AddDiagnostic(DiagnosticLevel.Error, $"Failed to locate index of '{unaryExprOrTypeTraitExpr}' in callee declaration. Generated bindings may be incomplete.", calleeDecl);
    }
}

We can then have:

private bool IsStmtAsWritten(Stmt stmt, Stmt expectedStmt, bool removeParens = false)
{
    if (stmt == expectedStmt)
    {
        return true;
    }

    if (stmt is not Expr expr)
    {
        return false;
    }

    expr = GetExprAsWritten(expr, removeParens);
    return expr == expectedStmt;
}

This will make it easier to cover more cases in the future and to consistently look into an expr to remove implicit casts or other bits as necessary.

Copy link
Member

Choose a reason for hiding this comment

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

We have an existing IsStmtAsWritten that does a type check, rather than a value check, and it follows roughly this pattern.

@tannergooding
Copy link
Member

If we didn't find an expression in its parents args, look through the parent's args to see if any of them are an implicit cast that then contains the expression we didn't find. Without this, I get a crash because the index is -1.

It would be nice to have a small repro for this case. I'd like to add it and a test for ExcludeAnonymousFieldHelpers to ensure they don't break in the future. We have roughly 4 core modes we are currently testing:

  • Windows vs Unix
  • Compat vs Latest

@tannergooding
Copy link
Member

tannergooding commented Mar 24, 2021

Going to merge this after CI passes as it is correct as is and the tests/possible improvement to finding the CallExpr index can be done in a follow up PR.

@tannergooding tannergooding merged commit 90de91d into dotnet:main Mar 24, 2021
@sotteson1
Copy link
Contributor Author

Thanks for the quick response @tannergooding!

@tannergooding
Copy link
Member

Thanks for the quick response @tannergooding!

Sure thing @sotteson1! If you could confirm that things are working as expected for win32metdata now, then I can publish a new beta package to NuGet.

@sotteson1
Copy link
Contributor Author

Sure thing @sotteson1! If you could confirm that things are working as expected for win32metdata now, then I can publish a new beta package to NuGet.

I'm using binaries built with the latest changes and things look great, so I'm ready try out a new package.

@tannergooding
Copy link
Member

An 11.0.0-beta3 package should now be available: https://www.nuget.org/packages/ClangSharpPInvokeGenerator/11.0.0-beta3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants