Skip to content

Conversation

@michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Dec 18, 2025

In this PR we introduce support for implicit span conversions. The feature is documented here. Even though the documentation states that this only applies to covariance convertible types (which is not defined), it appears that it is supported for all variance convertible types - and more generally all reference type conversions (between the element types).
That is, C# now supports implicit conversions to ReadOnlySpan. Some of these were previously supported by implicit operators, but now they are an integrated part of the language. Here are some examples:

string[] arr;
Span<string> span = arr;
ReadOnlySpan<string> rospan = arr; 

There is out-of-the-box extractor support for this.

DCA looks good. Even though the content of the PR might affect some queries (like cs/useless-upcast) it doesn't look like there are any changes to results.

@github-actions github-actions bot added the C# label Dec 18, 2025
@michaelnebel michaelnebel force-pushed the csharp/implicitspanconversions branch from 16a8982 to 741ef80 Compare December 18, 2025 10:59
@michaelnebel michaelnebel force-pushed the csharp/implicitspanconversions branch from 741ef80 to 1817f9c Compare December 18, 2025 11:51
@michaelnebel michaelnebel marked this pull request as ready for review December 18, 2025 15:00
@michaelnebel michaelnebel requested a review from a team as a code owner December 18, 2025 15:00
Copilot AI review requested due to automatic review settings December 18, 2025 15:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for implicit span conversions in C# 14, allowing automatic conversions between arrays, Span<T>, and ReadOnlySpan<T> types. This language feature is now integrated into the CodeQL library to properly model these conversions during analysis.

Key changes:

  • Implemented convSpan predicate to handle implicit span conversions including array-to-span, span-to-readonly-span, and string-to-readonly-span-of-char conversions
  • Added covariance support for span conversions through new convCovariance helper predicate
  • Added comprehensive test cases covering various span conversion scenarios

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
csharp/ql/lib/semmle/code/csharp/Conversion.qll Core implementation of span conversion logic and covariance helper
csharp/ql/test/library-tests/conversion/span/Span.cs Test cases demonstrating various span conversion scenarios
csharp/ql/test/library-tests/conversion/span/span.ql Query to verify span conversions are correctly identified
csharp/ql/test/library-tests/conversion/span/span.expected Expected output for span conversion test cases
csharp/ql/lib/change-notes/2025-12-18-implicit-span-conversions.md Release note documenting the new feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +510 to +515
private class SimpleArrayType extends ArrayType {
SimpleArrayType() {
this.getRank() = 1 and
this.getDimension() = 1
}
}
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The SimpleArrayType class lacks documentation explaining what distinguishes it from other array types. Add a docstring clarifying that this represents single-dimensional, zero-based arrays (SZ arrays), as getRank() = 1 and getDimension() = 1 restricts it to this specific array form.

Copilot uses AI. Check for mistakes.
@michaelnebel michaelnebel requested a review from hvitved December 19, 2025 12:04
@michaelnebel michaelnebel requested review from hvitved and removed request for hvitved January 6, 2026 09:22
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM, two comments.

| ReadOnlySpan<CovariantInterface<Derived>> | ReadOnlySpan<CovariantInterface<Base>> |
| Span<CovariantInterface<Derived>> | ReadOnlySpan<CovariantInterface<Base>> |
| Span<CovariantInterface<Derived>> | ReadOnlySpan<CovariantInterface<Derived>> |
| Span<Interface<Derived, string>> | ReadOnlySpan<Interface<Derived, string>> |
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected something like

| Span<Interface<Derived, string>> | ReadOnlySpan<Interface<Base, string>> |

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, that is a very nice catch! The documentation on variance conversion is a bit sketchy and I mistakenly assumed that all type parameters needed to be declared out. However, as you point out (and this is also accepted by the compiler), identity conversions are also accepted for non-covariant type parameters.
I will improve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that the implicit conversions are allowed for all variance convertible types (and not only covariant convertible types as hinted in the documentation of the feature)

@michaelnebel michaelnebel force-pushed the csharp/implicitspanconversions branch from 874dde9 to b686890 Compare January 6, 2026 14:08

// Assignments are included to illustrate that it compiles.
// Only the use of the types matter in terms of test output.
// Covariant conversions to ReadOnlySpan
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add a test case for something like

ReadOnlySpan<Base> x = // something of type `Derived[]`;
ReadOnlySpan<Base> x = // something of type `Span<Derived>`;

which I'm not sure the current logic is able to handle?

|
convIdentity(fromElementType, toElementType)
or
convVariance(fromElementType, toElementType)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should maybe be convRefTypeNonNull instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which should fix the issue I mention above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!
Yes, that I also what I am converging towards (started out with trying to use implicitConversionRestricted and then started narrowing and see how that aligns with what actually compiles).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right!

@michaelnebel michaelnebel requested a review from hvitved January 7, 2026 11:32
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM (assuming DCA is good).

@michaelnebel
Copy link
Contributor Author

DCA looks good.

@michaelnebel michaelnebel merged commit 7ed3d3f into github:main Jan 7, 2026
23 checks passed
@michaelnebel michaelnebel deleted the csharp/implicitspanconversions branch January 7, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants