swig / swig

SWIG is a software development tool that connects programs written in C and C++ with a variety of high-level programming languages.
http://www.swig.org
Other
5.97k stars 1.26k forks source link

C#: Check nullable reference types and omit ? during typeof #3114

Open synvima opened 2 months ago

synvima commented 2 months ago

Omit the ? for generated swigMethodTypes when using a nullable reference type.

For nullable value types the generated code stays the same. E.g:

Fixes #3108.

wsfulton commented 2 months ago

Can you please provide a motivating example or even better add a test to the patch. Without an example and without any new documentation in the Manual/Doc directory, it's hard to know what feature you are trying to add as currently SWIG does not have a need to parse any nullable reference types. While this patch seems to be needed for std::optional, what is a simple stand-alone use case?

christophe-calmejane commented 2 months ago

Hi William, motivation for this comes from https://github.com/swig/swig/pull/3078 (for which I can add a unit test that will fail until this is merged, if you prefer we do it that way). I can't think of any stand-alone use case, as you said there is no need for it directly in SWIG (currently).

wsfulton commented 2 months ago

A testcase for #3078 is of course needed. Maybe it will help, but for the generic feature put in here I need a standalone simple testcase.

synvima commented 2 months ago

I'm preparing a minimal example. This should also apply to null pointer types.

synvima commented 4 weeks ago

I've created a minimal example using pointers that maps to nullable types in C#. However, I'm currently unable to run the CI tests successfully.

So the current test definitions as they are. _director_nullable_args.i:_

%module(directors="1") director_nullable_args
#if defined SWIGCSHARP

#include <typemaps.i>
#include <std_string.i>

%typemap(cstype)       std::string const * "string?"
%typemap(csin)         std::string const * "new System.Runtime.InteropServices.HandleRef()"
%typemap(csdirectorin) std::string const * "($iminput != global::System.IntPtr.Zero) ? System.Runtime.InteropServices.Marshal.PtrToStringAnsi($iminput) : null"
%typemap(directorin)   std::string const * "$input = ($1 != nullptr) ? (void*)$1->c_str() : nullptr;"

%typemap(cstype)       int const * "int?"
%typemap(csin)         int const * "new System.Runtime.InteropServices.HandleRef()"
%typemap(csdirectorin) int const * "($iminput != global::System.IntPtr.Zero) ? System.Runtime.InteropServices.Marshal.ReadInt32($iminput) : null"

%feature("director") TestObjectDirected;

%typemap(csimports) TestObjectDirected %{
#nullable enable
%}

%inline %{
class TestObjectDirected
{
public:
    TestObjectDirected()          = default;
    virtual ~TestObjectDirected() = default;

    virtual void onIndex(int const* index)
    {
    }

    virtual void onMessage(std::string const* message)
    {
    }

    void nextMessage()
    {
        if (counter++ % 2 == 0)
        {
            onIndex(nullptr);
            onMessage(nullptr);
        }
        else
        {
            std::string msg("Hello " + std::to_string(counter));
            onIndex(&counter);
            onMessage(&msg);
        }
    }

private:
    int counter { 0 };
};
%}

#endif

_director_nullable_args.cs:_

#nullable enable

using System;
using director_nullable_argsNamespace;

public class runme
{
  public class DeriveredTestObject : TestObjectDirected
  {
      public override void onIndex(int? index)
      {
          base.onIndex(index);
          Indexes.Add(index);
      }

      public override void onMessage(string? message)
      {
          base.onMessage(message);
          Messages.Add(message);
      }

      public readonly List<string?> Messages = [];
      public readonly List<int?> Indexes = [];
  }

  private static void check(string got, string expected)
  {
    if (got != expected)
      throw new ApplicationException("Failed, got: " + got + " expected: " + expected);
  }

  static void Main()
  {
    var testDerivedDirected = new DeriveredTestObject();

    testDerivedDirected.nextMessage(); // counter = 0
    testDerivedDirected.nextMessage(); // counter = 1
    testDerivedDirected.nextMessage(); // counter = 2
    testDerivedDirected.nextMessage(); // counter = 3

    // Check Indexes
    check(testDerivedDirected.Indexes.Count.ToString(), "4");
    check(testDerivedDirected.Indexes[0]?.ToString() ?? "null", "null");
    check(testDerivedDirected.Indexes[1]?.ToString() ?? "null", "2");
    check(testDerivedDirected.Indexes[2]?.ToString() ?? "null", "null");
    check(testDerivedDirected.Indexes[3]?.ToString() ?? "null", "4");

    // Check Messages
    check(testDerivedDirected.Messages.Count.ToString(), "4");
    check(testDerivedDirected.Messages[0] ?? "null", "null");
    check(testDerivedDirected.Messages[1] ?? "null", "Hello 2");
    check(testDerivedDirected.Messages[2] ?? "null", "null");
    check(testDerivedDirected.Messages[3] ?? "null", "Hello 4");
  }
}

In the windows pipeline, i'm getting this errors:

error CS0518: Predefined type 'System.Runtime.CompilerServices.NullableAttribute' is not defined or imported
error CS0518: Predefined type 'System.Runtime.CompilerServices.NullableContextAttribute' is not defined or imported

And in the linux pipeline, i'm getting this error:

error CS1024: Wrong preprocessor directive

It appears that the Windows pipeline targets an older .NET Framework 4 runtime, while the Linux pipeline runs entirely within the Mono runtime. Neither (?) of them supports the newer nullable reference types feature.

wsfulton commented 3 weeks ago

From the docs at https://swig.org/Doc4.3/CSharp.html#CSharp_introduction:

SWIG 3 and later requires .NET 2.0 at a minimum. There are some minor exceptions, where the minimum required is .NET 4.0. This is when using the std::complex and std::list STL containers.

Hence we use .NET 4 to test. Ideally we test the majority of the code on .NET 2 except where .NET 4 is the stated minimum, but we havn't figured out any creative solutions for that. I've actually got some work in progress to get the dotnet tool to work for .net core 6/8 as it is radically different to the .NET framework, but I need to get back to it so we can at least additionally test later .NET versions. This work is related to #3078.

I think we'll still need creating solutions to only test .NET 8 features on .NET 8. So maybe take that as a challenge to get this merge request to pass on older .NET versions. We'll probably need to check the version of csc.exe / dotnet and only enable .NET 8 tests if .NET is later enough. So something similar to what we do for testing newer c++ standards. If fancy tackling all that, please do, but you may want to just provide a testcase that works only on .NET 8 and later and update your pull request until we have that machinery in place.