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

[xtro] Report missing-protocol-conformance when protocols are defined… #3187

Merged
merged 18 commits into from
Jan 18, 2018

Conversation

VincentDondain
Copy link
Contributor

… in categories

… in categories

- Fixes bug #59272: [xtro] Report !missing-protocol-conformance! when protocols are defined in categories
(https://bugzilla.xamarin.com/show_bug.cgi?id=59272)
- Implemented missing protocol conformances based on tool's new data.
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ Nice!

@monojenkins
Copy link
Collaborator

Build failure

@VincentDondain
Copy link
Contributor Author

Damn I thought I fixed: [FAIL] Selector not found for MapKit.MKMapItem : encodeWithCoder:. Investigating.

Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -3542,7 +3551,7 @@ interface NSDate : NSSecureCoding, NSCopying {
}

[BaseType (typeof (NSObject))]
interface NSDictionary : NSSecureCoding, NSMutableCopying {
interface NSDictionary : NSSecureCoding, NSMutableCopying, NSFetchRequestResult, INSFastEnumeration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not have correct support for NSFastEnumeration
it better remains in the .ignore files (like others) until we do (or we'll miss this case when we add support)

Copy link
Contributor Author

@VincentDondain VincentDondain Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I saw that: https://github.com/xamarin/xamarin-macios/blob/master/src/foundation.cs#L13034-L13043

But INSFastEnumeration has a message saying: "Placeholer, just so we can start flagging things" and NSArray is already using INSFastEnumeration so I figured it was valid to add it to NSDictionary.

I already added it to the .ignore file: https://github.com/xamarin/xamarin-macios/pull/3187/files#diff-e909b6d483a4c2c9cbff0a2076e1406aR889

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure your first link is related - bad copy/paste ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please add INSFastEnumeration when applicable, that way we'll automatically get it when it's implemented.

src/webkit.cs Outdated
partial interface WebView {
partial interface WebView
#if MONOMAC
: NSUserInterfaceValidations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: indent

src/wkwebkit.cs Outdated
#if MONOMAC
: NSUserInterfaceValidations
#endif
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: indent

@@ -184,6 +184,9 @@ protected virtual bool Skip (Type type, string protocolName)
return true;
}
break;
// Undocumented conformance (members were inlinded in 'UIViewController' before so all subtypes should conform)
case "UIStateRestoring":
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the condition seems a bit too general, i.e. it could hide future failures
if this was inlined (typo above) in UIViewController then it's the (only) case where it should be skipped

Copy link
Contributor Author

@VincentDondain VincentDondain Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But skipping UIViewController doesn't skip all the subtypes and if I manually add all the subtypes (20 or 30) it'll fail whenever we add a new subtype (hence the super general condition :/ )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have Type type so you should be able to check for subclasses - which, I agree, would be better than adding 20-30 more entries

return;

if (categoryName.EndsWith ("Internal", StringComparison.Ordinal))
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's likely better in the data files - along with a comment
otherwise a valid type, ending with Internal, could be missed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I thought they were all removed

@@ -0,0 +1,2 @@
## Getting "[FAIL] Selector not found for Foundation.NSUrl : previewItemTitle" when NSUrl conforms to 'QLPreviewItem'
!missing-protocol-conformance! NSURL should conform to QLPreviewItem (defined in 'QLPreviewConvenienceAdditions' category)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, I had never realized some types were duplicated between quicklook[UI].cs files


[Export ("flush")]
void Flush ();
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it's a conflict with the protocol name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflict it is (:

void RequestMediaDataWhenReadyOnQueue (DispatchQueue queue, Action enqueuer);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same ;)

interface GKLocalPlayer
#if !TVOS && !WATCH
: GKSavedGameListener
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's something that should be fixed

@@ -1,2 +1,5 @@
## fixed for XAMCORE_4_0
!incorrect-protocol-member! QLPreviewItem::previewItemTitle is OPTIONAL and should NOT be abstract

## Getting "[FAIL] Selector not found for Foundation.NSUrl : previewItemTitle" when NSUrl conforms to 'QLPreviewItem'
!missing-protocol-conformance! NSURL should conform to QLPreviewItem (defined in 'QLPreviewConvenienceAdditions' category)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably because of the entry just above - if it's optional then having it not responding is normal

@spouliot
Copy link
Contributor

spouliot commented Jan 10, 2018

Something looks 🐟 with NSString in API diff, several properties/methods are being added as virtual but they should already exists in NSObject (so at best it would be override).

edit / update

interface CNKeyDescriptor : NSObjectProtocol, NSSecureCoding, NSCopying

it's likely because of NSObjectProtocol which is the name (like swift) used for the NSObject protocol (not type). Not clear why the generator is re-generating them (as non-override)

edit / update 2

Yup, that's the problem and the generated code is incorrect. @VincentDondain please do a diff of the generated code (like if this was a generator change) to see if it's the only place this happens. If that's the case we could ignore that specific case (until fixed) and move along with the PR.

edit / update 3

Yup^2

public new virtual string DebugDescription {

is what's produced (so compiling does not complain) but it's not really what we want - even if it should work (but duplicate the code) in most cases.

@@ -3542,7 +3551,7 @@ interface NSDate : NSSecureCoding, NSCopying {
}

[BaseType (typeof (NSObject))]
interface NSDictionary : NSSecureCoding, NSMutableCopying {
interface NSDictionary : NSSecureCoding, NSMutableCopying, NSFetchRequestResult, INSFastEnumeration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please add INSFastEnumeration when applicable, that way we'll automatically get it when it's implemented.

@VincentDondain
Copy link
Contributor Author

VincentDondain commented Jan 10, 2018

@spouliot here's the diff from the generator. Everything other than NSString.g.cs looks fine to me (expected based on the code changes).

src/mapkit.cs Outdated
, NSItemProviderReading, NSItemProviderWriting
: NSItemProviderReading, NSItemProviderWriting
#if ARCH_64 // [FAIL] Selector not found for MapKit.MKMapItem : encodeWithCoder: on 32-bit iOS
, NSSecureCoding
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that in public API, e.g. if 32bits and 64bits signatures are different then this leaks into user code
It also becomes impossible (or at least harder) to de-duplicate the platform assemblies (for fat apps)

This needs to be considered a OS version change (which it is) e.g. when a new version of iOS adds conformance then we add the API (and fix up introspection not to report it for older versions)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that it's actually tied to the OS version (it works in iOS 11 (64-bit only), but not iOS 10.3 (where we run our 32-bit tests))

Copy link
Contributor Author

@VincentDondain VincentDondain Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explains why I couldn't find many ARCH_64, thought I was being clever here :P

But yes I now remember how we usually handle NSSecureCoding, I was distracted.

src/mapkit.cs Outdated
#endif
#endif
#if WATCH || TVOS || MONOMAC && ARCH_64
: NSSecureCoding
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -9,3 +9,6 @@
!missing-field! MKAnnotationCalloutInfoDidChangeNotification not bound
!missing-protocol-conformance! MKUserLocation should conform to MKAnnotation
!missing-protocol-member! MKOverlay::coordinate not found

## Getting "[FAIL] Selector not found for MapKit.MKMapItem : encodeWithCoder:" when 'MKMapItem' implements 'NSSecureCoding' on 32-bit iOS
!missing-protocol-conformance! MKMapItem should conform to NSSecureCoding (defined in 'MKMapItemSerialization' category)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the previous case, it must be handled like a version change (not an arch specific change)
keep it mind xtro uses the 64bits .pch (from sharpie) to applies the rules

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I reviewed the individual commits and some comments are not showing, see the previous in 0f00cb1

@@ -6,5 +6,5 @@
!unknown-native-enum! MKFeatureVisibility bound
!unknown-native-enum! MKUserTrackingMode bound

## Getting "[FAIL] Selector not found for MapKit.MKMapItem : encodeWithCoder:" when 'MKMapItem' implements 'NSSecureCoding' on macOS
## Getting "[FAIL] Selector not found for MapKit.MKMapItem : encodeWithCoder:" when 'MKMapItem' implements 'NSSecureCoding' on 32-bit macOS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same (it's also using 64bits .pch for macOS)


// Undocumented conformance (members were inlinded in 'UIViewController' before so all subtypes should conform)
case "UIStateRestoring":
return type.Name == "UIViewController" || type.IsSubclassOf (typeof (UIViewController));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@spouliot
Copy link
Contributor

@VincentDondain there's a small problem with https://gist.github.com/VincentDondain/36cd2ca79628bb4b07a5145e46f0150c
and it does show why (at least some) delegates where not inlined.

AVQueuedSampleBufferRendering (iOS 11) was added after AVSampleBufferDisplayLayer (iOS 8) so members, like Enqueue are now shown to require iOS11 (which is not true). Same for AVSampleBufferDisplayLayer

UIViewController also has a similar issue with DecodeRestorableState and EncodeRestorableState

There are also an extra (generated, the one I mentioned before were after the final compilation) new https://gist.github.com/VincentDondain/36cd2ca79628bb4b07a5145e46f0150c#file-diff-L228 https://gist.github.com/VincentDondain/36cd2ca79628bb4b07a5145e46f0150c#file-diff-L883

note: I only looked at iOS (not others platforms) so there might be a few more.

We might need to split the commit (a few things, like availability, likely requires a generator changes) to get merge most of it soonish

@monojenkins
Copy link
Collaborator

Build failure

@VincentDondain
Copy link
Contributor Author

@spouliot not sure what's "breaking" when adding a new interface to an existing protocol (the new protocol I'm adding is empty on tvOS). It's not like I'm adding a member (doc).

See https://jenkins.mono-project.com/job/xamarin-macios-pr-builder/5929/API_diff

Also no idea why MetalPerformanceShaders breaking changes are raised (does not happen locally).

@spouliot
Copy link
Contributor

@VincentDondain sorry, too many comments :) I need context for not sure what's "breaking" when adding a new interface to an existing protocol (the new protocol I'm adding is empty on tvOS

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@VincentDondain
Copy link
Contributor Author

@spouliot about the breaking change that shows up on the tvOS API diff (link above).

Namespace GameKit

Type Changed: GameKit.IGKLocalPlayerListener

Added interface:

	IGKSavedGameListener

GKLocalPlayerListener.g.cs show:

public interface IGKLocalPlayerListener : INativeObject, IDisposable, 
		GameKit.IGKChallengeListener
		, GameKit.IGKInviteEventListener
		, GameKit.IGKSavedGameListener
		, GameKit.IGKTurnBasedEventListener
		
{
}

and

public unsafe partial class GKLocalPlayerListener : NSObject, IGKLocalPlayerListener, IGKChallengeListener, IGKInviteEventListener, IGKSavedGameListener, IGKTurnBasedEventListener {
(...)

Why is adding a new interface a breaking change? It's different from adding a new required member to an existing protocol.

@spouliot
Copy link
Contributor

@VincentDondain oh, that's one we have to tolerate. It's not a problem with the protocol, but with .net.

E.g. If you have an existing binary A.dll with a type that implements IGKLocalPlayerListener and then update to the new XI+1 then this type becomes invalid (because the type does not implement all the interface members).

It's one of place where the (upcoming) default interface methods will help us.

@monojenkins
Copy link
Collaborator

Build success

!missing-protocol-member! MKOverlay::coordinate not found
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future: make sure to end files with a newline to avoid diff noise 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I should stop commenting until I've read all the commits...

@VincentDondain
Copy link
Contributor Author

this type becomes invalid (because the type does not implement all the interface members)

@spouliot but in the case of this PR, there are no new interface members since IGKSavedGameListener is empty. So IGKLocalPlayerListener still implements all the interface members...

Also what did you mean by: "that's one we have to tolerate" ignore the API diff? Special case?

@spouliot
Copy link
Contributor

@VincentDondain api-diff is not that smart :) it's also a bad practice (in .net) to use empty interface (it's better to use attributes). Of course since ours are mapped to protocols we must add them (even if empty).

The tolerance is for the case I mentioned before. We cannot stop adding new protocols even if, in some case, they could break things (we take more care for members).

@VincentDondain
Copy link
Contributor Author

New generated code diff: https://gist.github.com/VincentDondain/dbd1efe6976e748a2d72df10775a4524

A breaking change should be reported for AUViewController, it's expected, the method was moved to the parent, nothing's broken (:


Also I went for the option of inlining the protocol members that weren’t there already.

e.g: https://gist.github.com/VincentDondain/36cd2ca79628bb4b07a5145e46f0150c#file-diff-L638-L677
and https://gist.github.com/VincentDondain/36cd2ca79628bb4b07a5145e46f0150c#file-diff-L146-L164

@monojenkins
Copy link
Collaborator

Build failure

[FAIL] Selector not found for UIKit.UIViewController : objectRestorationClass
[FAIL] Selector not found for UIKit.UIViewController : restorationParent
@monojenkins
Copy link
Collaborator

Build failure

@VincentDondain
Copy link
Contributor Author

Xamarin.iOS.Tasks.TargetTests.RebuildExecutable_NoModifications: #1: ../MySingleView/bin/iPhoneSimulator/Debug/MySingleView.app/MySingleView Expected: 2018-01-13 00:15:29.000 But was: 2018-01-13 00:15:30.000

Known issue https://github.com/xamarin/maccore/issues/592

src/gamekit.cs Outdated
[Export ("player:didModifySavedGame:")]
void DidModifySavedGame (GKPlayer player, GKSavedGame savedGame);

[NoTV]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me go back one step (I might have induced you into an error earlier)

This looks like the protocol does not really exists in tvOS and that Apple just annotated the members and not the type itself. In doubt let's open a radar and add a comment/link into the xtro data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this is confusing.

Also one comment in xtro said:

## all members are not available so the protocol is empty
## however this is confusing because some protocols have no members (so it can't just be ignored)

I was thinking they added the conformance (even if the protocol is empty on tvOS) for later.

But yea I can also revert that, open a radar and annotate xtro.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's tricky since exposing the protocol (if not supported) could be considered a private API (when apps are analyzed) on tvOS. Since I don't see any benefit from an empty protocol then let's play it safe :)

src/webkit.cs Outdated
#if MONOMAC
: NSUserInterfaceValidations
#endif
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: indent

src/wkwebkit.cs Outdated
#if MONOMAC
: NSUserInterfaceValidations
#endif
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: indent

// 'previewItemTitle' should be optional (fixed in XAMCORE_4_0)
return true;
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: that block could have been inside a #if !XAMCORE_4_0

@@ -7,7 +7,6 @@
## all members are not available so the protocol is empty
## however this is confusing because some protocols have no members (so it can't just be ignored)
!missing-protocol! GKFriendRequestComposeViewControllerDelegate not bound
!missing-protocol! GKSavedGameListener not bound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

@monojenkins
Copy link
Collaborator

Build success

@VincentDondain VincentDondain merged commit 4da8016 into xamarin:master Jan 18, 2018
@VincentDondain VincentDondain deleted the bug-59272 branch January 18, 2018 18:42
@@ -28,7 +28,7 @@ namespace XamCore.CoreAudioKit {
#if XAMCORE_2_0 || !MONOMAC
[iOS (9,0)][Mac (10,11, onlyOn64 : true)]
[BaseType (typeof(AUViewControllerBase))]
interface AUViewController : NSExtensionRequestHandling {
interface AUViewController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VincentDondain: This is a breaking change:

screenshot 2018-01-29 10 39 14

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rolfbjarne It's UIViewController that implements NSExtensionRequestHandling now (as part of this PR).

Then based on https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md#types:

Allowed

Removing an interface implementation from a type when the interface is already implemented lower in the hierarchy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I asked about this exact case and you answered me :P

https://xamarinhq.slack.com/archives/C03CCJNR7/p1515786537000437

Vincent:

I’m getting a breaking change from API diff but I don’t understand how come it’s a breaking change
I move a protocol from a subclass to its parent. So the generated method from the protocol is removed from the subclass (that’s what’s reported as a breaking change) but that method is now on the base class…
so even if a subclass of a subclass was overriding that virtual method, it should still be fine
https://gist.github.com/VincentDondain/dbd1efe6976e748a2d72df10775a4524#file-new-diff-L189-L213 and https://gist.github.com/VincentDondain/dbd1efe6976e748a2d72df10775a4524#file-new-diff-L599-L623

Rolf:

You’re assuming apidiff is infallible 🙂
Moving members to a base class is one case where it will say it’s a breaking change when it isn’t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VincentDondain heh, ok, hopefully I'll remember the next time I run into this 😄 👍

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

Successfully merging this pull request may close these issues.

5 participants