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

Add initial documentation for the projection tooling #2512

Merged
merged 11 commits into from
Feb 23, 2024

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Feb 8, 2024

Description

This PR adds initial documentation for the tooling focusing on static methods and P/Invoke declarations.

@kotlarmilos kotlarmilos added the area-SwiftBindings Swift bindings for .NET label Feb 8, 2024
@kotlarmilos kotlarmilos self-assigned this Feb 8, 2024
@kotlarmilos kotlarmilos changed the title Add initial documentation skeleton Add initial documentation for the projection tooling Feb 8, 2024
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated
{
}
internal BarInt(SwiftNominalCtorArgument unused)
Copy link
Member

Choose a reason for hiding this comment

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

The internal constructor won't work well if the bindings are spanning multiple assemblies. I assume that we will eventually want to have one assembly per component for pre-generated bindings.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you consider a component? Should we split them by Swift namespaces?

@stephen-hawley Is there any particular reason the constructor has been set to internal?

Copy link
Member

Choose a reason for hiding this comment

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

I would consider each .framework a separate component.

docs/README.md Outdated
Comment on lines 56 to 58
"$s21NewClassCompilerTests6BarIntVMn",
"$s21NewClassCompilerTests6BarIntVN", "")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an explanation of what these parameters mean to this doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the current definition:

	[AttributeUsage (AttributeTargets.Class, AllowMultiple = false)]
	public sealed class SwiftStructAttribute : SwiftValueTypeAttribute {

		public SwiftStructAttribute (string libraryName, string nominalTypeDescriptor, string metadata, string witnessTable)
			: base (libraryName, nominalTypeDescriptor, metadata, witnessTable)
		{
		}
	}

It seems to be intended for future extensibility. If that's the case, I would removing it to simplify and narrow the scope.

Copy link
Member

Choose a reason for hiding this comment

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

It would make sense to have a way to identify the Swift type - specifically for the Projection B relies on Projection A scenario - B will need to be able to find which C# type is a specific Swift type from A.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the implemented interface sufficient for identifying the Swift type?

docs/README.md Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated
- **CLIInterface**: A command-line interface that orchestrates the tooling workflow.
- **SwiftReflector**: A component that aggregates public API definitions from Swift modules.
- **Dynamo**: A component that provides API for C# code generation.
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason Roslyn wouldn't work?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Rolf that we should use Roslyn. But I think it's OK to stick with the existing code for now and change this as a subsequent PR - but Roslyn should be the plan (unless there's some technical reason not to)

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, Roslyn is not a particularly great API for generating source code. Most source generators generate the source by printing it one line at time, without much help from Roslyn. Random example: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs#L461-L485

Copy link
Member

Choose a reason for hiding this comment

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

Another facet of this that has been mentioned is that source generators aren't the best solution when C# isn't the "source of truth". When the C# is the "source of truth", the source generators are appropriate - see LibraryImport or the COM source generator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the list in dotnet/runtime#95633.

docs/README.md Outdated
{
}
public byte[] SwiftData
Copy link
Member

Choose a reason for hiding this comment

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

What happens if code modifies the array directly?

var value = new BarInt ();
value.SwiftData [1] = 42;

I'm assuming nothing good... so maybe we should figure out a way to expose this so that users can't shoot themselves in the foot (as easily at least)?

One simple solution might be to mark it as EditorBrowseable.Never.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it need to be a public member?

Copy link
Member

Choose a reason for hiding this comment

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

If it needs to be public, could we use ReadOnlySpan or ReadOnlyMemory?

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated
{
}
public byte[] SwiftData
Copy link
Member

Choose a reason for hiding this comment

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

Is there a description somewhere how exactly this works and what is it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the docs, it is an interface that represents Swift named value types.

  • byte[] SwiftData { get; set } - gets an array of opaque data that represents the swift value type. The size of the array can be found by calling StructMarshal.Marshal.Strideof().

@stephen-hawley Could you please provide additional context on this?

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@kotlarmilos
Copy link
Member Author

This PR has been updated to align with the implementation in #2517. The scope has been narrowed to only include static methods and P/Invoke calls.

@kotlarmilos kotlarmilos merged commit 05d836d into feature/swift-bindings Feb 23, 2024
2 checks passed
@kotlarmilos kotlarmilos deleted the swift-bindings/docs branch February 23, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-SwiftBindings Swift bindings for .NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants