Char is casted to int without warning in string.Remove(int startIndex)

[vsfeedback]

This issue has been moved from a ticket on Developer Community.

---

[severity:It bothers me. A fix would be nice]
Dear ladies and gentlemen,

The Problem:

    public static class InvalidChars
    {
        public const string InvalidNameChars = "";

public static string CleanName(this string input)
        {
            if (string. IsNullOrWhiteSpace(input))
            {
                return input;
            }

var newInput = input;
            foreach (var c in InvalidNameChars)
            {
                newInput = newInput.Remove(c);
            }

return newInput;
        }
    }

I was writing the following piece of code.
I originally wanted to use replace with an empty character, however then I saw Remove in the autocomplete dropdown and decided to try it.
Thankfully I saw the declaration of the function before proceeding:

There is no override that takes a char.
For some reason it just automatically casts the char into an int without any warnings.
I tried it with a fresh .NET 8 project (We are using .NET 8) as well, no warnings.
My VS Version is 17.13.6

While I don't think it's such an important issue, it could be dangerous if someone were to just write code and trust the editor. (Which I think we are all guilty of nowadays)

Demo.zip

Thank you for your time.

Sincerely,

Istvan Galfi

---

Original Comments

Feedback Bot on 4/13/2025, 10:13 PM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[ericstj]

Could be an opportunity for an analyzer. We probably have a few APIs like this where implicit conversion will do the wrong thing.

[Clockwork-Muse]

... the specific issue with this API aside, this is really not the most optimal way to do this operation; a compiled regex (especially if it can be created via the generator) is almost certainly going to be faster and use less memory, especially as the number of invalid characters increases. It also enables some other things like removing entire classes (ie, all whitespace) instead of listing them individually, or just removing everything but a set of valid characters (eg, [a-zA-Z]+). Care must also be taken when going by individual character, since the UTF16 encoding used for char has combining characters which can have some unexpected quirks.

Although if this is for human names, extra care must be taken, because the valid range of characters may be wider than you expect (eg, whitespace or punctuation).

Also, with things like this:

if (string. IsNullOrWhiteSpace(input))
{
    return input;
}

Should probably return String.Empty or null (depending on what the rest of the model looks like), to remove any extra whitespace.

[Light2027]

... the specific issue with this API aside, this is really not the most optimal way to do this operation; a compiled regex (especially if it can be created via the generator) is almost certainly going to be faster and use less memory, especially as the number of invalid characters increases. It also enables some other things like removing entire classes (ie, all whitespace) instead of listing them individually, or just removing everything but a set of valid characters (eg, [a-zA-Z]+). Care must also be taken when going by individual character, since the UTF16 encoding used for char has combining characters which can have some unexpected quirks.

Although if this is for human names, extra care must be taken, because the valid range of characters may be wider than you expect (eg, whitespace or punctuation).

Also, with things like this:

if (string. IsNullOrWhiteSpace(input))
{
return input;
}

Should probably return String.Empty or null (depending on what the rest of the model looks like), to remove any extra whitespace.

I agree that Regex in this case would be better.
However, that is not the point of this ticket.
This ticket is about how the behavior of VS in the example could potentially create bugs, should someone write code without double checking the methods they use.

[Clockwork-Muse]

I agree that Regex in this case would be better. However, that is not the point of this ticket. This ticket is about how the behavior of VS in the example could potentially create bugs, should someone write code without double checking the methods they use.

... Such a bug should never be committed to a repo, whatever project should already have test cases that would catch such a problem.
This method is also well suited to generative/randomized data testing, which would reduce to a minimum case very quickly

[Light2027]

I agree that Regex in this case would be better. However, that is not the point of this ticket. This ticket is about how the behavior of VS in the example could potentially create bugs, should someone write code without double checking the methods they use.

... Such a bug should never be committed to a repo, whatever project should already have test cases that would catch such a problem. This method is also well suited to generative/randomized data testing, which would reduce to a minimum case very quickly

Look, like I said, I don't think it's such an important bug:

While I don't think it's such an important issue, it could be dangerous if someone were to just write code and trust the editor. (Which I think we are all guilty of nowadays)

But for the love of life, VS is screaming about nullability left and right even in LINQ Statements where you explicitly check for null-s before hand like:
.Where(x => x.MyValue.HasValue)
.Select(x => x.MyValue.Value) // VS will warn ya here
What you just said applies far more to nullability than this case, yet nullability gives ya warnings all over but this doesn't?

Also, I just wanted to bring your attention to it, cause that's just what I do when I find something that I don't think is working right.
Having in depth discussions about this wasn't my point.
I will thank you for pointing out that RegEx would be better here, even if that wasn't point, and I shall unsubscribe, y'all may do with this issue what you wish.

[tannergooding]

Notably this is how C# has always worked. It has implicit conversions and that can lead to various quirks in how APIs are used.

Calling Math.Sqrt(someInt) or Math.Sqrt(someFloat) equally works, but returns a double result and this can lead to surprises.

Types with less precision can implicitly upcast to types with more precision and so types like char can upcast to int. Further arithmetic on small integral-like types (byte, sbyte, short, ushort, and char) produce int results rather than the input types.

You can also get into cases where an API takes ReadOnlySpan<char> and you "intended" it to be distinct characters, but a user can pass in a string (like "apple") and it can lead to confusion there as well.

Ultimately developers always need to be considerate of the APIs they're calling and if the behavior works as they expect.

---

Now with that being said, this may be a scenario that warrants exposing an overload that takes char as that would be likely what the user intended and unlikely to cause a source break (although there is still risk of that). It would also be more convenient than the current pattern of string.Replace("c", "")

I think it would need additional weigh in from @stephentoub before we opened such a proposal.

Otherwise, I agree with @ericstj that an analyzer might be beneficial here. I don't think it's something we could make that is general-purpose, however. It would end up needing to be very API specific because there are plenty of cases where such implicit conversions are intentional and desired, so we can't just flag any API that takes int where the user passes in char.