[generator] Escape hyphens for callback names so they're valid C#. #497
Conversation
| @@ -105,7 +105,7 @@ internal string CalculateEventName (Func<string, bool> checkNameDuplicate) | |||
|
|
|||
| public string ConnectorName => $"Get{Name}{IDSignature}Handler"; | |||
|
|
|||
| public string EscapedCallbackName => $"cb_{JavaName}{IDSignature}"; | |||
| public string EscapedCallbackName => $"cb_{JavaName}{IDSignature}".Replace ("-", "_x45_").Replace ("$", "_x36_"); | |||
|
|
|||
| public string EscapedIdName => "id_" + JavaName.Replace ("<", "_x60_").Replace (">", "_x62_") + IDSignature; | |||
There was a problem hiding this comment.
I see in this instance you only replace the JavaName and not the signature. Does it matter?
There was a problem hiding this comment.
I'm not sure I understand?
There was a problem hiding this comment.
This line is JavaName.Replace ("<", "_x60_").Replace (">", "_x62_")
and then new one is "cb_{JavaName}{IDSignature}".Replace (...
There was a problem hiding this comment.
I wonder if this is relevant: #498 (comment)
Have a "single" "ensure the string is a valid C# identifier" method and use that consistently ~everywhere.
There was a problem hiding this comment.
I'll be merging this PR, but on the whole I believe @mattleibow is correct, and EscapedIdName should likewise use IdentifierValidator.CreateValidIdentifier() as well.
However, AFAICT EscapedIdName is only used for the legacy JNIEnv-based output format, not the newer XAJavaInterop1 type, so I'm not entirely concerned about this. We should prefer XAJavaInterop.
|
Just noticed that my current code is in an abstract class, but are not actually abstract themselves. Are any changes needed for the invoker if they were? |
| // We use [^ ...] to detect any character that is NOT a match. | ||
| static Regex validIdentifier = new Regex ($"[^{Identifier}]", RegexOptions.Compiled); | ||
|
|
||
| public static string CreateValidIdentifier (string identifier, bool useEncodedReplacements = false) |
There was a problem hiding this comment.
Could we move this into Java.Interop.Tools.JavaCallableWrappers or some other library referenced by generator.csproj and also referenced by Xamarin.Android.Build.Tasks.dll? That way a future xamarin-android PR could unify onto this method, instead of us having two "separate" methods which do the same thing.
Having the code in generator.exe slightly complicates that.
| private const string LetterCharacter = @"\p{Lu}\p{Ll}\p{Lt}\p{Lm}\p{Lo}\p{Nl}"; | ||
|
|
||
| private const string IdentifierPartCharacter = LetterCharacter + | ||
| DecimalDigitCharacter + |
There was a problem hiding this comment.
This code is using spaces instead of tabs for indentation. Please fix. :-)
(Which is odd, as other methods in this file are using tabs...)
…lidating identifier names.
no whitespace changes!
) Context: #496 Kotlin possibly inserts non-Java characters into method names. For example, *spaces* may be used. (This apparently doesn't work on Android at the moment, but it is a [documented convention][0] for use in unit tests!) class MyTestCase { @test fun `ensure everything works`() { /* ... */ } } Which is to say, Kotlin Is Not Java™. Unfortunately, most of `generator` was coded with Java and C# in mind, so it will unfortunately fail when it encounters "unexpected" characters such as spaces (above) and hyphens (`-`)[^0]. To help address this, add a new `Java.Interop.Tools.JavaCallableWrappers.IdentifierValidator.CreateValidIdentifier()` method -- [cribbed from xamarin-android][1] -- which translates all characters which cannot appear in a C# identifier and replaces them with either `_` *or* `_x@DEC@_`, where `@DEC@` is the decimal value of the character that C# doesn't support. For example: Assert.AreEqual ("my_identifier_test", IdentifierValidator.CreateValidIdentifier ("my-identifier$test")); Assert.AreEqual ("my_x45_identifier_x36_test", IdentifierValidator.CreateValidIdentifier ("my-identifier$test", true)); Next, update `MonoDroid.Generation.Method.EscapedCallbackName` to use `IdentifierValidator.CreateValidIdentifier()`, so that the emitted `cb_{JavaName}{IDSignature}` field remains a valid C# identifier, no matter what Kotlin decides to throw at it. [^0]: ***WHY?!*** [1]: dotnet/android@af61ecd
Context: #496.