From a584de18e8a46a954e4eafc45f227e33f93c7e47 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 20 Mar 2020 13:10:51 -0400 Subject: [PATCH] [Java.Interop.Tools.JavaCallableWrappers] Fix IsValidIdentifier() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/xamarin/xamarin-android/pull/4431 Commit e7c5f54a added `IdentifierValidator.IsValidIdentifier()`, replacing use of `CSharpCodeProvider.IsValidIdentifier()`, and this change caused a unit test failure within xamarin-android: // Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateEventName obj/Debug/generated/src/Com.Xamarin.Testing.Test.cs(350,29,350,32): error CS1001: Identifier expected The line in question? public event EventHandler 123 { Oops. It Turns Out™ that `IdentifierValidator.IsValidIdentifier("123")` returned True, so we attempted to create an event named `123`, which the C# compiler Does Not Like. Fix `IdentifierValidator.IsValidIdentifier()` by using a new `IsValidIdentifierRegex` regex to match against identifiers, instead of attempting to reuse the `validIdentifier` regex, which matches for *invalid* characters (it negates the match via `[^...]`), and thus cannot differentiate between "starting" characters and everything else, which is fine for `IdentifierValidator.CreateValidIdentifier()`, but not for `IdentifierValidator.IsValidIdentifier()`. Add unit tests for `IdentifierValidator.IsValidIdentifier()`. Update `make run-test-generator-core` tests so that we add a `View.setOn123Listener()` method, which prior to this commit would add a `View.123` event: partial class View { public event EventHandler 123 { add { … } remove { … } } } With a corrected `IdentifierValidator.IsValidIdentifier()`, this event is no longer generated. --- .../IdentifierValidator.cs | 4 ++- .../IdentifierValidatorTests.cs | 13 +++++++ tests/generator-Tests/Tests-Core/api.xml | 4 +++ .../expected.ji/Android.Views.View.cs | 30 ++++++++++++++++ .../Tests-Core/expected/Android.Views.View.cs | 36 +++++++++++++++++++ 5 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidator.cs b/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidator.cs index 8f0039889..ef5c09cb4 100644 --- a/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidator.cs +++ b/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidator.cs @@ -26,6 +26,8 @@ public static class IdentifierValidator private const string Identifier = IdentifierStartCharacter + "(" + IdentifierPartCharacter + ")"; + static Regex IsValidIdentifierRegex = new Regex ($"^[{IdentifierStartCharacter}][{IdentifierPartCharacter}]*$", RegexOptions.Compiled); + // We use [^ ...] to detect any character that is NOT a match. static Regex validIdentifier = new Regex ($"[^{Identifier}]", RegexOptions.Compiled); @@ -43,7 +45,7 @@ public static string CreateValidIdentifier (string identifier, bool useEncodedRe public static bool IsValidIdentifier (string identifier) { - return !validIdentifier.IsMatch (identifier); + return IsValidIdentifierRegex.IsMatch (identifier); } // Makes uglier but unique identifiers by encoding each invalid character with its character value diff --git a/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidatorTests.cs b/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidatorTests.cs index abb1593bb..772383627 100644 --- a/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidatorTests.cs +++ b/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers/IdentifierValidatorTests.cs @@ -24,5 +24,18 @@ public void CreateValidIdentifier_Encoded () Assert.AreEqual ("my_x45_identifier_x36_test", IdentifierValidator.CreateValidIdentifier ("my-identifier$test", true)); Assert.AreEqual ("myidentifier_x55357__x56842_test", IdentifierValidator.CreateValidIdentifier ("myidentifier😊test", true)); } + + [Test] + public void IsValidIdentifier () + { + Assert.IsTrue (IdentifierValidator.IsValidIdentifier ("name")); + Assert.IsTrue (IdentifierValidator.IsValidIdentifier ("Name_With_Underscores")); + + // Yes, this is "wrong" -- keywords aren't identifiers -- but the keyword check is done elsewhere. + Assert.IsTrue (IdentifierValidator.IsValidIdentifier ("true")); + + Assert.IsFalse (IdentifierValidator.IsValidIdentifier ("name-with-hyphens and spaces")); + Assert.IsFalse (IdentifierValidator.IsValidIdentifier ("123")); + } } } diff --git a/tests/generator-Tests/Tests-Core/api.xml b/tests/generator-Tests/Tests-Core/api.xml index b62bd74e3..b979cf7c7 100644 --- a/tests/generator-Tests/Tests-Core/api.xml +++ b/tests/generator-Tests/Tests-Core/api.xml @@ -6,6 +6,10 @@ + + + + diff --git a/tests/generator-Tests/Tests-Core/expected.ji/Android.Views.View.cs b/tests/generator-Tests/Tests-Core/expected.ji/Android.Views.View.cs index a51b106fa..7f8ce10c2 100644 --- a/tests/generator-Tests/Tests-Core/expected.ji/Android.Views.View.cs +++ b/tests/generator-Tests/Tests-Core/expected.ji/Android.Views.View.cs @@ -179,6 +179,36 @@ public virtual unsafe void SetOnClickListener (Android.Views.View.IOnClickListen } } + static Delegate cb_setOn123Listener_Landroid_view_View_OnClickListener_; +#pragma warning disable 0169 + static Delegate GetSetOn123Listener_Landroid_view_View_OnClickListener_Handler () + { + if (cb_setOn123Listener_Landroid_view_View_OnClickListener_ == null) + cb_setOn123Listener_Landroid_view_View_OnClickListener_ = JNINativeWrapper.CreateDelegate ((Action) n_SetOn123Listener_Landroid_view_View_OnClickListener_); + return cb_setOn123Listener_Landroid_view_View_OnClickListener_; + } + + static void n_SetOn123Listener_Landroid_view_View_OnClickListener_ (IntPtr jnienv, IntPtr native__this, IntPtr native_l) + { + Android.Views.View __this = global::Java.Lang.Object.GetObject (jnienv, native__this, JniHandleOwnership.DoNotTransfer); + Android.Views.View.IOnClickListener l = (Android.Views.View.IOnClickListener)global::Java.Lang.Object.GetObject (native_l, JniHandleOwnership.DoNotTransfer); + __this.SetOn123Listener (l); + } +#pragma warning restore 0169 + + // Metadata.xml XPath method reference: path="/api/package[@name='android.view']/class[@name='View']/method[@name='setOn123Listener' and count(parameter)=1 and parameter[1][@type='android.view.View.OnClickListener']]" + [Register ("setOn123Listener", "(Landroid/view/View$OnClickListener;)V", "GetSetOn123Listener_Landroid_view_View_OnClickListener_Handler")] + public virtual unsafe void SetOn123Listener (Android.Views.View.IOnClickListener l) + { + const string __id = "setOn123Listener.(Landroid/view/View$OnClickListener;)V"; + try { + JniArgumentValue* __args = stackalloc JniArgumentValue [1]; + __args [0] = new JniArgumentValue ((l == null) ? IntPtr.Zero : ((global::Java.Lang.Object) l).Handle); + _members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args); + } finally { + } + } + static Delegate cb_addTouchables_Ljava_util_ArrayList_; #pragma warning disable 0169 static Delegate GetAddTouchables_Ljava_util_ArrayList_Handler () diff --git a/tests/generator-Tests/Tests-Core/expected/Android.Views.View.cs b/tests/generator-Tests/Tests-Core/expected/Android.Views.View.cs index a7bfff021..4c3a00556 100644 --- a/tests/generator-Tests/Tests-Core/expected/Android.Views.View.cs +++ b/tests/generator-Tests/Tests-Core/expected/Android.Views.View.cs @@ -172,6 +172,42 @@ public virtual unsafe void SetOnClickListener (Android.Views.View.IOnClickListen } } + static Delegate cb_setOn123Listener_Landroid_view_View_OnClickListener_; +#pragma warning disable 0169 + static Delegate GetSetOn123Listener_Landroid_view_View_OnClickListener_Handler () + { + if (cb_setOn123Listener_Landroid_view_View_OnClickListener_ == null) + cb_setOn123Listener_Landroid_view_View_OnClickListener_ = JNINativeWrapper.CreateDelegate ((Action) n_SetOn123Listener_Landroid_view_View_OnClickListener_); + return cb_setOn123Listener_Landroid_view_View_OnClickListener_; + } + + static void n_SetOn123Listener_Landroid_view_View_OnClickListener_ (IntPtr jnienv, IntPtr native__this, IntPtr native_l) + { + Android.Views.View __this = global::Java.Lang.Object.GetObject (jnienv, native__this, JniHandleOwnership.DoNotTransfer); + Android.Views.View.IOnClickListener l = (Android.Views.View.IOnClickListener)global::Java.Lang.Object.GetObject (native_l, JniHandleOwnership.DoNotTransfer); + __this.SetOn123Listener (l); + } +#pragma warning restore 0169 + + static IntPtr id_setOn123Listener_Landroid_view_View_OnClickListener_; + // Metadata.xml XPath method reference: path="/api/package[@name='android.view']/class[@name='View']/method[@name='setOn123Listener' and count(parameter)=1 and parameter[1][@type='android.view.View.OnClickListener']]" + [Register ("setOn123Listener", "(Landroid/view/View$OnClickListener;)V", "GetSetOn123Listener_Landroid_view_View_OnClickListener_Handler")] + public virtual unsafe void SetOn123Listener (Android.Views.View.IOnClickListener l) + { + if (id_setOn123Listener_Landroid_view_View_OnClickListener_ == IntPtr.Zero) + id_setOn123Listener_Landroid_view_View_OnClickListener_ = JNIEnv.GetMethodID (class_ref, "setOn123Listener", "(Landroid/view/View$OnClickListener;)V"); + try { + JValue* __args = stackalloc JValue [1]; + __args [0] = new JValue (l); + + if (((object) this).GetType () == ThresholdType) + JNIEnv.CallVoidMethod (((global::Java.Lang.Object) this).Handle, id_setOn123Listener_Landroid_view_View_OnClickListener_, __args); + else + JNIEnv.CallNonvirtualVoidMethod (((global::Java.Lang.Object) this).Handle, ThresholdClass, JNIEnv.GetMethodID (ThresholdClass, "setOn123Listener", "(Landroid/view/View$OnClickListener;)V"), __args); + } finally { + } + } + static Delegate cb_addTouchables_Ljava_util_ArrayList_; #pragma warning disable 0169 static Delegate GetAddTouchables_Ljava_util_ArrayList_Handler ()