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

ManagedNameUtilities does not support spaces and other special characters in method names #2733

Closed
Haplois opened this issue Feb 4, 2021 · 12 comments · Fixed by #2738
Closed
Assignees

Comments

@Haplois
Copy link
Contributor

Haplois commented Feb 4, 2021

Description

We're switching to the Microsoft.TestPlatform.AdapterUtilities package to ensure every test method has a unique name. (See https://github.com/microsoft/vstest-docs/blob/master/RFCs/0017-Managed-TestCase-Properties.md) In our current implementation, we won't allow spaces in method names; F# does. Do we need to support it?

It looks like CIL supports all kinds of strings in type names. (https://www.ecma-international.org/wp-content/uploads/ECMA-335_5th_edition_december_2010.pdf, page 135, 5.3 Identifiers)

Should we support all cases CIL supports? TestPlatfrom currently supports spaces and dots in the method name; if we omit this support, it'll be a breaking change.

Steps to reproduce

Compile

namespace Library2

type Type1() = 
    member this.``Type name with ` . ' and !@#$^%^&*()-_~[]{}\|``<'T, 'X, '``mad -.- :O ness``>(a:'T, b:'X, c:'``mad -.- :O ness``) = "F#"

Call

var madMethod = typeof(Library2.Type1).GetMethods()[0];

ManagedNameHelper.GetManagedName(madMethod, out var typeName, out var methodName);
ManagedNameHelper.GetMethod(typeof(Library2.Type1).Assembly, typeName, methodName);

Expected behavior

It should return the correct MethodBase.

Actual behavior

Microsoft.TestPlatform.AdapterUtilities.ManagedNameUtilities.InvalidManagedNameException: 'Whitespace is not valid in a ManagedName (pos: 4)'

AB#1274433

@peterwald
Copy link
Member

Should we support all cases CIL supports? TestPlatfrom currently supports spaces and dots in the method name; if we omit this support, it'll be a breaking change.

Yes, I agree we should add this support.

@Haplois
Copy link
Contributor Author

Haplois commented Feb 9, 2021

Some escaped examples are below:

IL:

.method static public int32 '𝑓'(int32 '𝓧', int32 '𝓨') cil managed

         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: 𝑓
  Parsed Type: CleanNamespaceName.SecondLevel.'\ud835\udc0c\ud835\udc32\u0020\ud835\uddee\ud835\ude04\ud835\uddf2\ud835\ude00\ud835\uddfc\ud835\uddfa\ud835\uddf2\u0020\ud835\ude24\ud835\ude2d\ud835\ude22\ud835\ude34\ud835\ude34\u0020\ud835\ude22\ud835\ude2f\u0020\ud835\udc8a\ud835\udc8f\ud835\udc82\ud835\udc84\ud835\udc84\ud835\udc86\ud835\udc94\ud835\udc94\ud835\udc8a\ud835\udc83\ud835\udc8d\ud835\udc86\u0020\ud835\ude63\ud835\ude56\ud835\ude62\ud835\ude5a\u0020\ud83d\ude2d'
Parsed Method: '\ud835\udc53'(System.Int32,System.Int32)

IL:

.method static public int32 Sum(int32 '𝓧', int32 '𝓨') cil managed

         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: Sum
  Parsed Type: CleanNamespaceName.SecondLevel.'\ud835\udc0c\ud835\udc32\u0020\ud835\uddee\ud835\ude04\ud835\uddf2\ud835\ude00\ud835\uddfc\ud835\uddfa\ud835\uddf2\u0020\ud835\ude24\ud835\ude2d\ud835\ude22\ud835\ude34\ud835\ude34\u0020\ud835\ude22\ud835\ude2f\u0020\ud835\udc8a\ud835\udc8f\ud835\udc82\ud835\udc84\ud835\udc84\ud835\udc86\ud835\udc94\ud835\udc94\ud835\udc8a\ud835\udc83\ud835\udc8d\ud835\udc86\u0020\ud835\ude63\ud835\ude56\ud835\ude62\ud835\ude5a\u0020\ud83d\ude2d'
Parsed Method: Sum(System.Int32,System.Int32)

IL:

.method static public int32 'Method with . in it'(int32 '𝓧', int32 '𝓨') cil managed

         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: Method with . in it
  Parsed Type: CleanNamespaceName.SecondLevel.'\ud835\udc0c\ud835\udc32\u0020\ud835\uddee\ud835\ude04\ud835\uddf2\ud835\ude00\ud835\uddfc\ud835\uddfa\ud835\uddf2\u0020\ud835\ude24\ud835\ude2d\ud835\ude22\ud835\ude34\ud835\ude34\u0020\ud835\ude22\ud835\ude2f\u0020\ud835\udc8a\ud835\udc8f\ud835\udc82\ud835\udc84\ud835\udc84\ud835\udc86\ud835\udc94\ud835\udc94\ud835\udc8a\ud835\udc83\ud835\udc8d\ud835\udc86\u0020\ud835\ude63\ud835\ude56\ud835\ude62\ud835\ude5a\u0020\ud83d\ude2d'
Parsed Method: 'Method\u0020with\u0020\u002e\u0020in\u0020it'(System.Int32,System.Int32)

IL:

.method public instance int32 'MethodName`1(System.Int32,System.Int32,System.Int32)'()

         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`1(System.Int32,System.Int32,System.Int32)
  Parsed Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'
Parsed Method: 'MethodName\u00601\u0028System\u002eInt32\u002cSystem\u002eInt32\u002cSystem\u002eInt32\u0029'

IL:

.method public instance int32 'MethodName`2(System.Int32,System.Int32,System.Int32)'(int32 '1st parameter', int32 '2nd parameter')

         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`2(System.Int32,System.Int32,System.Int32)
  Parsed Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'
Parsed Method: 'MethodName\u00602\u0028System\u002eInt32\u002cSystem\u002eInt32\u002cSystem\u002eInt32\u0029'(System.Int32,System.Int32)

IL:

.method public instance int32 'MethodName`3'(int32 '1st parameter', int32 '2nd parameter')

         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`3
  Parsed Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'
Parsed Method: 'MethodName\u00603'(System.Int32,System.Int32)

IL:

.method public instance int32 'MethodName`3'(int32 '1st parameter', int32 '2nd parameter')

         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2+Inner Class
       Method: MethodName`3
  Parsed Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'+'Inner\u0020Class'
Parsed Method: 'MethodName\u00603'(System.Int32,System.Int32)

IL:

.method public instance int32 'MethodName`2(System.Int32,System.Int32,System.Int32)'()

         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`2(System.Int32,System.Int32,System.Int32)
  Parsed Type: NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'
Parsed Method: 'MethodName\u00602\u0028System\u002eInt32\u002cSystem\u002eInt32\u002cSystem\u002eInt32\u0029'

IL:

.method public instance int32 'MethodName`3'(int32 '1st parameter', int32 '2nd parameter')

         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`3
  Parsed Type: NamespaceA.NamespaceB.'ClassName\u00601\u005c\u002bInnerClass\u00602'
Parsed Method: 'MethodName\u00603'(System.Int32,System.Int32)

@shyamnamboodiripad
Copy link
Contributor

@peterwald @Haplois Note that we will need to implement identical support on the Roslyn side. I am not sure if Roslyn will create symbols for types / methods with such names - but we should at least ensure that the code fails in a graceful / predictable way.

@shyamnamboodiripad
Copy link
Contributor

shyamnamboodiripad commented Feb 9, 2021

The choices above seem reasonable although I have a few questions -

  • How do we determine that a particular character should be escaped? Does any non-ascii character get escaped with \u***? I imagine its something more since white space and dot characters are ascii and looks like they are still escaped above...
  • Do we need to escape regular white space characters too (considering that we already add ' around escaped names)? Conversely, considering that escaped names can never contain white space above, is the ' around escaped names required?
  • Why are( and ) (As well as dots, spaces and commas in parameters) escaped for some Parsed Method names (such as 'MethodName\u00602\u0028System\u002eInt32\u002cSystem\u002eInt32\u002cSystem\u002eInt32\u0029') above and not escaped for other names (such as 'MethodName\u00603'(System.Int32,System.Int32))?

@shyamnamboodiripad
Copy link
Contributor

@Haplois Could you please share a link to the IL documentation around escaping upon which the proposed escaping format above is based?

@Haplois
Copy link
Contributor Author

Haplois commented Feb 9, 2021

  • Why are( and ) (As well as dots, spaces and commas in parameters) escaped for some Parsed Method names (such as 'MethodName\u00602\u0028System\u002eInt32\u002cSystem\u002eInt32\u002cSystem\u002eInt32\u0029')

In this case, (, ) and , are part of the method name, and for the other one, the method name is MethodName`3, and the rest is the parameters for that method.

@Haplois
Copy link
Contributor Author

Haplois commented Feb 9, 2021

  • How do we determine that a particular character should be escaped? Does any non-ascii character get escaped with \u***? I imagine its something more since white space and dot characters are ascii and looks like they are still escaped above...

We don't escape a char if

  • it's a digit and not the first char
  • a letter
  • or belong to one of these Unicode categories:
    • NonSpacingMark (Mn)
    • SpacingCombiningMark (Mc)
    • ConnectorPunctuation (Pc)
    • Format (Cf)

It's the same as C# standard (https://www.ecma-international.org/wp-content/uploads/ECMA-334_5th_edition_december_2017.pdf, page 41, 7.4.3 Identifiers)

@Haplois
Copy link
Contributor Author

Haplois commented Feb 9, 2021

  • Do we need to escape regular white space characters too (considering that we already add ' around escaped names)? Conversely, considering that escaped names can never contain white space above, is the ' around escaped names required?

It's easier to parse if we use '. (We can stop encoding whitespace)

@shyamnamboodiripad
Copy link
Contributor

It's easier to parse if we use '. (We can stop encoding whitespace)
Sounds good so long as we only do one of the two

@peterwald
Copy link
Member

peterwald commented Feb 9, 2021

I like the idea of matching the encoding of C#. How would we escape ' in the type or method name since it's being used to delimit the name? Would we just use the unicode escaped encoding for it?

@Haplois
Copy link
Contributor Author

Haplois commented Feb 9, 2021

I like the idea of matching the encoding of C#. How would we escape ' in the type or method name since it's being used to delimit the name? Would we just use the unicode escaped encoding for it?

We're escaping that as \u0027.

@Haplois
Copy link
Contributor Author

Haplois commented Feb 15, 2021

I made some changes to our escaping. With the new changes, if a string segment is not a valid c# identifier I put it inside single quotes, and only escape ' and \ chars (to \' and \\ respectively). So with my new change, the output is a bit cleaner. Some samples below:

         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: 𝑓
  Parsed Type: CleanNamespaceName.SecondLevel.'𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭'
Parsed Method: '𝑓'(System.Int32,System.Int32)
         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: Sum
  Parsed Type: CleanNamespaceName.SecondLevel.'𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭'
Parsed Method: Sum(System.Int32,System.Int32)
         Type: CleanNamespaceName.SecondLevel.𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭
       Method: Method with . in it
  Parsed Type: CleanNamespaceName.SecondLevel.'𝐌𝐲 𝗮𝘄𝗲𝘀𝗼𝗺𝗲 𝘤𝘭𝘢𝘴𝘴 𝘢𝘯 𝒊𝒏𝒂𝒄𝒄𝒆𝒔𝒔𝒊𝒃𝒍𝒆 𝙣𝙖𝙢𝙚 😭'
Parsed Method: 'Method with . in it'(System.Int32,System.Int32)
         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.My generic class
       Method: Method with . in it
  Parsed Type: CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.'My generic class'
Parsed Method: 'Method with . in it'`1(!0,System.Int32)
         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`1(System.Int32,System.Int32,System.Int32)
  Parsed Type: CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: 'MethodName`1(System.Int32,System.Int32,System.Int32)'
         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`2(System.Int32,System.Int32,System.Int32)
  Parsed Type: CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: 'MethodName`2(System.Int32,System.Int32,System.Int32)'(System.Int32,System.Int32)
         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`3
  Parsed Type: CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: 'MethodName`3'(System.Int32,System.Int32)
         Type: CleanNamespaceName.SecondLevel.Deeply wrong .namespace name.NamespaceA.NamespaceB.ClassName`1\+InnerClass`2+Inner Class
       Method: MethodName`3
  Parsed Type: CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'+'Inner Class'
Parsed Method: 'MethodName`3'(System.Int32,System.Int32)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`2(System.Int32,System.Int32,System.Int32)
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: 'MethodName`2(System.Int32,System.Int32,System.Int32)'
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: MethodName`3
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: 'MethodName`3'(System.Int32,System.Int32)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Int32)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Int32,NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2')
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Int32*)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Object)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(!!0)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`2
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(!!0[])
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(!!0[][])
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(!!0[,])
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(!!0[,,])
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(System.Collections.Generic.List`1<System.Int32>)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(System.Collections.Generic.List`1<!!0>)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`2(System.Tuple`2<!!0,!!1>,System.Tuple`2<!!1,!!0>)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Tuple`1<System.Tuple`2<System.String[,],System.Int32>>)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0(System.Tuple`2<System.Tuple`1<System.String>,System.Tuple`1<System.Int32>>)
         Type: NamespaceA.NamespaceB.ClassName`1\+InnerClass`2
       Method: Overload0
  Parsed Type: NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'
Parsed Method: Overload0`1(System.Tuple`1<System.Tuple`1<CleanNamespaceName.SecondLevel.'Deeply wrong '.'namespace name'.NamespaceA.NamespaceB.'ClassName`1\\+InnerClass`2'>>)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants