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

Add missing type name parsing test coverage #98562

Merged
merged 9 commits into from Feb 22, 2024

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Feb 16, 2024

While working on the new Type Name Parser API (#97566) I've discovered some testing gaps.

Namely:

  1. Reporting parse error for a backslash that was used to escape a character that is not legal to escape inside a type name:
    if (!NeedsEscapingInTypeName(c))
    {
    // If we got here, a backslash was used to escape a character that is not legal to escape inside a type name.
    ParseError();
    return null;
    }
  2. Ignore leading '.' for type names without namespace:
    // Compat: Ignore leading '.' for type names without namespace. .NET Framework historically ignored leading '.' here. It is likely
    // that code out there depends on this behavior. For example, type names formed by concatenating namespace and name, without checking for
    // empty namespace (bug), are going to have superfluous leading '.'.
    // This behavior means that types that start with '.' are not round-trippable via type name.
    static string ApplyLeadingDotCompatQuirk(string typeName)
    {
    #if NETCOREAPP
    return (typeName.StartsWith('.') && !typeName.AsSpan(1).Contains('.')) ? typeName.Substring(1) : typeName;
  3. Supporting all kinds of leading whitespaces (existing test use only (space character)):
    // The type name parser has a strange attitude towards whitespace. It throws away whitespace between punctuation tokens and whitespace
    // preceding identifiers or assembly names (and this cannot be escaped away). But whitespace between the end of an identifier
    // and the punctuation that ends it is *not* ignored.
    //
    // In other words, GetType(" Foo") searches for "Foo" but GetType("Foo ") searches for "Foo ".
    //
    // Whitespace between the end of an assembly name and the punction mark that ends it is also not ignored by this parser,
    // but this is irrelevant since the assembly name is then turned over to AssemblyName for parsing, which *does* ignore trailing whitespace.
    //
    private void SkipWhiteSpace()
    {
    while (_index < _input.Length && char.IsWhiteSpace(_input[_index]))
  4. Type names with escape characters in their name (illegal for C# and F#, but valid for IL)
    private static ReadOnlySpan<char> CharsToEscape => "\\[]+*&,";
    private static bool NeedsEscapingInTypeName(char c)
    => CharsToEscape.Contains(c);

    cc @GrabYourPitchforks @jeffhandley

@ghost
Copy link

ghost commented Feb 16, 2024

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

Issue Details

While working on the new Type Name Parser API (#97566) I've discovered some testing gaps.

Namely:

  1. Reporting parse error for a backslash that was used to escape a character that is not legal to escape inside a type name:
    if (!NeedsEscapingInTypeName(c))
    {
    // If we got here, a backslash was used to escape a character that is not legal to escape inside a type name.
    ParseError();
    return null;
    }
  2. Ignore leading '.' for type names without namespace:
    // Compat: Ignore leading '.' for type names without namespace. .NET Framework historically ignored leading '.' here. It is likely
    // that code out there depends on this behavior. For example, type names formed by concatenating namespace and name, without checking for
    // empty namespace (bug), are going to have superfluous leading '.'.
    // This behavior means that types that start with '.' are not round-trippable via type name.
    static string ApplyLeadingDotCompatQuirk(string typeName)
    {
    #if NETCOREAPP
    return (typeName.StartsWith('.') && !typeName.AsSpan(1).Contains('.')) ? typeName.Substring(1) : typeName;
  3. Supporting all kinds of leading whitespaces:
    // The type name parser has a strange attitude towards whitespace. It throws away whitespace between punctuation tokens and whitespace
    // preceding identifiers or assembly names (and this cannot be escaped away). But whitespace between the end of an identifier
    // and the punctuation that ends it is *not* ignored.
    //
    // In other words, GetType(" Foo") searches for "Foo" but GetType("Foo ") searches for "Foo ".
    //
    // Whitespace between the end of an assembly name and the punction mark that ends it is also not ignored by this parser,
    // but this is irrelevant since the assembly name is then turned over to AssemblyName for parsing, which *does* ignore trailing whitespace.
    //
    private void SkipWhiteSpace()
    {
    while (_index < _input.Length && char.IsWhiteSpace(_input[_index]))
  4. Type names with escape characters in their name (illegal for C# and F#, but valid for IL(
    private static ReadOnlySpan<char> CharsToEscape => "\\[]+*&,";
    private static bool NeedsEscapingInTypeName(char c)
    => CharsToEscape.Contains(c);

    cc @GrabYourPitchforks @jeffhandley
Author: adamsitnik
Assignees: adamsitnik
Labels:

area-System.Reflection

Milestone: -

@jkotas
Copy link
Member

jkotas commented Feb 17, 2024

The tests are failing on Mono. Mono does not use the unified parser for all code paths yet - #45033 . I think you can disable the failing tests on Mono against this issue.

@jkotas
Copy link
Member

jkotas commented Feb 17, 2024

While you are on it, could you please also add a coverage for byref-of-byref and pointer-of-byref (e.g. here

public void GetTypeByName_Invalid(string typeName, Type expectedException, bool alwaysThrowsException)
)? We have been missing coverage for it - see #98426.

@adamsitnik adamsitnik force-pushed the typeLoadingEdgeCaseTestCoverage branch from 843023a to 8d9b537 Compare February 19, 2024 13:14
@adamsitnik
Copy link
Member Author

@jkotas I've addressed your feedback, PTAL. Thanks!

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Comment on lines 527 to 528
yield return new object[] { "System.Void[]", expectedException, true };
yield return new object[] { "System.TypedReference[]", expectedException, true };
Copy link
Member Author

Choose a reason for hiding this comment

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

@jkotas these two test cases are falling on Mono. Is my understanding correct that with #94835 they should be working fine?

sample log file

Suggested change
yield return new object[] { "System.Void[]", expectedException, true };
yield return new object[] { "System.TypedReference[]", expectedException, true };
if (!PlatformDetection.IsMonoRuntime)
{
yield return new object[] { "System.Void[]", expectedException, true };
yield return new object[] { "System.TypedReference[]", expectedException, true };
}

@adamsitnik adamsitnik merged commit 8ec001d into dotnet:main Feb 22, 2024
107 of 111 checks passed
@adamsitnik adamsitnik added the test-enhancement Improvements of test source code label Feb 22, 2024
@adamsitnik adamsitnik added this to the 9.0.0 milestone Feb 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants