Skip to content

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

@adamsitnik adamsitnik requested a review from jkotas February 16, 2024 16:08
@ghost ghost assigned adamsitnik Feb 16, 2024
@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

@adamsitnik adamsitnik Feb 20, 2024

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
@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.

2 participants