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

bool does not implement IParsable<TSelf> #78523

Closed
znakeeye opened this issue Nov 17, 2022 · 10 comments · Fixed by #82836
Closed

bool does not implement IParsable<TSelf> #78523

znakeeye opened this issue Nov 17, 2022 · 10 comments · Fixed by #82836
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@znakeeye
Copy link

Description

I'm writing a parser and this one is quite problematic. For the most basic type, a boolean, the new IParsable<TSelf> interface is not available. This makes no sense.

I realize that there is little need for a bool.Parse(string, IFormatProvider?) but since the second parameter may be null, it would be a no-brainer to implement. See Boolean.cs(228). Just add the interface, and these methods:

public static bool Parse(string value, IFormatProvider? _) => Parse(value);
public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? _, [MaybeNullWhen(false)] out TSelf result)
{
    return TryParse(s, out result);
}

Reproduction Steps

Running VS 17.5 targeting .NET 7.0, simply create a console app with the code below.

internal class Program
{
    static void Main(string[] args)
    {
        Cast<int>("123");   // OK
        Cast<bool>("true"); // CS0315
    }

    public static TSelf Cast<TSelf>(string s) where TSelf : IParsable<TSelf> => TSelf.Parse(s, null);
}

Expected behavior

Boolean implements IParsable<TSelf>, just like all other primitive types.

Actual behavior

Boolean does not implement IParsable<TSelf>.

Regression?

No

Known Workarounds

Wherever a method expects IParsable<T> you can duplicate that method to handle bool specifically. E.g.:

internal class Program
{
    static void Main(string[] args)
    {
        Cast<int>("123");
        CastBool("true"); // CS0315
    }

    public static TSelf Cast<TSelf>(string s) where TSelf : IParsable<TSelf> => TSelf.Parse(s, null);
    public static bool CastBool(string s) => bool.Parse(s);
}

Configuration

.NET 7.0
Windows 11 (x64)

Other information

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Boolean.cs#L24

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 17, 2022
@svick
Copy link
Contributor

svick commented Nov 17, 2022

This is a subset of #67388.

@znakeeye
Copy link
Author

This is a subset of #67388.

Ah, really cool. I want to see that feature in .NET 7.01 😄

I guess my suggestion could theoretically break once #67388 is shipped. Though, I would still argue that my bug report is valid. Not having this interface for booleans is painful.

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Nov 17, 2022

I want to see that feature in .NET 7.0[.]1

Not going to happen. Servicing releases do not get new APIs.

@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

Description

I'm writing a parser and this one is quite problematic. For the most basic type, a boolean, the new IParsable<TSelf> interface is not available. This makes no sense.

I realize that there is little need for a bool.Parse(string, IFormatProvider?) but since the second parameter may be null, it would be a no-brainer to implement. See Boolean.cs(228). Just add the interface, and these methods:

public static bool Parse(string value, IFormatProvider? _) => Parse(value);
public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? _, [MaybeNullWhen(false)] out TSelf result)
{
    return TryParse(s, out result);
}

Reproduction Steps

Running VS 17.5 targeting .NET 7.0, simply create a console app with the code below.

internal class Program
{
    static void Main(string[] args)
    {
        Cast<int>("123");   // OK
        Cast<bool>("true"); // CS0315
    }

    public static TSelf Cast<TSelf>(string s) where TSelf : IParsable<TSelf> => TSelf.Parse(s, null);
}

Expected behavior

Boolean implements IParsable<TSelf>, just like all other primitive types.

Actual behavior

Boolean does not implement IParsable<TSelf>.

Regression?

No

Known Workarounds

Wherever a method expects IParsable<T> you can duplicate that method to handle bool specifically. E.g.:

internal class Program
{
    static void Main(string[] args)
    {
        Cast<int>("123");
        CastBool("true"); // CS0315
    }

    public static TSelf Cast<TSelf>(string s) where TSelf : IParsable<TSelf> => TSelf.Parse(s, null);
    public static bool CastBool(string s) => bool.Parse(s);
}

Configuration

.NET 7.0
Windows 11 (x64)

Other information

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Boolean.cs#L24

Author: znakeeye
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@dakersnar dakersnar removed the untriaged New issue has not been triaged by the area owner label Nov 18, 2022
@dakersnar dakersnar added this to the Future milestone Nov 18, 2022
@rickbrew
Copy link
Contributor

Yeah I just found myself needing bool support for IFormattable and IParsable<bool>. It already has the methods, it just doesn't specify the interfaces 🤔

@znakeeye
Copy link
Author

znakeeye commented Feb 1, 2023

Can we expect this to be fixed in .NET 8?

@tannergooding tannergooding added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Feb 1, 2023
@tannergooding
Copy link
Member

Depends on what API review says for the API.

@bartonjs
Copy link
Member

Adding the interface decl on bool and implementing the methods explicitly seems reasonable, though we should go all the way to ISpanParsable

namespace System;

public partial struct Boolean : ISpanParsable<Boolean>
{
    // ISpanParsable and IParsable members explicitly implemented (if they're not already present)
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 28, 2023
@tannergooding tannergooding self-assigned this Feb 28, 2023
@tannergooding
Copy link
Member

Will have a PR up covering this and #78842 shortly

@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants