From 9be55e2bf4ff52a187fa8d312b2472f7cb381780 Mon Sep 17 00:00:00 2001 From: Vincent Dondain Date: Thu, 29 Nov 2018 11:33:37 -0500 Subject: [PATCH] [xtro] Report incorrect 'ArgumentSemantic' enum value usage (#5114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ⚠️ Rule deactivated until we have an `xcode10.2` branch where we'll fix the issues. - We only report `copy` mistakes since they're the ones we really care about fixing (we can end up with invalid pointers). - Fixes #4018: [xtro] Report incorrect 'ArgumentSemantic' enum value usage (https://github.com/xamarin/xamarin-macios/issues/4018). - Performance with the added test: - Extrospection.SelectorCheck: Elapsed=00:00:00.7263742 - Extrospection.SelectorCheck: Elapsed=00:00:01.2136911 - Extrospection.SelectorCheck: Elapsed=00:00:01.2747602 - Extrospection.SelectorCheck: Elapsed=00:00:01.7494063 --- tests/xtro-sharpie/Helpers.cs | 38 ++++++++++++++++ tests/xtro-sharpie/SelectorCheck.cs | 69 ++++++++++++++++------------- 2 files changed, 76 insertions(+), 31 deletions(-) diff --git a/tests/xtro-sharpie/Helpers.cs b/tests/xtro-sharpie/Helpers.cs index 70e33a8062..4548745271 100644 --- a/tests/xtro-sharpie/Helpers.cs +++ b/tests/xtro-sharpie/Helpers.cs @@ -397,5 +397,43 @@ namespace Extrospection { else return (o1, o2); } + + public enum ArgumentSemantic { + None = -1, + Assign = 0, + Copy = 1, + Retain = 2, + Weak = 3, + Strong = Retain, + UnsafeUnretained = Assign, + } + + public static ArgumentSemantic ToArgumentSemantic (this ObjCPropertyAttributeKind attr) + { + if ((attr & ObjCPropertyAttributeKind.Retain) != 0) + return ArgumentSemantic.Retain; + else if ((attr & ObjCPropertyAttributeKind.Copy) != 0) + return ArgumentSemantic.Copy; + else if ((attr & ObjCPropertyAttributeKind.Assign) != 0) + return ArgumentSemantic.Assign; + else if ((attr & ObjCPropertyAttributeKind.Weak) != 0) + return ArgumentSemantic.Weak; + else if ((attr & ObjCPropertyAttributeKind.Strong) != 0) + return ArgumentSemantic.Strong; + else if ((attr & ObjCPropertyAttributeKind.UnsafeUnretained) != 0) + return ArgumentSemantic.UnsafeUnretained; + else + return ArgumentSemantic.Assign; // Default + } + + public static string ToUsableString (this ArgumentSemantic argSem) + { + if (argSem == ArgumentSemantic.Retain) + return "Strong|Retain"; + if (argSem == ArgumentSemantic.Assign) + return "UnsafeUnretained|Assign"; + + return argSem.ToString (); + } } } \ No newline at end of file diff --git a/tests/xtro-sharpie/SelectorCheck.cs b/tests/xtro-sharpie/SelectorCheck.cs index b4c080c896..d3b5323fff 100644 --- a/tests/xtro-sharpie/SelectorCheck.cs +++ b/tests/xtro-sharpie/SelectorCheck.cs @@ -4,9 +4,6 @@ // !missing-selector! // if headers defines a selector for which we have no bindings // -// !unknown-selector! -// if we have a selector that is not part of the header files -// using System; using System.Collections.Generic; @@ -18,24 +15,9 @@ using Clang.Ast; namespace Extrospection { public class SelectorCheck : BaseVisitor { -// Dictionary> exports = new Dictionary> (); - - // missing - // -> it's not in the type or ancestor or their interface (protocols) - // -> it's not in a category - - // unknown - // -> quick check (HashSet) to see if it's used anywhere - // -> - - // duplicate - // -> the selector is defined more than once for the same type - - HashSet known_selectors = new HashSet (); HashSet qualified_selectors = new HashSet (); - - //Dictionary> type_exports = new Dictionary> (); + Dictionary qualified_properties = new Dictionary (); // most selectors will be found in [Export] attribtues public override void VisitManagedMethod (MethodDefinition method) @@ -51,24 +33,49 @@ namespace Extrospection { foreach (var ca in method.CustomAttributes) { switch (ca.Constructor.DeclaringType.Name) { case "ExportAttribute": - string selector = ca.ConstructorArguments [0].Value as string; - if (!known_selectors.Contains (selector)) - known_selectors.Add (selector); + var methodDefinition = method.GetName (); + if (!string.IsNullOrEmpty (methodDefinition)) { + var argumentSemantic = Helpers.ArgumentSemantic.Assign; // Default + if (ca.ConstructorArguments.Count > 1) { + argumentSemantic = (Helpers.ArgumentSemantic)ca.ConstructorArguments [1].Value; + qualified_properties.Add (methodDefinition, argumentSemantic); + } + + qualified_selectors.Add (methodDefinition); + } - qualified_selectors.Add (method.GetName ()); -// -// TypeDefinition type = method.DeclaringType; -// HashSet list; -// if (!type_exports.TryGetValue (type, out list)) { -// list = new HashSet (); -// type_exports.Add (type, list); -// } -// list.Add (selector); break; } } } + public override void VisitObjCPropertyDecl (ObjCPropertyDecl decl) + { + // protocol members are checked in ObjCProtocolCheck + if (decl.DeclContext is ObjCProtocolDecl) + return; + + // check availability macros to see if the API is available on the OS and not deprecated + if (!decl.IsAvailable ()) + return; + + var framework = Helpers.GetFramework (decl); + if (framework == null) + return; + + var nativeArgumentSemantic = decl.Attributes.ToArgumentSemantic (); + var nativeMethodDefinition = decl.QualifiedName; + + bool found = qualified_properties.TryGetValue (nativeMethodDefinition, out var managedArgumentSemantic); + if (found && managedArgumentSemantic != nativeArgumentSemantic) { + // FIXME: only Copy mistakes are reported now + if (managedArgumentSemantic == Helpers.ArgumentSemantic.Copy || nativeArgumentSemantic == Helpers.ArgumentSemantic.Copy) { + // FIXME: rule disactivated for now + //Log.On (framework).Add ($"!incorrect-argument-semantic! Native '{nativeMethodDefinition}' is declared as ({nativeArgumentSemantic.ToUsableString ().ToLowerInvariant ()}) but mapped to 'ArgumentSemantic.{managedArgumentSemantic.ToUsableString ()}'"); + } + } + } + public override void VisitObjCMethodDecl (ObjCMethodDecl decl, VisitKind visitKind) { if (visitKind != VisitKind.Enter)