Skip to content

Decorated the out parameter of ResourceIdentifier.TryParse() with MaybeNullWhen#39332

Closed
abatishchev wants to merge 1 commit intoAzure:mainfrom
abatishchev:alex/TryParse-MaybeNullWhen
Closed

Decorated the out parameter of ResourceIdentifier.TryParse() with MaybeNullWhen#39332
abatishchev wants to merge 1 commit intoAzure:mainfrom
abatishchev:alex/TryParse-MaybeNullWhen

Conversation

@abatishchev
Copy link
Contributor

@abatishchev abatishchev commented Oct 17, 2023

Split from #39060.

Updated:

public static bool TryParse(string input, out ResourceIdentifier? result)

with:

public static bool TryParse(string input, [MaybeNullWhen(false)] out ResourceIdentifier? result)

@github-actions github-actions bot added Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Oct 17, 2023
@github-actions
Copy link

Thank you for your contribution @abatishchev! We will review the pull request and get back to you soon.

@abatishchev abatishchev changed the title Decorated the out parameter of TResourceIdentifier.TryParse() with MaybeNullWhen Decorated the out parameter of ResourceIdentifier.TryParse() with MaybeNullWhen Oct 17, 2023
/// </param>
/// <returns> True if the parse operation was successful; otherwise, false. </returns>
public static bool TryParse(string input, out ResourceIdentifier? result)
public static bool TryParse(string input, [MaybeNullWhen(false)] out ResourceIdentifier? result)
Copy link
Member

@m-nash m-nash Oct 17, 2023

Choose a reason for hiding this comment

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

Tested this to validate and apparently the warning always exists if the out var is nullable so the attribute seems to make no difference.

If we change to

public static bool TryParse(string input, [MaybeNullWhen(false)] out ResourceIdentifier result)

Then it seems to work as expected. I don't now the backwards compatibility rules on nullable annotation changes. @KrzysztofCwalina is there another link I should be using besides https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules to check?

My guess is going from ResourceIdentifier to ResourceIdentifier? is fine but going from ResourceIdentifier? to ResourceIdentifier is not?

namespace nullable
{
    internal class Program
    {
        static void Main(string[] args)
        {
            if (Foo.TryParse("x", out var y))
                Console.WriteLine(y.Length); //does not give warning

            Foo.TryParse("x", out var y2);
            Console.WriteLine(y2.Length); //gives warning cs8602
        }
    }

    internal class Foo
    {
        public static bool TryParse(string x, [MaybeNullWhen(false)] out string y)
        {
            y = null;
            return false;
        }
    }
}

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@abatishchev abatishchev deleted the alex/TryParse-MaybeNullWhen branch October 17, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants