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

NCCS incorrectly treating some methods as extern #759

Open
devhawk opened this issue Jul 1, 2022 · 7 comments
Open

NCCS incorrectly treating some methods as extern #759

devhawk opened this issue Jul 1, 2022 · 7 comments

Comments

@devhawk
Copy link
Contributor

devhawk commented Jul 1, 2022

If I try and compile a contract with ExecutionEngine.Assert(false, "message"); statement, C# compilation succeeds but nccs compilation fails

error NC1002: Unknown method: Neo.SmartContract.Framework.ExecutionEngine.Assert(bool, string)
Compilation failed.

cc @erikzhang

@shargon
Copy link
Member

shargon commented Jul 1, 2022

@devhawk
Copy link
Contributor Author

devhawk commented Jul 2, 2022

@shargon Can you test compiling the contracts in this zip file using nccs 3.3.0? There are two single-file contracts in this zip file.

> nccs AssertContract.cs
error NC1002: Unknown method: Neo.SmartContract.Framework.ExecutionEngine.Assert(bool, string)
Compilation failed.

> nccs SomeToken.cs
error NC1002: Unknown method: Neo.SmartContract.Framework.TokenContract.TotalSupply()
Compilation failed.

AssertContract is a simple contract with a single method demonstrating the failure to compile a contract that calls the ExecutionEngine.Assert(bool, string) overload

public class AssertContract : SmartContract
{
    public static void TestAssertWithMessage()
    {
        ExecutionEngine.Assert(1 == 2, "Bad Math");
    }
}

SomeToken is the simplest TokenContract subclass you can implement, but it won't compile either - though the error message is different (unknown method TokenContract.TotalSupply) than the error message for AssertContract.

public class SomeToken : TokenContract
{
    public override byte Decimals() => 10;
    public override string Symbol() => "SOME";
}

TestContracts.zip

@devhawk
Copy link
Contributor Author

devhawk commented Jul 2, 2022

For AssertContract, nccs is throwing in ConvertExtern even though the ExecutionEngine.Assert(bool, string) overload is not an extern method

From https://github.com/neo-project/neo-devpack-dotnet/blob/master/src/Neo.Compiler.CSharp/MethodConvert.cs#L123

public void Convert(SemanticModel model)
{

// when converting ExecutionEngine.Assert(bool, string), 
// ContainingType.DeclaringSyntaxReferences.IsEmpty is true

    if (Symbol.IsExtern || Symbol.ContainingType.DeclaringSyntaxReferences.IsEmpty)
    {
        if (Symbol.Name == "_initialize")
        {
            ProcessStaticFields(model);
            if (context.StaticFieldCount > 0)
            {
                _instructions.Insert(0, new Instruction
                {
                    OpCode = OpCode.INITSSLOT,
                    Operand = new[] { (byte)context.StaticFieldCount }
                });
            }
        }
        else
        {

// Symbol.Name is "Assert" so we end up here. 
// But the Assert overload we're calling isn't extern so ConvertExtern fails

            ConvertExtern();
        }
    }

// remainder of Convert method omitted for clarity

@devhawk
Copy link
Contributor Author

devhawk commented Jul 2, 2022

Note, SomeToken contract ends up in ConvertExtern when trying to compile TotalSupply as well. I will update the issue title to reflect this

@devhawk devhawk changed the title ExecutionEngine.Assert(bool, string) overload fails to compile NCCS incorrectly treating some methods as extern Jul 2, 2022
@devhawk
Copy link
Contributor Author

devhawk commented Jul 2, 2022

FYI, it appears both of these contracts also fail to compile under nccs v3.1.0

@devhawk
Copy link
Contributor Author

devhawk commented Jul 3, 2022

I think I know what's going on here. In nccs, depending on the command line parameters the execution path goes thru either CompilationContext.CompileProject or CompilationContext.CompileSources.

The CompileSources path simply adds Neo.SmartContract.Framework.dll as a normal C# assembly reference. The CompileProject path parses the project.assets.json file to find the referenced packages. If the package includes source code, nccs creates a CSharpCompilation from the package source which is then added to the contract project.

Since the SmartContract Framework source code is not available when nccs goes down the CompileSources path, then Symbol.ContainingType.DeclaringSyntaxReferences.IsEmpty value is true for any method that is in the package reference (aka in SmartContract Framework). For methods like ExecutionEngine.Assert(bool) which are extern and implemented via attributes, it's no problem. For methods like ExecutionEngine.Assert(bool, string), the compilation fails because the source is not available.

To solve this, I think we need to:

  • Include the SmartContract Framework source in NCCS so that it can be used during compilation
  • nccs needs a mechanism to receive references, similar to C# CSC's --references option

@devhawk
Copy link
Contributor Author

devhawk commented Jul 8, 2022

FYI, I'm going to tackle this bug after I do #760

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

No branches or pull requests

2 participants