Skip to content

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

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.

Sign up for GitHub

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

Closed
abatishchev wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.net5.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ public ResourceIdentifier(string resourceId) { }
public static bool operator <=(Azure.Core.ResourceIdentifier left, Azure.Core.ResourceIdentifier right) { throw null; }
public static Azure.Core.ResourceIdentifier Parse(string input) { throw null; }
public override string ToString() { throw null; }
public static bool TryParse(string input, out Azure.Core.ResourceIdentifier? result) { throw null; }
public static bool TryParse(string input, [System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute(false)] out Azure.Core.ResourceIdentifier? result) { throw null; }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public readonly partial struct ResourceType : System.IEquatable<Azure.Core.ResourceType>
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.net6.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ public ResourceIdentifier(string resourceId) { }
public static bool operator <=(Azure.Core.ResourceIdentifier left, Azure.Core.ResourceIdentifier right) { throw null; }
public static Azure.Core.ResourceIdentifier Parse(string input) { throw null; }
public override string ToString() { throw null; }
public static bool TryParse(string input, out Azure.Core.ResourceIdentifier? result) { throw null; }
public static bool TryParse(string input, [System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute(false)] out Azure.Core.ResourceIdentifier? result) { throw null; }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public readonly partial struct ResourceType : System.IEquatable<Azure.Core.ResourceType>
Expand Down
3 changes: 2 additions & 1 deletion sdk/core/Azure.Core/src/ResourceIdentifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;

Expand Down Expand Up @@ -526,7 +527,7 @@ public static ResourceIdentifier Parse(string input)
/// If the method returns false, result will be null.
/// </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
Copy Markdown
Member

@m-nash m-nash Oct 17, 2023 •
edited
Loading

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;
        }
    }
}

{
result = null;
if (string.IsNullOrEmpty(input))
Expand Down
32 changes: 19 additions & 13 deletions sdk/core/Azure.Core/tests/ResourceIdentifierTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,27 +816,33 @@ public void InvalidTenantID(string id)
Assert.IsFalse(ResourceIdentifier.TryParse(id, out var result));
}

[TestCase(null)]
public void NullInput(string invalidID)
{
Assert.Throws<ArgumentNullException>(() => { _ = new ResourceIdentifier(invalidID).Name; });
Assert.Throws<ArgumentNullException>(() => ResourceIdentifier.Parse(invalidID));
Assert.IsFalse(ResourceIdentifier.TryParse(invalidID, out var result));
}

[TestCase("")]
public void EmptyInput(string invalidID)
{
Assert.Throws<ArgumentException>(() => { _ = new ResourceIdentifier(invalidID).Name; });
Assert.Throws<ArgumentException>(() => ResourceIdentifier.Parse(invalidID));
Assert.IsFalse(ResourceIdentifier.TryParse(invalidID, out var result));
}

[TestCase(" ")]
[TestCase("asdfghj")]
[TestCase("123456")]
[TestCase("!@#$%^&*/")]
[TestCase("/subscriptions/")]
[TestCase("/0c2f6471-1bf0-4dda-aec3-cb9272f09575/myRg/")]
public void InvalidRPIds(string invalidID)
public void InvalidInput(string invalidID)
{
if (invalidID == String.Empty)
{
Assert.Throws<ArgumentException>(() => { _ = new ResourceIdentifier(invalidID).Name; });
Assert.Throws<ArgumentException>(() => ResourceIdentifier.Parse(invalidID));
Assert.IsFalse(ResourceIdentifier.TryParse(invalidID, out var result));
}
else
{
Assert.Throws<FormatException>(() => { _ = new ResourceIdentifier(invalidID).Name; });
Assert.Throws<FormatException>(() => ResourceIdentifier.Parse(invalidID));
Assert.IsFalse(ResourceIdentifier.TryParse(invalidID, out var result));
}
Assert.Throws<FormatException>(() => { _ = new ResourceIdentifier(invalidID).Name; });
Assert.Throws<FormatException>(() => ResourceIdentifier.Parse(invalidID));
Assert.IsFalse(ResourceIdentifier.TryParse(invalidID, out var result));
}

[TestCase(TrackedResourceId)]
Expand Down